Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeyNav: focus jumps when user clicks in an accordion panel #462

Closed
wkeese opened this issue Aug 23, 2016 · 1 comment
Closed

KeyNav: focus jumps when user clicks in an accordion panel #462

wkeese opened this issue Aug 23, 2016 · 1 comment

Comments

@wkeese
Copy link
Member

wkeese commented Aug 23, 2016

There's a bug with deliteful/Accordion where clicking a focusable element inside an Accordion panel can make focus jump to the most recently focused Accordion header. There also appear to be some less serious issues where TAB/SHIFT-TAB will unwantedly stop on the Accordion root node, or the root of an Accordion header.

Why does this happen? Accordion extends KeyNav. It's a complicated case because half of the Accordion children are [role=tab] headers, which match descendantSelector and are navigable via KeyNav up/down arrows, but the other half of the Accordion children are panels, which are not navigable through KeyNav.

This is the current KeyNav focusin handling code:

this.on("focusin", function (evt) {
    if (this.focusDescendants) {
        var target = this._getTargetElement(evt);
        if (target === this) {
            this._keynavFocusHandler(evt);
        } else {
            this._descendantNavigateHandler(target, evt);
        }
    }
}.bind(this), this.keyNavContainerNode);

The main issue is that when an Accordion panel's descendant gets focus, KeyNav calls this._keynavFocusHandler(evt). But _keynavFocusHandler() should only be called when the KeyNav (root node) itself gets focus, because it shifts focus to one of the descendants. Also, its API doc says "Handler for when the container itself gets focus.".

However, whenever a descendant element of the KeyNav is focused, KeyNav should remove its own tabindex, so that Shift-TAB works as expected, i.e. leaving the KeyNav completely without stopping at the KeyNav root node. The code to remove the KeyNav's root node tabIndex is currently in _keynavFocusHandler() and _descendantNavigateHandler().

A third possible issue is that _descendantNavigateHandler() changes the Accordion header from tabindex=-1 to tabindex=0 even when a node inside of the header got focused. That could lead to unwanted tab stops. http://www.oaa-accessibility.org/examplep/accordian1/ shares the same problem: clicking one of the radio buttons and then pressing SHIFT-TAB focuses the first accordion header. It probably shouldn't. But have to read https://www.w3.org/TR/wai-aria-practices-1.1/#accordion carefully as I'm not sure what makes sense here. It probably doesn't matter much since users don't usually mix mouse and keyboard.

Actually, it would be better to temporarily clear the tabIndex on the Accordion header altogether, as Firefox and Safari have issues with focusable elements inside of other focusable elements. On those browsers, clicking the <button> in this test case actually focuses the <div>:

<div tabindex="0" style="width: 300px;">
     i'm a div, including a button:
    <button>click me</button>
</div>

(That's explained on Safari because it doesn't focus <button> by default but Firefox is just weird.)

@wkeese wkeese changed the title KeyNav: focus jumps when user clicks an accordion panel KeyNav: focus jumps when user clicks in an accordion panel Aug 23, 2016
wkeese added a commit to wkeese/delite that referenced this issue Aug 25, 2016
wkeese added a commit to wkeese/delite that referenced this issue Aug 26, 2016
Also updated delite/on to use native focusin/focusout events when possible
(AFAICT only on IE.)  There's no reason not to, and it avoids an IE problem
where the blur event doesn't set evt.relatedTarget.

Finally, fix bug where d-active-descendant class wasn't removed
upon tabbing out of the entire KeyNav.

Fixes ibm-js#462.
wkeese added a commit to wkeese/delite that referenced this issue Aug 26, 2016
Also updated delite/on to use native focusin/focusout events when possible
(AFAICT only on IE.)  There's no reason not to, and it avoids an IE problem
where the blur event doesn't set evt.relatedTarget.

Finally, fix bug where d-active-descendant class wasn't removed
upon tabbing out of the entire KeyNav.

Fixes ibm-js#462.
wkeese added a commit to wkeese/delite that referenced this issue Aug 26, 2016
Also updated delite/on to use native focusin/focusout events when possible
(AFAICT only on IE.)  There's no reason not to, and it avoids an IE problem
where the blur event doesn't set evt.relatedTarget.

Finally, fix bug where d-active-descendant class wasn't removed
upon tabbing out of the entire KeyNav.

Fixes ibm-js#462.
@wkeese wkeese closed this as completed in 3b7c96f Aug 28, 2016
wkeese added a commit that referenced this issue Dec 5, 2016
…ivated event.

The problem as seen on deliteful/Accordion was:

1. User tabs to Accordion.   This causes a "focusin" event and then a "focus" event.
2. Accordion "focusin" event triggers KeyNav#focusinHandler().
3. KeyNav#focusinHandler() calls KeyNav#focus() which eventually calls AccordionHeader#focus().
4. AccordionHeader gets "focus" event which makes activationTracker send "delite-activated" event to AccordionHeader.
5. Accordion "focusin" event processing finally finished.
6. Accordion "focus" event processing starts.   That makes activationTracker send "delite-deactivated"
    event to AccordionHeader because it thinks focus has shifted back to the Accordion itself.

Refs #462.
wkeese added a commit that referenced this issue Dec 7, 2016
wkeese added a commit to ibm-js/deliteful that referenced this issue Dec 7, 2016
brunano21 pushed a commit to brunano21/deliteful that referenced this issue Dec 7, 2016
brunano21 pushed a commit to brunano21/deliteful that referenced this issue Dec 7, 2016
wkeese added a commit to ibm-js/deliteful that referenced this issue Dec 7, 2016
wkeese added a commit to wkeese/deliteful that referenced this issue Dec 27, 2016
and fix Accordion shift-tab related issues.

Also, change tabbing into Accordion to go to header for first open panel,
rather than most recently focused header.
That makes shift-tab into the Accordion somewhat more predictable,
although still not perfect.  Perfect would require differentiating between shift-tab
into the Accordion vs. clicking into the Accordion, and that would require adding
a template with a containerNode and a 0x0 focusable <span> after the containerNode.

Fixes ibm-js#681, refs ibm-js#679, ibm-js/delite#462.
wkeese added a commit to wkeese/delite that referenced this issue Dec 28, 2016
wkeese added a commit to wkeese/deliteful that referenced this issue Dec 28, 2016
and fix Accordion shift-tab related issues.

Also, change tabbing into Accordion to go to header for first open panel,
rather than most recently focused header.
That makes shift-tab into the Accordion somewhat more predictable,
although still not perfect.  Perfect would require differentiating between shift-tab
into the Accordion vs. clicking into the Accordion, and that would require adding
a template with a containerNode and a 0x0 focusable <span> after the containerNode.

Fixes ibm-js#681, refs ibm-js#679, ibm-js/delite#462.
wkeese added a commit that referenced this issue Dec 29, 2016
wkeese added a commit to ibm-js/deliteful that referenced this issue Dec 29, 2016
and fix Accordion shift-tab related issues.

Also, change tabbing into Accordion to go to header for first open panel,
rather than most recently focused header.
That makes shift-tab into the Accordion somewhat more predictable,
although still not perfect.  Perfect would require differentiating between shift-tab
into the Accordion vs. clicking into the Accordion, and that would require adding
a template with a containerNode and a 0x0 focusable <span> after the containerNode.

Fixes #681, refs #679, ibm-js/delite#462.
wkeese added a commit that referenced this issue Jan 10, 2017
make the List non-tab-navigable in the future.
It was affecting at least the List inside the deliteful ComboPopup
(used by mobile Combobox).  See deliteful/tests/functional/ComboPopup.html.

The problem occurred in KeyNav#focusoutHandler(), when there’s a focusout
event on the category renderer.  Since the category renderer is not
tab navigable, previouslyNavigatedDescendant was computed to be “this”
(i.e. the List itself) rather than the category renderer.  At this point
focusoutHandler() should have skipped the section for handling defocused
descendants, but it didn’t, due to a bug from 3e0192d,
which said:

        previouslyNavigatedDescendant !== this.keyNavContainerNode

but should have said:

        previouslyNavigatedDescendant !== this

So, KeyNav#focusOutHander() incorrectly set this.tabIndex = “-1”.
Then due to the tabIndex={{tabIndex}} in the List’s template, on the next
refreshRendering() cycle, this.containerNode.tabIndex was set to -1.

Although KeyNav#focusoutHandler() has code to set this.containerNode's
tabIndex to this._savedTabIndex, that code ran *before* the refreshRendering()
call that updated the DOM according to the template.

I fixed all this by changing _getTargetElement() to return null when the event
is not on one of the List's navigable descendants.

Refs #462, #473.
wkeese added a commit to ibm-js/deliteful that referenced this issue Jan 10, 2017
not the first category, ex "France" not "EU".

Not sure if that matters since ComboPopup is only for mobile, but in general
type=listbox Lists are only supposed to have navigation on the list items,
not on the categories.

Tangentially refs ibm-js/delite#462, ibm-js/delite#473.
@wkeese
Copy link
Member Author

wkeese commented Sep 15, 2019

Note that on Firefox (at least Firefox 69 on Mac), although you can focus a button by tabbing to it, clicking a button never focuses it. Even in the simple case when the container doesn't have a tabindex, focus will simply go to the <body>:

<div style="width: 300px;">
     i'm a div, including a button:
    <button>click me</button>
</div>

A <span role=button tabindex=0> will work exactly how we want it, both on Firefox and Safari.

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

No branches or pull requests

1 participant