Accessibility: Add disposable focus Higher Order Component #901

youknowriad · 2017-05-25T09:20:51Z

closes #895

This PR adds a generic disposableFocus higher order component.
A component wrapped in the "disposableFocus" hoc will have the following behaviour:

Saves the active element when the disposable element is mounted
Restores the active element when the disposable element is unmounted (unless the focus has moved outside the disposable component)

All disposable elements should be wrapped within this HoC: Thinking of inserter, sidebar...

jasmussen · 2017-05-25T09:34:23Z

Thanks for working on this. I'm not entirely convinced how to test this exactly, so would appreciate @afercia's eyes as well. The inserter seems to be working well (outside of that focus rectangle which is my fault, and for which I'm opening a separate ticket for soon).

Codewise, I like all the red I see this clearing up!

afercia · 2017-05-25T12:21:12Z

Looks very promising to me, as it's a simple way to get an UI control to return focus to later. Tested on both the Inserter and Sidebar, and focus is correctly moved back to the related buttons. About the Sidebar for example, it solves the focus loss pointed out in #895

I'd like to see this as the first tool of a more complete "focus management" set of tools.

Just one thing, I'd consider to don't use a <div> element for the wrapper, since it could happen to use it in places where a <div> is not allowed and would cause invalid HTML. After all this wrapper is not meant for layout purposes and could be, for example, a span.

Not sure about the naming 🙂 maybe disposableFocus is not so clear, but I wouldn't know what other naming to use. Minor point anyways.

youknowriad · 2017-05-25T12:26:32Z

Just one thing, I'd consider to don't use a <div> element for the wrapper

Unfortunately, it's not possible to avoid a wrapper here. The wrapper tagName could be made customizable though (if we need to). We need a wrapper to get a reference to the dom node.

youknowriad · 2017-05-25T12:42:04Z

Actually, I was able to drop the wrapper in 74f0dc8

afercia · 2017-05-25T12:42:05Z

Sorry, just noticed a drawback: when clicking using the mouse, document.activeElement returns the body element in Firefox and Safari. Returns the button in Chrome. Haven't tested in IE.

The previous method used for the inserter worked also when clicking with the mouse, because it was using the click event current target.

It would be great to find a way to make this work both with mouse and keyboard!

youknowriad · 2017-05-25T12:48:30Z

@afercia Good catch! I found that this happening because when we click on the "Insert" Button for example, the focus is not being moved to the button in Firefox and Safari while it's done in Chrome. I can't find yet why the focus is not moved to buttons when clicking on them.

youknowriad · 2017-05-25T12:53:06Z

I found this link showing these differences. thinking how can we make this work the Chrome way everywhere https://gist.github.com/cvrebert/68659d0333a578d75372

aduth · 2017-05-25T12:56:20Z

components/disposable-focus/index.js

+		}
+
+		componentWillUnmount() {
+			const wrapper = findDOMNode( this );


findDOMNode is documented as an escape hatch, though in this case I think a reasonable one. Concerns me to see some conversation in which React maintainers appear inclined to put it on a path toward deprecation.

I noted the earlier point about wrapper components. I think we should keep this as-is, but leave open as options for future consideration:

Wrap with span to reduce layout implications (inline, not block)

Expect the mounted component to define a node getter (similar to react-click-outside's handleClickOutside)

Pass callback as prop to the wrapped component, expected to be called on mount with reference to the DOM node

I was thinking of providing a config object as a parameter to the HoC as an alternative.

disposableFocus({ wrapper: 'span' })(Component)

youknowriad · 2017-05-25T12:57:02Z

@afercia Do you agree that buttons should receive the focus when we click on them all the time (like chrome does)? In that case, we can add a onClick handle to focus the buttons in a generic way in the Button component to mimic the Chrome's behaviour?

Thoughts on this @aduth?

aduth · 2017-05-25T12:57:41Z

components/disposable-focus/index.js

@@ -0,0 +1,32 @@
+/**


Re: Folder placement. This is technically not a component, but rather a function which returns a modified component. Dunno if or how we'd want to distinguish this.

aduth · 2017-05-25T12:58:20Z

editor/inserter/index.js

 	}

-	toggle( event ) {
-		// When opening the menu, track reference to the current active element


I think some of the inline documentation could be adapted for use in the new higher-order variant.

afercia · 2017-05-25T13:24:52Z

Do you agree that buttons should receive the focus when we click on them all the time (like chrome does)?

Just tested Firefox on Windows and document.activeElement correctly returns the buttons also when clicking with the mouse. 😬 That confirms what reported in the link you posted, data actually comes from MDN, see: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus

Ideally, I think it would make sense to try to pair this behaviour across platforms/browsers.

afercia · 2017-05-25T13:32:24Z

Tested also in:
Edge: works
IE 11: can't test, still getting Object doesn't support property or method 'from', see #664

youknowriad · 2017-05-26T09:03:00Z

I'm thinking we should merge this. And try (or not) to fix the browser issue separately. The fact that this issue breaks the disposableFocus is just a consequence and the HoC is not doing anything wrong.

aduth · 2017-05-26T13:52:48Z

@youknowriad What do we want to do about these review points?

youknowriad · 2017-05-26T14:14:48Z

@aduth Oh sorry, I forgot about the documentation comment.

About the findDomNode I'm thinking we should leave it for now, since it's not deprecated or anything yet but keeping the possibility to add the wrapper back with a config if this changes. What do you think?

…eFocus HoC

aduth · 2017-05-26T14:25:22Z

Misbehaving hashbang behavior I think 😄 Was referring to the folder placement remark, not findDOMNode.

youknowriad · 2017-05-26T14:30:49Z

@aduth 😅 I need a good long weekend :P

For the placement, I don't know, I'm open to suggestions. I put it in "components" to make it reusable across other WordPress modules (outside Editor/Blocks), maybe we can create a subfolder components/hoc

aduth · 2017-05-26T15:23:22Z

Yeah, I don't really have a great answer. It's not a true component, but I don't necessarily want another top-level directory for these functions. I think they're related to components and in that way could belong in the components directory, but I find the hoc acronym to be a fair bit cryptic. It would be nice to have something short for plugin consumers using the global wp.components.hoc. I'm wondering if components/higher-order/ (wp.components.higherOrder) would be an "okay-ish" compromise.

youknowriad · 2017-05-26T15:43:11Z

Let's move it to the higher-order subfolder for now and we'll see how it feels with other HoCs :)

Also, thinking we should add aindex.js to the components and the higher-order folder to allow import { Button, IconButton } from 'components' and import { disposableFocus } from 'components/higher-order'. But let's leave this to a separate PR

aduth · 2017-05-26T15:44:14Z

Also, thinking we should add aindex.js to the components and the higher-order folder to allow import { Button, IconButton } from 'components' and import { disposableFocus } from 'components/higher-order'`. But let's leave this to a separate PR

Yep, this is needed too for exposing components on the global. I was wondering if there might be a way to automate it, so we can avoid this kind of scenario.

aduth · 2017-05-26T15:56:19Z

Not sure about the naming 🙂 maybe disposableFocus is not so clear, but I wouldn't know what other naming to use. Minor point anyways.

Maybe something about focus being remembered or returned? focusMemory, withFocusReturn ?

mtias · 2017-05-29T19:32:13Z

components/higher-order/with-focus-return/index.js

@@ -0,0 +1,41 @@
+/**


What about utilities instead of higher-order? Alternatively, we can just keep them in the root. cc @aduth @youknowriad

I don't have a preference here, happy to update to what we'll settle on.

I find "utilities" to be a meaningless word.

nylen · 2017-05-30T04:04:44Z

Why is this component named withFocusReturn instead of WithFocusReturn? I would think that everything under components should be named in upper-case.

youknowriad · 2017-05-30T08:44:14Z

@nylen to make the distinction that it's not a component, it's a component factory. It can't be used like this <WithReturnFocus />

Accessibility: Add disposable focus Higher Order Component

f07a316

youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 25, 2017

youknowriad self-assigned this May 25, 2017

youknowriad requested review from aduth, afercia and jasmussen May 25, 2017 09:20

Accessibility: Drop the div wrapper in the disposableFocus HoC

74f0dc8

aduth reviewed May 25, 2017

View reviewed changes

Accessibility: Adding JSDoc explaining the behaviour of the disposabl…

e5a1272

…eFocus HoC

Accessibility: Move the disposableFocus HoC to a new higher-order folder

2186b44

aduth approved these changes May 26, 2017

View reviewed changes

Accessibility: Rename disposableFocus as withFocusReturn

a3a9086

youknowriad merged commit 4ce764f into master May 29, 2017

youknowriad deleted the add/disposable-focus-component branch May 29, 2017 08:39

mtias reviewed May 29, 2017

View reviewed changes

nylen mentioned this pull request May 30, 2017

De-duplicate Webpack build, especially components/ #929

Merged

Accessibility: Add disposable focus Higher Order Component #901

Accessibility: Add disposable focus Higher Order Component #901

Uh oh!

Conversation

youknowriad commented May 25, 2017

Uh oh!

jasmussen commented May 25, 2017

Uh oh!

afercia commented May 25, 2017

Uh oh!

youknowriad commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented May 25, 2017

Uh oh!

afercia commented May 25, 2017

Uh oh!

youknowriad commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowriad commented May 25, 2017

Uh oh!

aduth May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad May 25, 2017

Choose a reason for hiding this comment

Uh oh!

youknowriad commented May 25, 2017

Uh oh!

aduth May 25, 2017

Choose a reason for hiding this comment

Uh oh!

aduth May 25, 2017

Choose a reason for hiding this comment

Uh oh!

afercia commented May 25, 2017

Uh oh!

afercia commented May 25, 2017

Uh oh!

youknowriad commented May 26, 2017

Uh oh!

aduth commented May 26, 2017

Uh oh!

youknowriad commented May 26, 2017

Uh oh!

aduth commented May 26, 2017

Uh oh!

youknowriad commented May 26, 2017

Uh oh!

aduth commented May 26, 2017

Uh oh!

youknowriad commented May 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth commented May 26, 2017

Uh oh!

aduth commented May 26, 2017

Uh oh!

mtias May 29, 2017

Choose a reason for hiding this comment

Uh oh!

youknowriad May 30, 2017

Choose a reason for hiding this comment

Uh oh!

aduth May 31, 2017

Choose a reason for hiding this comment

Uh oh!

nylen commented May 30, 2017

Uh oh!

youknowriad commented May 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

youknowriad commented May 25, 2017 •

edited

Loading

youknowriad commented May 25, 2017 •

edited

Loading

aduth May 25, 2017 •

edited

Loading

youknowriad commented May 26, 2017 •

edited

Loading