cypress-plugin-tab icon indicating copy to clipboard operation
cypress-plugin-tab copied to clipboard

fix: allow tabbing out of [tabindex=-1] elements

Open geebru opened this issue 4 years ago • 3 comments

Description

Definitely just a draft but wanted to get my changes in the hands of the maintainers and others if interest arises to assist in resolving this.

Currently the plugin will error if you focus a focusable, tabindex=-1 element and try to tab out of it. This is incorrect as long as the element is capable of being focused programmatically, which -1 is.

Solutions:

@Bkucera suggested moving from ally.js to tabbable.js but ally.js does include a differentiation query between focusable and tabbable. The changes here allow tabbing out of focusable elements (but will still error if trying to tab from non-focusable elements).

WIP:

Focusing on a -1 element and pressing tab currently goes backwards. I believe this has to do with how the index traversal was setup based on isTabbable and needs to be updated to work based on the next item in the isTabbable tree based on the current isFocusable item.

I'll keep trying things but JS isn't my top language by any means so I definitely welcome help from anyone.

This PR includes a separate HTML in fixtures with a simplified setup, as well a series of rudimentary tests on the fixture. Note that it also adds a skip to the plugins original spec files to speed up the testing flow for this issue and it should be reverted prior to merging (potentially in addition to removing the new HTML/spec files).

geebru avatar May 21 '20 21:05 geebru

@geebru thanks for this. I believe this was my issue with a11y - if the activeElement isn't tabbable (but is focusable), then you can't get the next element in the tabSequence since the current activeElement isn't in there.

I also looked at a11y's context parameter but it seems to only limit the tabSquence to inside an element, not from the element to the end of the DOM.

However looking at the jquery plugin it should be able to query from any element and give us the next tabbable.

kuceb avatar May 22 '20 15:05 kuceb

@Bkucera That makes sense (and good to know the issue I was hitting wasn't just me). I just find it surprising that ally wouldn't have a solution for this since their examples around tabbable and focusable all work as one might expect, so it seems odd that a combination would fail in this way.

I should have some more time next week to look into this more - might take one more stab at ally but then see if I can't get the jquery.tabbable plugin working.

geebru avatar May 22 '20 15:05 geebru

@Bkucera Secondary note - while perusing ally.js issues and backlog things it looks like they have open collaboration with jQuery UI to actually replace JQUI's own focusable/tabbable with ally's versions if I'm reading this right: medialize/ally.js#41

Seems the biggest blocker to that right now is browser support (JQUI down to IE8, ally at IE10) but otherwise the teams seem amicable to the merger.

geebru avatar May 22 '20 15:05 geebru