Skip to content

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

Closed
geebru wants to merge 4 commits into
kuceb:masterfrom
geebru:tabbable-fix
Closed

fix: allow tabbing out of [tabindex=-1] elements#24
geebru wants to merge 4 commits into
kuceb:masterfrom
geebru:tabbable-fix

Conversation

@geebru

@geebru geebru commented May 21, 2020

Copy link
Copy Markdown

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).

@kuceb

kuceb commented May 22, 2020

Copy link
Copy Markdown
Owner

@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.

@geebru

geebru commented May 22, 2020

Copy link
Copy Markdown
Author

@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

geebru commented May 22, 2020

Copy link
Copy Markdown
Author

@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.

@kuceb

kuceb commented Mar 1, 2026

Copy link
Copy Markdown
Owner

I've integrated this fix into #65

@kuceb kuceb closed this Mar 1, 2026
@kuceb kuceb mentioned this pull request Mar 2, 2026
kuceb added a commit that referenced this pull request Mar 2, 2026
Long-awaited Cypress 10+ Support for cy.tab()


- remove Ally.js dependency (abandoned) 
  - closes #59 
  - closes #37  
- fix focus randomly being lost due (due to ally.js) 
  - closes #46  
- upgrade to Cypress 10+, with new compatible type definitions 
  - closes #6 
  - closes #40 
- fix tabbing from focusable elements 
  - fixes #18 
  - subsumes #24  
- follow subject changes in Cypress command chain / events sent in proper order before focus change 
  - closes #47
- fix hanging tests when used with cy.clock 
  - closes #42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants