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

Menu remains open after actions which moves focus #15313

Closed
karlgroves opened this issue Apr 30, 2019 · 4 comments · Fixed by #15543
Closed

Menu remains open after actions which moves focus #15313

karlgroves opened this issue Apr 30, 2019 · 4 comments · Fixed by #15543
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Comments

@karlgroves
Copy link

Menu remains open after actions which moves focus

  • Severity: Medium
  • Affected Populations:
    • Low-Vision
    • Motor Impaired
    • Cognitively Impaired
  • Platform(s):
    • All / Universal
  • Components affected:
    • Block Options

Issue description

When the Unified Toolbar is being used, the Options menu remains open
after triggering an action that causes the keyboard focus to be moved to
the edit area. This results in a situation where the focus position
might be underneath the menu. The actions of duplicating a block,
inserting new content before or after a block, or removing a block, all
have this behaviour.

Obscuring the focus position is potentially confusing for sighted
keyboard users, since it may appear as though no item has focus, or make
it unclear as to where the focus position is. Additionally it is simply
difficult to view the content and moving keyboard focus to and from the
toolbar is cumbersome.

Remediation Guidance

Whenever an action in the Options menu causes the focus to move, the
menu should close automatically.

Relevant standards

Note: This issue may be a duplicate with other existing accessibility-related bugs in this project. This issue comes from the Gutenberg accessibility audit, performed by Tenon and funded by WP Campus. This issue is GUT-101 in Tenon's report

@gziolo gziolo added the Needs Accessibility Feedback Need input from accessibility label Apr 30, 2019
@melchoyce
Copy link
Contributor

Screenshot from the full report:

image

@afercia
Copy link
Contributor

afercia commented May 5, 2019

Noting this is also inconsistent depending on the state of the "Top Toolbar" option, not sure what the argumentation behind this different behavior is:

  • disabled: toolbars appear on the blocks: activating one of those command in the More options menu moves the focus and closes the menu
  • enabled: toolbar unified at the top: the same actions move the focus but keep the menu open

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed Needs Accessibility Feedback Need input from accessibility labels May 5, 2019
@karmatosed karmatosed added Needs Design Feedback Needs general design feedback. and removed Needs Design Feedback Needs general design feedback. labels May 6, 2019
@mapk
Copy link
Contributor

mapk commented May 7, 2019

The solution suggested in the issue sounds like the right way to proceed.

Whenever an action in the Options menu causes the focus to move, the menu should close automatically.

@aduth
Copy link
Member

aduth commented May 14, 2019

This is technically a duplicate of #14754.

That being said, I think the titling and context included in the comments here prove more useful toward a solution. I'll close the other in favor of this.

Including some specific comment text from #14754 in the debugging I had performed there:

At #14754 (comment) :

I've reproduced the issue. I assume it's likely related to how we forcibly re-insert a default block when all other blocks are removed. This re-insertion likely triggers focus to be moved to the block and out of the menu.

if ( count === 0 ) {
yield insertDefaultBlock();
}

componentDidMount() {
if ( this.props.isSelected ) {
this.focusTabbable();
}
}

I assume that if this analysis is correct, the bug will have been present for a while. That said, the insertDefaultBlock behavior was recently refactored as part of #14189 (cc @jorgefilipecosta ).

I'd suggest action items:

  • Find a way to avoid focus changes as part of the "ensure default block" behavior, or reimplement the behavior such that backspacing in the first empty block will simply behave as a noop (rather than having a new block be reinstated after the fact)
  • Make sure that dropdown menus become closed when focus leaves

At #14754 (comment) :

Part of the problem is also that selecting the menu items does not close the menu. I'm unsure whether it was done so intentionally, but my reading of the recommendations is that the menu should be closed.

I wonder also if it's something we can improve in the abstractions and relationships between Dropdown and MenuItem components. There's also DropdownMenu which... is neither used by the block settings menu, nor itself uses the MenuItem component. At least in how it is implemented, it may serve as a suitable replacement for the block settings menu's use of the more generic Dropdown component, with the auto-close behavior built in.

gziolo pushed a commit that referenced this issue May 23, 2019
* Close block settings menu after removing block

* Undo change to BlockToolbar component

* Also close settings menu after other items

See #15313

* Do not show BlockSettingsMenu if block is invalid

* Use flow()

* Block Editor: Select previous / next only when exists

* Remove now unneeded isValid check
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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants