Components: Add onFocusLoss option to withFocusReturn

[aduth]

Fixes #1918
Addresses #10215 (comment)

This pull request seeks to enhance withFocusReturn to handle cases of focus loss by accepting an onFocusLoss handler. This handler can be passed as an option when calling withFocusReturn, or applied as fallback on a virtual hierarchy using the new wp.components.FocusReturnProvider context component. Furthermore, the Modal component accepts the onFocusLoss prop to allow customized handling.

To this end, the following behaviors take place:

• When a modal opened through "More Menu" options is closed, the "More Menu" button is focused
• When the sidebar is closed, it will direct focus to the Settings button if focus loss were to otherwise occur
• All other focus loss (if exists, not intended) would redirect focus to the edit-post-header wrapper

In all cases, the specific behavior can be customized if there's a more suitable alternative presented.

Pending tasks:

This could do for some improved unit and/or end-to-end tests, pending feedback on direction.

Testing instructions:

Repeat Steps to Reproduce from #1918 and #10215 (comment), verifying that focus loss does not occur.

[aduth]

@gziolo mentioned an alternative take which seemed interesting: If the idea with suggesting that focus be placed on More Options menu button when a modal is closed is on the basis that it's the next best place as far as being the most recent still-present element in the user's focus history, maybe we can represent this as some sort of stack where the implementation of focus return is to pop the stack until it finds something which still exists in the DOM to which to direct focus.

The upsides to this are decreased reliance on DOM selectors (a maintenance concern), simpler implementation for developers using withFocusReturn, and a more faithful representation of the thinking behind the More Options button being the "closest" focus return location.

[gziolo]

This PR needs a rebase.

I reviewed the code and shared the idea about a different approach with @aduth as explained above. It would be more complex than the current proposal but might have better ergonomics for plugin developers as it would (hopefully) magically work behind the scenes. The issue is that it is still an assumption. This PR offers very solid fix which I'm happy with. It resolves the issue completely and can be included in the upcoming Gutenberg release, as well as, it would be part of WordPress 5.2 release. I think we should land this PR and keep looking into alternatives while users can benefit from the fact that bug is resolved.

[gziolo]

Consumer is never used outside of this file.

[gziolo]

There are 2 failing snapshot tests: https://travis-ci.com/WordPress/gutenberg/jobs/185021828#L5512

They have to be regenerated since modals are now wrapped with a different component.

Aside: this is the part of snapshot testing which is quite annoying and introduces unnecessary noise.

[afercia]

Ideally, focus should be moved back to the "invoking context", i.e.: the control that opened the modal.

I'd like to point out again that this is more a design issue rather than a technical one. Opening a modal from a dropdown menu that closes as soon as the modal opens doesn't seem the best idea to start with.

This should have been taken into consideration when this feature was designer. It's a perfect case to explain why accessibility specialists stress that accessibility should be designed and implemented since the beginning.

I really appreciate the effort to solve this issue. I'd just like to invite everyone to consider that sometimes the best solution doesn't need to be a complicated, technical, one. Sometimes, keeping things simple and solving the issue in the design is way better. Just saying because sometimes I see a certain tendency to solve issues only with advanced technicalities. Instead, accessibility is more about simplicity.

I do realize proposing relevant design changes would bring us into endless discussions. If there are no better ideas (e.g. can we keep the menu open?), the only option I can see is a compromise. It's not ideal for accessibility but it's better than what we have now. A fallback to the "more options" button would be a sort of extended concept of "invoking context".

[aduth]

I've pushed a few revisions:

• I introduced the idea of the focus history stack, tracked via and passed down from FocusReturnProvider. While there's some complexity in the implementation of managing the stack history, on the consuming end it simplifies the use of FocusReturnProvider, as the developer no longer needs to provide their own explicit fallback implementation. Outside of further design consideration, I think this is a faithful representation of the idea of the extended concept of "invoking context".
• In working at this, I realized that the regression described at Sidebar: withFocusReturn doesn't work when the Sidebar is already open #1918 (comment) appeared to be a consequence of the fact that we at some point changed the implementation of the sidebar to apply withFocusReturn to its fill, rather than its actual rendered DOM element. The fill caused focus to not be tracked correctly. This should be resolved as of b491faf.

