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

focus.tabbables fails when the tabbables collection is made of only radio buttons #6988

Closed
afercia opened this issue May 28, 2018 · 7 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] DOM /packages/dom [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented May 28, 2018

Splitting this out from #6987

There's a problem with the focus.tabbables implementation when the tabbables are exclusively radio buttons. This is clearly visible in the "Visibility setting" when switching to the #6987 branch (see also screenshot below).

Radio buttons are technically "tabbables" but in a set of grouped radio buttons it's actually possible to tab into just one of them. In this case, the current implementation fails as focus.tabbables returns a collection of 3 elements but only one of them can be "tabbed" into:

screen shot 2018-05-28 at 17 17 14

While technically all the radio buttons are tabbable, the current behavior doesn't help so much and in some cases it defeats the purpose of having such a tool. Gutenberg uses focus.tabbables mainly to get the next or previous tabbable elements and manage focus accordingly.

In the above scenario, the three radio buttons should be considered 1 tabbable element, to match the native browsers behavior.

@afercia afercia added the [Type] Bug An existing feature does not function as intended label May 28, 2018
@aduth
Copy link
Member

aduth commented Jun 28, 2018

A challenge here is directionality. For example, in the WritingFlow we move to the last tabbable when tabbing in reverse. focus.tabbable doesn't consider direction, so if we collapsed sequences of radio inputs to a single one (presumably the first), then shift-tabbing to a radio group would incorrectly move focus to the first radio button, instead of the last in the set.

Demo / Playground: https://codepen.io/aduth/pen/pKQKvm

@aduth
Copy link
Member

aduth commented Jun 28, 2018

One possibility: We add a nextTabbable( node ) / previousTabbable( node ) utility into the module to handle the directionality behaviors. Unsure how much of this could be reused from WritingFlow's getClosestTabable which has additional logic to "prefer" fields instead of the block's focus stop. Perhaps isTabCandidate can be an argument to this utility function.

@aduth aduth added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] DOM /packages/dom labels Aug 23, 2018
@afercia
Copy link
Contributor Author

afercia commented Feb 26, 2019

To clarify, the practical consequence of this in the Visibility panel is that tabbing with the keyboard is not constrained within the panel. For example:

  • when "Public" is selected, focus.tabbables thinks there are 3 tabbable elements
  • the browser disagrees, as there's just one radio button which is "tabbable" at a time

So this is a case where constraining tabbing fails and screen reader users get lost.

Second example:

  • select the last radio button "Password Protected"
  • an additional input field for the password appears
  • tab with the keyboard
  • tabbing is now constrained but: it goes from the input to the first radio button, which is not the one that should be tabbable

@aduth
Copy link
Member

aduth commented Feb 26, 2019

In the above scenario, the three radio buttons should be considered 1 tabbable element, to match the native browsers behavior.

Considering the aforementioned complexities of directionality, would it be considered an improvement if tabbables considered all radio elements of the same name as a single element, where the chosen element is either the checked input (if available) or otherwise the first in the set?

It's non-perfect in that in a browser, Shift+Tab to focus into a radio group where there is no checked input would normally select the last radio in the DOM preceding the current focused element. But it would be relatively more trivial to implement this way.

Form with checked radio group: https://codepen.io/aduth/pen/xBbWrm
Form without checked radio group: https://codepen.io/aduth/pen/LaEdLv

I've sifted through a few relevant specifications ([1] [2]) but am yet to find where this particular behavior is prescribed. If it's not specified, it wouldn't surprise me then if this behavior differed across browsers.

@afercia
Copy link
Contributor Author

afercia commented Feb 26, 2019

Thanks for the codepens, really useful. Yep in the "without checked radio group" the behavior is different in Chrome and Firefox. On the other hand, a group of radio buttons should always have one of them selected. As you pointed out, in that case the expected behavior is unspecified and left to browsers implementations so... it differs 🙂 It's also a not ideal UI so that should be avoided.

Ideally, the unselected state of a radio buttons group shouldn't be allowed in Gutenberg, at least in the documentation.

That said, I'd tend to think your proposal is the best option available +1 !

@aduth
Copy link
Member

aduth commented Feb 28, 2019

From an already-interesting React Focus Management RFC, I was led to the following recommendation:

The convention for where focus lands in a composite when it receives focus as a result of a Tab key event depends on the type of composite. It is typically one of the following.

  • The element that had focus the last time the composite contained focus. Or, if the composite has not yet contained the focus, the first element. Widgets that usually employ this pattern include grid and tree grid.
  • The selected element. Or, if there is no selected element, the first element. Widgets where this pattern is commonly implemented include radio groups, tabs, list boxes, and trees. Note: For radio groups, this pattern is referring to the checked radio button; the selected state is not supported for radio buttons.
  • The first element. Components that typically follow this pattern include menubars and toolbars.

https://www.w3.org/TR/wai-aria-practices-1.1/#kbd_general_within

The proposal in #14128 abides by the second.

@afercia
Copy link
Contributor Author

afercia commented Feb 28, 2019

Interesting. From the comments on the same React RFC proposal:

Focus management is there to repair the standard DOM focus when we disrupt it with our code.

Yes. 🙂

Also, a very interesting consideration about portals, focus, and tab order (this was discussed at lenght for the popovers): reactjs/rfcs#104 (comment)

Besides that, worth noting the ARIA Authoring Practices relates to ARIA widgets. The Section "Keyboard Navigation Inside Components" relates to custom components built with ARIA and doesn't directly applies to native elements and how browsers implement them.

The closest thing I've found about native radio buttons is from the old HTML 4.0.1 recommendation:

https://www.w3.org/TR/html401/interact/forms.html#h-17.2.1

If no radio button in a set sharing the same control name is initially "on", user agent behavior for choosing which control is initially "on" is undefined. Note. Since existing implementations handle this case differently, the current specification differs from RFC 1866 ([RFC1866] section 8.1.2.4), which states:

At all times, exactly one of the radio buttons in a set is checked. If none of the elements of a set of radio buttons specifies `CHECKED', then the user agent must check the first radio button of the set initially.

Since user agent behavior differs, authors should ensure that in each set of radio buttons that one is initially "on".

The above basically just acknowledged browsers have different implementations. That said, #14128 is the best option!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] DOM /packages/dom [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

2 participants