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

Sidebar header: avoid focus loss and other improvements #8079

Closed
afercia opened this Issue Jul 20, 2018 · 12 comments

Comments

Projects
None yet
6 participants
@afercia
Contributor

afercia commented Jul 20, 2018

When using a keyboard and activating one of the sidebar header buttons to switch sidebar, there's a focus loss:

screen shot 2018-07-20 at 14 15 31

The focus loss is more evident in some browsers (IE11) but it happens in all browsers and it's particularly annoying when using a screen reader. The only difference is that modern browsers are a bit smarter and they try to keep focus "in place" but it's very clear the button is not focused any longer after it gets pressed (the outline disappears).

Seems to me this has to do with the changes introduced in d4749ff

Previously (Gutenberg 2.6.0), the sidebar header was outside of the panels. Instead, now it's placed within the panels so it gets re-rendered every time users switch to the other sidebar. Compared with the previous behavior, this is actually an accessibility regression.

@afercia

This comment has been minimized.

Contributor

afercia commented Jul 20, 2018

Forgot the part related to "and other improvements" 🙂

  • there's no semantic indication of which the active button is
  • since the sidebar region is names "Editor Settings", it would be nice to rename the close button from Close settings to Close Editor Settings

@gziolo gziolo added this to the 4.1 milestone Oct 10, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

There is no corresponding PR, moving to 4.2.

@brandonpayton brandonpayton self-assigned this Oct 20, 2018

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Oct 20, 2018

Hi @gziolo, do you know if there is any reason we can't pull <SettingsHeader/> out of BlockSidebar and DocumentSidebar and into the editor layout so it persists as the user switches between tabs? I believe @afercia is right about the reason the tab buttons lose focus.

My naive approach to indicating the active button would be to:

  1. Mark the selected tab button with aria-selected
  2. Add role="tablist" to the buttons' container.
  3. Use aria-controls tabs and panels.

@afercia, do you have any thoughts on the general approach? I've seen posts about how aria-controls isn't well supported but haven't tested it yet.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 20, 2018

Yes, we should consolidate those two sidebars into one Fill to avoid complete DOM update which causes focus lose. I can take care of this on Monday. I didn’t take that into account when refactored code to work almost like plugin sidebars like Yoast. I want to look into the whole implementation of Plugin sidebar and apply some changes in a way which would allow us to to have any number of tabs in the sidebar. I think that might be necessary when we work on phase 2. @youknowriad any thoughts on this?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 20, 2018

Can't we just wrap the sidebars with withFocusReturn?

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 21, 2018

@brandonpayton hi and thanks for looking into this. I've considered to change these buttons in an ARIA tabbed interface. I'm not fully convinced it would really improve things though, as a tabbed interface adds some complexity to the keyboard interaction. I'd tend to think a tabbed interface adds value when there are many tabs. These are just two buttons and I'd rather keep things simple. Just my personal opinion 🙂

You're right there's no indication of which one of the twos is currently selected. What I'd probably do would be:

  • wrap the buttons in a <ul> which makes screen readers automatically announce how many list items are in the list
  • use is-active to add a proper information to the aria-labels on the buttons, for example when the buttons are active their aria label should change to:
    • aria-label="Document settings (selected)"
    • aria-label="Block settings (selected)"
      Any thoughts and feedback welcome.

Re: aria-controls here's an interesting reading 🙂 http://www.heydonworks.com/article/aria-controls-is-poop

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Oct 23, 2018

I can take care of this on Monday. I didn’t take that into account when refactored code to work almost like plugin sidebars like Yoast. ...

Thanks, @gziolo! Please let me know if there's anything I can help with. I'll take a look at fixing the active button indication.

Can't we just wrap the sidebars with withFocusReturn?

@youknowriad, I may misunderstand, but withFocusReturn doesn't seem to fit this case because focus doesn't need to return anywhere. In this case, we have a button that appears as a single button to the user but is actually part of two separate UI trees under the covers. When Document Settings are active, clicking the Block Settings button should result in that button being focused, but because DocumentSettings and BlockSettings both render their own instance of the button, the clicked-and-focused button is unmounted and replaced with an unfocused button as part of the BlockSettings UI. (👈 That's pretty wordy. I hope it makes sense.)

Thank you, @afercia. Simple is good. :) I'll work on your suggestion. Thanks for the link re: aria-controls. I'd skimmed it a bit before asking but need to read it in full.

@brandonpayton brandonpayton referenced this issue Oct 23, 2018

Merged

Improve a11y of settings sidebar tabs #10927

4 of 4 tasks complete
@gziolo

This comment has been minimized.

Member

gziolo commented Oct 23, 2018

@aduth started #10917 with failing e2e test which detects focus loss when active tab changes. I will try to find some time today to better understand what kind of fix is required.

@aduth

This comment has been minimized.

Member

aduth commented Oct 26, 2018

This should have been three issues if it prescribes three tasks. In implementing the focus loss solution in #10917, it's not clear whether it can "close" this one without tracking down several other pull requests in the process.

For the future maintainer, this is my understanding of the status:

  • Focus loss (Fixed by #10917)
  • Active tab indication (Fixed by #10927)
  • Close button label "Close settings" -> "Close editor settings"
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 27, 2018

Close button label "Close settings" -> "Close editor settings"

I think I disagree with this change because we're not tweaking the editor settings in this tab but the content settings. We may want to change the region name? Anyway I'm moving out of 4.2 for now.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 27, 2018

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 27, 2018

Yep whatever the button label is, ideally it should match the related navigable region name. The main reason why the navigable regions have a label prepended with "Editor" is to distinguish them from other existing landmarks in the page:

landmarks labels

I'd agree the wording can be improved, for example "top bar" is not appropriate. Not so simple though, as some of these regions have mixed content (e.g. Document settings and Block settings) or content that can change depending on settings (e.g. the Unified toolbar). There used to be a specific issue to keep track of labels improvements, see #2387 which is now closed. If there are no objections, I'd like to reopen #2387 and start listing the labels to improve there.

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 31, 2018

If no objections, I'd say this can be closed now that #10917 and #10927 solved the main issues. Labels can be improved later. Thanks for all the feedback and work done here. Can always be reopened if I'm missing something.

@afercia afercia closed this Oct 31, 2018

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