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

Components: Shift focus into popover when opened #2323

Merged
merged 6 commits into from Sep 7, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Aug 9, 2017

Partially addresses #2306

This pull request seeks to move focus to the first focusable element within a Popover when it becomes opened.

Implementation notes:

Included are some focus utilities for finding the next available focusable element. A few points:

Testing instructions:

Verify that focus is moved to first focusable element when opening Inserter:

  1. Navigate to Gutenberg > New Post
  2. Click the Inserter
  3. Note focus is shifted to the search field (and that search field behavior is otherwise unaffected, including focus constraint looping)

Verify that focus is moved to first focusable element when opening Post Visibility:

  1. Navigate to Gutenberg > New Post
  2. In the sidebar, click Public
  3. Note focus is shifted to the first input option

@aduth aduth requested a review from afercia Aug 9, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 9, 2017

Codecov Report

Merging #2323 into master will increase coverage by 0.49%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
+ Coverage   33.16%   33.65%   +0.49%     
==========================================
  Files         182      185       +3     
  Lines        5530     5574      +44     
  Branches      963      973      +10     
==========================================
+ Hits         1834     1876      +42     
- Misses       3130     3132       +2     
  Partials      566      566
Impacted Files Coverage Δ
editor/inserter/menu.js 48.41% <ø> (ø) ⬆️
utils/focus/tabbable.js 100% <100%> (ø)
utils/focus/test/utils/create-element.js 100% <100%> (ø)
components/popover/index.js 92.68% <90.9%> (-0.28%) ⬇️
utils/focus/focusable.js 93.33% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9b7923...76875ee. Read the comment docs.

codecov bot commented Aug 9, 2017

Codecov Report

Merging #2323 into master will increase coverage by 0.49%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
+ Coverage   33.16%   33.65%   +0.49%     
==========================================
  Files         182      185       +3     
  Lines        5530     5574      +44     
  Branches      963      973      +10     
==========================================
+ Hits         1834     1876      +42     
- Misses       3130     3132       +2     
  Partials      566      566
Impacted Files Coverage Δ
editor/inserter/menu.js 48.41% <ø> (ø) ⬆️
utils/focus/tabbable.js 100% <100%> (ø)
utils/focus/test/utils/create-element.js 100% <100%> (ø)
components/popover/index.js 92.68% <90.9%> (-0.28%) ⬇️
utils/focus/focusable.js 93.33% <93.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9b7923...76875ee. Read the comment docs.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 10, 2017

Contributor

Great work thanks! There are a few edge cases worth considering though.
Probably, the most reliable (and simple) implementation is the one from jQuery UI, I guess it's been tested extensively on the field over time and I'd propose to follow that.
Looking at their implementation, a few examples:

  • links with no href and with a tabindex are focusable
  • AREA elements are focusable if they are inside a named map, have an href attribute, and there is a visible image using the map
  • also, seems to me they check if the element is visible (also ally.js does)

There are also other (very) edge cases for example SVG in IE and scrollable containers in Firefox, not included in jQuery UI but they're included in ally.js see tabbable and focusable.

This might introduce some more complexity, worth considering if using a library could alleviate development (and maintenance) time.

Quoting from jQuery UI:

Note: Elements with a negative tab index are :focusable, but not :tabbable

So, about naming, this should probably be "tabbable".

References:

https://api.jqueryui.com/focusable-selector/
https://github.com/jquery/jquery-ui/blob/74f8a0ac952f6f45f773312292baef1c26d81300/ui/focusable.js

https://api.jqueryui.com/tabbable-selector/
https://github.com/jquery/jquery-ui/blob/74f8a0ac952f6f45f773312292baef1c26d81300/ui/tabbable.js

One more important consideration: there may be cases where it would be desirable to move focus directly on the popover (or modal) wrapper. Imagine a Popover or modal that contains only text. Where should focus be moved to in that case? The Popover has a wrapper div with tabindex=0 that can receive focus and could be used for that.

About the Post Visibility, I have a doubt: focus is moved to the first radio button, as expected. But that happens also when it's not the selected radio button:

screen shot 2017-08-10 at 18 30 58

This is different from the native "tabbing" behavior: when tabbing to a set of radio buttons, only the selected one receives focus. The other ones receive focus when using the arrow keys. I think we cam probably live with this, just wanted to mention it as something worth some testing.

Contributor

afercia commented Aug 10, 2017

Great work thanks! There are a few edge cases worth considering though.
Probably, the most reliable (and simple) implementation is the one from jQuery UI, I guess it's been tested extensively on the field over time and I'd propose to follow that.
Looking at their implementation, a few examples:

  • links with no href and with a tabindex are focusable
  • AREA elements are focusable if they are inside a named map, have an href attribute, and there is a visible image using the map
  • also, seems to me they check if the element is visible (also ally.js does)

There are also other (very) edge cases for example SVG in IE and scrollable containers in Firefox, not included in jQuery UI but they're included in ally.js see tabbable and focusable.

This might introduce some more complexity, worth considering if using a library could alleviate development (and maintenance) time.

Quoting from jQuery UI:

Note: Elements with a negative tab index are :focusable, but not :tabbable

So, about naming, this should probably be "tabbable".

References:

https://api.jqueryui.com/focusable-selector/
https://github.com/jquery/jquery-ui/blob/74f8a0ac952f6f45f773312292baef1c26d81300/ui/focusable.js

https://api.jqueryui.com/tabbable-selector/
https://github.com/jquery/jquery-ui/blob/74f8a0ac952f6f45f773312292baef1c26d81300/ui/tabbable.js

One more important consideration: there may be cases where it would be desirable to move focus directly on the popover (or modal) wrapper. Imagine a Popover or modal that contains only text. Where should focus be moved to in that case? The Popover has a wrapper div with tabindex=0 that can receive focus and could be used for that.

About the Post Visibility, I have a doubt: focus is moved to the first radio button, as expected. But that happens also when it's not the selected radio button:

screen shot 2017-08-10 at 18 30 58

This is different from the native "tabbing" behavior: when tabbing to a set of radio buttons, only the selected one receives focus. The other ones receive focus when using the arrow keys. I think we cam probably live with this, just wanted to mention it as something worth some testing.

@afercia

Thanks! If we want a simple, custom, implementation I'd consider to address just a couple edge cases.
For the future: maybe consider to just include ally.js.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 11, 2017

Member

Incidentally this will fix an issue with the content Inserter Menu opening atop the button. Seems the div wrapper introduced in #2321 is causing some issues with Popover being able to calculate its offset position. I think eventually we'll want to move withFocusReturn to be part of Popover itself, which should hopefully provide a better long-term solution.

Member

aduth commented Aug 11, 2017

Incidentally this will fix an issue with the content Inserter Menu opening atop the button. Seems the div wrapper introduced in #2321 is causing some issues with Popover being able to calculate its offset position. I think eventually we'll want to move withFocusReturn to be part of Popover itself, which should hopefully provide a better long-term solution.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 14, 2017

Member

The more I dig into ally.js, the more I feel it's not a good use of our time to try to reimplement something like tabbable querying from scratch. I am still quite wary about the size of the library, and have tried a few different approaches for remedying. The full library is about 75kb minified. Cherry-picking the query/first-tabbable module is still non-trivial but better: about 46kb minified when brought in (the impact won't be quite as large if we choose to opt in to a few more of its methods). Alternatively we could split off ally into its own vendor dependency, which has the added advantage of being consumable by other plugins.

Appears the latest commits are failing because of the ES2015 modules in the dependency. I'm going to sit on this for a bit to think about a direction forward.

Let me know if you have thoughts.

Member

aduth commented Aug 14, 2017

The more I dig into ally.js, the more I feel it's not a good use of our time to try to reimplement something like tabbable querying from scratch. I am still quite wary about the size of the library, and have tried a few different approaches for remedying. The full library is about 75kb minified. Cherry-picking the query/first-tabbable module is still non-trivial but better: about 46kb minified when brought in (the impact won't be quite as large if we choose to opt in to a few more of its methods). Alternatively we could split off ally into its own vendor dependency, which has the added advantage of being consumable by other plugins.

Appears the latest commits are failing because of the ES2015 modules in the dependency. I'm going to sit on this for a bit to think about a direction forward.

Let me know if you have thoughts.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 19, 2017

Contributor

The full library is about 75kb minified

Thinking there are several places in core where there's the need to get the focusable/tabbable elements, filter out the first/last ones, etc. and core is currently doing that with custom implementations that could be replaced with the methods that ally.js provides. In the future, with the advent of JS-based interfaces in core, there will be more and more the need for such methods. I'd propose to consider to adopt ally.js as a core library, and I think 75kb are worth the benefits ally.js introduces.

Contributor

afercia commented Aug 19, 2017

The full library is about 75kb minified

Thinking there are several places in core where there's the need to get the focusable/tabbable elements, filter out the first/last ones, etc. and core is currently doing that with custom implementations that could be replaced with the methods that ally.js provides. In the future, with the advent of JS-based interfaces in core, there will be more and more the need for such methods. I'd propose to consider to adopt ally.js as a core library, and I think 75kb are worth the benefits ally.js introduces.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 30, 2017

Member

I revisited this today and spent some more cycles on a few different ideas. Assuming most of our requirements are around finding focusables or tabbables, or some subset of the two, I took to seeing if we could at least use a smaller subset of the ally.js functionality in an minimal abstraction. I quickly found that the dependency graph of their modules are very far-reaching and struggled to not bring in hundreds of kilobytes with even the most targeted of imports.

Instead, I took another stab at another self-implementation, this time much simpler using a querySelector selector capturing focusable elements defined by the specification, including quirks around area elements, visibility, etc.

I'm inclined to see how far this takes us, and if it becomes an issue, we can easily swap the underlying implementation, or use ally more directly.

Member

aduth commented Aug 30, 2017

I revisited this today and spent some more cycles on a few different ideas. Assuming most of our requirements are around finding focusables or tabbables, or some subset of the two, I took to seeing if we could at least use a smaller subset of the ally.js functionality in an minimal abstraction. I quickly found that the dependency graph of their modules are very far-reaching and struggled to not bring in hundreds of kilobytes with even the most targeted of imports.

Instead, I took another stab at another self-implementation, this time much simpler using a querySelector selector capturing focusable elements defined by the specification, including quirks around area elements, visibility, etc.

I'm inclined to see how far this takes us, and if it becomes an issue, we can easily swap the underlying implementation, or use ally more directly.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 7, 2017

Member

One more important consideration: there may be cases where it would be desirable to move focus directly on the popover (or modal) wrapper. Imagine a Popover or modal that contains only text. Where should focus be moved to in that case? The Popover has a wrapper div with tabindex=0 that can receive focus and could be used for that.

Added in 76875ee.

Will also plan to follow-up this pull request with another for close-on-escape and focus return behaviors.

Member

aduth commented Sep 7, 2017

One more important consideration: there may be cases where it would be desirable to move focus directly on the popover (or modal) wrapper. Imagine a Popover or modal that contains only text. Where should focus be moved to in that case? The Popover has a wrapper div with tabindex=0 that can receive focus and could be used for that.

Added in 76875ee.

Will also plan to follow-up this pull request with another for close-on-escape and focus return behaviors.

// Find first tabbable node within content and shift focus, falling
// back to the popover panel itself.
const firstTabbable = focus.tabbable.find( content )[ 0 ];

This comment has been minimized.

@youknowriad

youknowriad Sep 7, 2017

Contributor

We could try to reuse this utility in WritingFlow. I don't know how well this will behave compared to our current selector.

@youknowriad

youknowriad Sep 7, 2017

Contributor

We could try to reuse this utility in WritingFlow. I don't know how well this will behave compared to our current selector.

This comment has been minimized.

@aduth

aduth Sep 7, 2017

Member

We could try to reuse this utility in WritingFlow.

I agree, it would be good to have a consistent, well-tested approach for this.

I don't know how well this will behave compared to our current selector.

Better (or more accurate anyways), I hope 😬

@aduth

aduth Sep 7, 2017

Member

We could try to reuse this utility in WritingFlow.

I agree, it would be good to have a consistent, well-tested approach for this.

I don't know how well this will behave compared to our current selector.

Better (or more accurate anyways), I hope 😬

This comment has been minimized.

@youknowriad

youknowriad Sep 7, 2017

Contributor

I'll try it ;)

@youknowriad

youknowriad Sep 7, 2017

Contributor

I'll try it ;)

@aduth aduth merged commit c6ceb8b into master Sep 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the fix/popover-open-focus branch Sep 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment