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

Popover: the new placement in the source order requires to manage focus programmatically #2306

Closed
afercia opened this issue Aug 9, 2017 · 8 comments
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Priority] High Used to indicate top priority items that need quick attention

Comments

@afercia
Copy link
Contributor

afercia commented Aug 9, 2017

The Popover component has been improved and refactored in #2160 and it is now a pretty solid component thanks to @aduth.

However, before the refactoring, it used to be injected "in place", that is: immediately after the UI control that opened it. This order in the source was ideal for keyboard users and screen reader users because the Popover was "natively" in the correct tab order.

Now instead, it is injected at the end of the page, before the closing </body> tag because it uses createPortal with the specific intent to render it outside its parent.

For accessibility, the current implementation is actually a regression. Ideally, the Popover component should be placed in the source immediately after the controls that opens it. This way, it would be in the natural tab order and pressing tab would move focus to the Popover. This worked well before the refactoring.

If the current implementation is going to stay, then focus must be managed programmatically: when the Popover opens, focus must be moved to the Popover. When it closes, focus must be moved back to the control that opened it. Actually, the Popover now works like a modal dialog and thus it needs the typical focus treatment for a modal dialog.

Here's what happens on current master:

  • Inserter: click the inserter, focus is moved programmatically to the search field

  • press Escape to close the inserter

  • focus in not returned any longer to the Inserter toggle button (this used to work at soem point)

  • Post visibility: click the toggle to make the Popover appear

  • using a keyboard, you now have to tab multiple times before reaching the Popover (because it's at the end of the markup)

  • press Escape or Tab away from the Popover: it doesn't closes (this was going to be addressed in Post visibility panel better keyboard interaction #1885 that now needs to be re-worked)

  • EnableTrackingPrompt: click on "More info" and nothing happens

Additionally, the correct usage in the readme file should be updated. We shouldn't encourage to use the Popover inside a button, even if it's later moved outside of its parent.

So the following example in the readme should be changed to illustrate the correct usage, with the button separated from the Popover:

Usage

Render a Popover within the parent to which it should anchor:

import { Popover } from '@wordpress/components';

function ToggleButton( { isVisible, toggleVisible } ) {
	return (
		<button onClick={ toggleVisible }>
			Toggle Popover!
			<Popover
				isOpen={ isVisible }
				onClose={ toggleVisible }
				onClick={ ( event ) => event.stopPropagation() }
			>
				Popover is toggled!
			</Popover>
		</button>
	);
}

Also PostVisibility should be adjusted accordingly.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Aug 9, 2017
@afercia
Copy link
Contributor Author

afercia commented Aug 9, 2017

Note: for the Inserter Menu, withFocusReturn now fails because wrapper is now the <span> element around the Popover and it doesn't contain the activeElement.

@afercia
Copy link
Contributor Author

afercia commented Aug 9, 2017

Note 2: the EnableTrackingPrompt now needs an isOpen prop, as done for the InserterMenu.

screen shot 2017-08-09 at 13 40 18

The Popover text is now very far in the source from the toggle button and basically not rechable for screen reader users. This should be now treated like a modal dialog.

Re: styling: CSS selectors needs to be adjusted since now the "clarification" text is no longer a children of enable-tracking-prompt.

@afercia
Copy link
Contributor Author

afercia commented Aug 9, 2017

Worth noting:

@aduth aduth self-assigned this Aug 9, 2017
@aduth aduth added the [Feature] UI Components Impacts or related to the UI component system label Aug 9, 2017
@afercia
Copy link
Contributor Author

afercia commented Aug 9, 2017

As a source of inspiration, React-modal could be useful: https://github.com/reactjs/react-modal
Not because it's coded using the same patters Gutenberg uses, but because it's a complete solution for accessible modals. I'd especially recommend to have a look at what's inside src/helpers.

@afercia
Copy link
Contributor Author

afercia commented Aug 29, 2017

Note 3: the EnableTrackingPrompt is no longer used.

@afercia
Copy link
Contributor Author

afercia commented Aug 29, 2017

We've discussed this issue during yesterday's accessibility meeting on Slack and agreed the best way to handle the PopOver is adding most of the features a modal should have. Marking with the high priority label to indicate it's an a11y priority.

@afercia afercia added the [Priority] High Used to indicate top priority items that need quick attention label Aug 29, 2017
@jeffpaul
Copy link
Member

This ticket was mentioned in Slack in #core-editor by jeffpaul. View the logs.

@jeffpaul
Copy link
Member

Decision in today's Gutenberg bug scrub to close this as we need to target specific Popovers if they have issues. Related #2323

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Priority] High Used to indicate top priority items that need quick attention
Projects
None yet
Development

No branches or pull requests

3 participants