I'd encountered a failure in one of the unit tests for withFocusReturn, but on evaluating it, it doesn't seem like it's testing an expected behavior, so I opted to remove it.

gutenberg/packages/components/src/higher-order/with-focus-return/test/index.js

Lines 74 to 85 in 39bd834

	it( 'should return focus to element associated with HOC', () => {
	const mountedComposite = renderer.create( <Composite /> );
	expect( getInstance( mountedComposite ).activeElementOnMount ).toBe( activeElement );
	// Change activeElement.
	document.activeElement.blur();
	expect( document.activeElement ).toBe( document.body );
	// Should return to the activeElement saved with this component.
	mountedComposite.unmount();
	expect( document.activeElement ).toBe( activeElement );
	} );

(It appears to test that focus would be forcibly returned even after the element had been blurred?)

I still want to add some additional test cases here, however.

[aduth]

Another note: I didn't do away with the idea of an onFocusLoss option entirely. For the sidebar specifically, I chose to implement it as a onFocusReturn override handler instead, to explicitly associate the sidebar with the menu button. There's otherwise no way for the "focus history" to place focus on the button when the sidebar is closed if, for example, the sidebar had been initially open at the start of the session (#1918).

[afercia]

Tested and works nicely as expected. Design considerations apart, I think the idea of a focusHistory "stack" is very valuable at the point I'd like to see it introduced in core in the future.

I'd consider to better highlight this feature in the readme, if possible.

Thanks also for fixing the regression described at #1918 (comment)

From an accessibility perspective 👍 I'll leave code related considerations to coders more skilled than me 🙂

[aduth]

I pushed a few more revisions:

• Focus history now only considers the last unique occurrence of an element having been focused. The original implementation was mindful of the fact that over a long session, an uncapped stack could become a source of memory exhaustion. It was thus capped at remembering the last 100 focused elements. However, I was concerned about scenarios wherein a user might cycle through the same set of focused elements and exhaust this limit; an example being holding Tab while the options menu is opened. Keeping only the unique members should avoid the issue where this limit being exceeded could inevitably cause a focus loss if every element in the focus history were to be removed at the same time.
• In the course of adding tests, I observed a potential issue where in the focus return behavior, it was considering candidates which were about to be removed. This is sensible, as those elements are technically still in the DOM when componentWillUnmount is called, but this is immediately prior to them being removed. The workaround was for withFocusReturn to monitor elements it's seen as having been focused within its own element tree, and excluding those from consideration when returning focus. It's a bit puzzling to me that this still seemed to work as expected without this change, but hopefully this makes the implementation a bit more durable.
• The new FocusReturnProvider and onFocusReturn options are documented in 015ad3d

The test coverage is still lacking here, but depending on if this is needed for feature freeze (vs. qualifying for a subsequent bug fix release), I'd be inclined to suggest those be tackled separately.

[gziolo]

All applied changes. since I reviewed last time, seem to look good. I would appreciate some sanity check for the updated implementation of withFocusReturn and its new provider. @youknowriad or @ellatrix?

[youknowriad]

What happens if I use two of these providers? Say each BlockEditorProvider uses one?

[aduth]

What happens if I use two of these providers? Say each BlockEditorProvider uses one?

Generally, I'd say it should probably be avoided, if possible. The component should fall under a "one-per-app" recommendation.

That said, in practice it shouldn't be too problematic. Any withFocusReturn-enhanced component would rely on its closest provider as a source to determine where focus returns. The only potential problem is that the highest provider would still detect focus changes into an area technically governed by another provider, so they're not mutually exclusive. It's hard to imagine it would result in any unexpected behavior, though.

[youknowriad]

LGTM 👍