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

Nav sidebar: tab navigation not working on Firefox #43972

Open
p-jackson opened this issue Jul 8, 2020 · 21 comments
Open

Nav sidebar: tab navigation not working on Firefox #43972

p-jackson opened this issue Jul 8, 2020 · 21 comments
Labels

Comments

@p-jackson
Copy link
Member

When you open the nav sidebar in the editor, the sidebar should behave like a modal. So clicking the dark background or pressing ESC should close the sidebar, and also importantly, pressing tab should only focus elements within the sidebar.

This is working on Chrome, but isn't working in Firefox.

My guess is that the call to .focus() to initially set the focus inside the sidebar isn't working on FF.

Part of #41298

@p-jackson
Copy link
Member Author

p-jackson commented Jul 13, 2020

The issue is that the only controls inside the sidebar (except for the close button) are <a>s, which aren't tabble in Firefox without special config. See this SO for details. tl;dr

  1. In System Preferences → Keyboard, in the Shortcuts pane, check the “all controls” radio at the bottom.
  2. In Firefox, type "about:config" in the URL bar. There is no accessibility.tabfocus preference on the mac, so you'll have to make one. Right click in the window, create a new "integer" pref, and set it to 7.

Step 1 being required seems fine, but step 2 is a surprise given it's not how Safari/Chrome/Edge work.

This causes a bug in Gutenberg's withConstrainedTabbing HoC because it assumes that links will always be tabble. The first and last controls in the sidebar need to be tabble for the logic of the HoC to work. I added a dummy <button> to the bottom of the sidebar to test this out and the tabbing was correctly constrained to the sidebar.

@autumnfjeld autumnfjeld self-assigned this Aug 24, 2020
@autumnfjeld
Copy link
Contributor

autumnfjeld commented Aug 25, 2020

@andrewserong First, just want to clarify that I think this issue is about the tabbing, and not about the “clicking the dark background or pressing ESC should close the sidebar” as I verified that closing the sidebar in this way works properly in Chrome, Safari, and Firefox on my Mac.

As @p-jackson mentioned in the comment above, the issue has to do with user preferences on a Mac. I did more digging on this.

I verified that the tab problem exists in both Safari and Firefox on my Mac, and according to the SO link that Phillip posted, the issue is system and/or browser preferences for keyboard navigation in both Safari and Firefox on a Mac. Mac users must change their accessibility settings in order to take advantage of tab navigation.

All the following is for user on a Mac:

  • Firefox: This MDN doc says that accessibility.tabfocus controls tab navigation, but if this is not set in Firefox (which it is not by default in Firefox), then the Mac setting in System Preferences > Keyboard > Shortcuts > Use keyboard navigation to move is honored
  • Safari: In my tests Safari does not respect the System Preferences setting. Instead this feature must be activated within Safari: Preferences > Advanced > Accessibility

My question now is: what problem are we trying to solve?

  1. Do we want to alter the markup to try to get around these Mac settings? To force tab navigation even though the user hasn't enabled these preference settings? My first gut feeling is this seems wrong, as we'd have to use non-semantic markup--and also, overriding user preferences seems kinda sketchy (even though here it is not for sketchy purposes).
  2. Or do we want to educate the user that they must turn these settings on to get the best WordPress.com experience?

@ramonjd
Copy link
Member

ramonjd commented Aug 25, 2020

To force tab navigation even though the user hasn't enabled these preference settings?

I agree we should consider this. But given it's the "default" setting in Firefox, I wonder how many users would know (and care)?

Being able to tab through (what are ostensibly) navigable elements, I'd have thought, is a fairly basic feature of web browsing. Is there a good reason to have these disabled in the first place?

My first gut feeling is this seems wrong, as we'd have to use non-semantic markup

This might be "out there", but could we try forcing <Button /> to be a button element by assigning a click handler and getting rid of the href?

<Button /> used here in the sidebar:
https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/wpcom-block-editor-nav-sidebar/src/components/nav-item/index.tsx#L44

Where <Button /> determines whether to use an <a /> or <button /> HTML element: https://github.com/WordPress/gutenberg/blob/master/packages/components/src/button/index.js#L67

Maybe there's a another way?

@autumnfjeld
Copy link
Contributor

autumnfjeld commented Aug 25, 2020

Ah, yes @ramonjd , <Button/> is a fine solution!! (And um ya I should have thought of that! 🤦‍♀️ )

I tested that on both Firefox & Safari (just using any ole website that has buttons & links to see what is tabable) and all good! Buttons & Inputs get hit by tabbing, <a /> links do not.

I will give that a go!!!! Thanks.

@andrewserong
Copy link
Member

@autumnfjeld I think this one is quite a subtle issue. One part is the slightly odd default behaviour in Safari and Firefox on Mac of not tabbing to links, and I think we probably want to respect the default behaviour where it makes sense. That said, personally I think you should be able to at least tab to the back button in the sidebar.

But the issue that Phil describes (if I'm reading it correctly) is that in Firefox, if you start tabbing in the sidebar and keep on pressing the tab key, it'll actually end up tabbing through some of the elements in the right hand sidebar. But the tabbing should be constrained to just the sidebar without escaping that area.

Here's a gif where you can see that eventually the buttons at the top of the UI are tabbed to, but because the sidebar should act like a modal, this shouldn't be allowed:

tabbing-in-the-sidebar-small

@andrewserong
Copy link
Member

andrewserong commented Aug 25, 2020

But yes, +1 to @ramonjd's idea of using a click handler to force the <Button/> component to render a button element!

@autumnfjeld
Copy link
Contributor

Ah, sorry guys! I forgot one bit of info that I think speaks to your point @andrewserong

When you turn the accessibility settings on for Firefox, the the tabbing works fine in Firefox, at least in my own tests. Thus I assume if we changed the <a /> to <Button /> we'll get the desired & correct functionality.

Screen Capture on 2020-08-25 at 10-44-00

@andrewserong
Copy link
Member

andrewserong commented Aug 25, 2020

Oh, @autumnfjeld one thing I forgot about when working on the nav sidebar locally, is that the feature is guarded behind both a filter and a config setting check (setting either will enable the feature) here: https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/full-site-editing-plugin.php#L307

The way I've switched this feature on for local development is to set the WPCOM_BLOCK_EDITOR_SIDEBAR setting in wp-env by adding a .wp-env.override.json file to /apps/full-site-editing/.wp-env.override.json (this directory will be moved soon to /apps/editing-toolkit`).

The contents of my .wp-env.override.json file is:

{
	"config": {
		"WPCOM_BLOCK_EDITOR_SIDEBAR": true
	}
}

Then, running wp-env start again, the sidebar is switched on for me. (More info on this override JSON file at: https://github.com/WordPress/gutenberg/tree/master/packages/env#wp-envoverridejson)

@ramonjd
Copy link
Member

ramonjd commented Aug 27, 2020

at the moment I'm still leaning toward educating the user on how to turn on tab navigation in Safari & Firefox, rather than changing our markup to get around a setting the user simply needs to set.

Replying to your comment from #45240 (comment)

I think I'm starting to agree.

The "Mac way" of enabling tab key navigation for links affects every site on the web, not just our sidebar.

This answer phrases it well: https://stackoverflow.com/a/49781088

I don't think that there is problem that you need to fix here.

Apple handles accessibility a little differently, and there are user preferences that must be enabled to allow for tabbing between links. This behavior is well known in the accessibility community, and I would not recommend implementing any custom solutions to modify this behavior, as it will likely only create other problems.

This article explains in more detail: http://www.weba11y.com/blog/2014/07/07/keyboard-navigation-in-mac-browsers/

cc @enriquesanchez who might have some ideas on how and whether we should tackle this

@enriquesanchez
Copy link
Contributor

Hi everyone!

Folks who rely on keyboard navigation and/or assistive technology usually set their browsers and OS to work the way they need/prefer. We shouldn't change the sidebar's behavior or semantics to force what we think should be the right keyboard focus functionality.

I also think we should avoid educating the user about this. The complexity needed to reliably detect the browser and OS would be too much, and then we wouldn't be able to know with certainty what the user's particular needs are.

As for semantics, we should avoid changing the behavior of an a element to be that of a button. These elements already carry the semantics needed by assistive technology to be interpreted and operated as such, and changing that will cause problems. Let links be links and buttons be buttons.

Related to constraining tabbing to the sidebar, we should definitively do this. Losing focus can be very disorienting. But there should also be an obvious way to close it and get back to the element that toggled it.

Last time I checked the sidebar had the role="dialog", I recommend taking a look at the WAI-ARIA Authoring Practices for this component. There's some really good info and guidelines on how to handle keyboard focus:

Like non-modal dialogs, modal dialogs contain their tab sequence. That is, Tab and Shift + Tab do not move focus outside the dialog. However, unlike most non-modal dialogs, modal dialogs do not provide means for moving keyboard focus outside the dialog window without closing the dialog.

Please feel free to reach out if I can help with anything else.

@p-jackson
Copy link
Member Author

I agree that we're better leaving the semantics as they are i.e. using <a> for the links instead of <button>. Users will have other features like "announce the links that are available" and we don't want to break that feature in order to fix this. I also think not trying to educate the user is also the correct, they will have already set things up according to their preferences.

IMO the issue is that the withConstrainedTabbing HoC has a bug. It should work regardless of what the system/browser settings are (i.e. if that means it skips over all the links in the dialog then that's fine). The bug hasn't been exposed before because the other places it's used just so happen not to include links. We could open an issue in the GB repo but I can imagine it'd be pretty low priority given there's no user facing bug in GB.

I recommend taking a look at the WAI-ARIA Authoring Practices for this component.

Interestingly the approach taken in this example is totally different from the one taken by withConstrainedTabbing. It uses a couple of hidden focusable divs to mark the beginning and end of the dialog, instead of making assumptions about which elements are tabbable (which is what GB does).

@autumnfjeld
Copy link
Contributor

Interestingly the approach taken in this example is totally different from the one taken by withConstrainedTabbing. It uses a couple of hidden focusable divs to mark the beginning and end of the dialog,

@p-jackson Could you clarify what example you are referring to?
Are you talking about a code example in the WAI-ARIA doc?

@p-jackson
Copy link
Member Author

@autumnfjeld This example here: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html
You can click the "Add Delivery Address" button to see an example modal.

The HTML source is on that same page, and the JS source is here.

Took me a while to decipher how it works, but it doesn't look like it makes assumptions about what sort of elements are focusable (unlike withConstrainedTabbing). Instead if focus ever leaves the dialog then it tries to focus the first (or last) control in the dialog by iterating through every element until it finds an element that it succeeds on.

@ramonjd
Copy link
Member

ramonjd commented Sep 7, 2020

IMO the issue is that the withConstrainedTabbing HoC has a bug. It should work regardless of what the system/browser settings are (i.e. if that means it skips over all the links in the dialog then that's fine). The bug hasn't been exposed before because the other places it's used just so happen not to include links. We could open an issue in the GB repo but I can imagine it'd be pretty low priority given there's no user facing bug in GB.

What are your thoughts here?

Can we close this for now, or could we report our findings about withConstrainedTabbing over at Gutenberg?

@p-jackson
Copy link
Member Author

I don't think we should close. It still feels like a serious bug imo. We should report in Gutenberg, but it'd be a bug in the component library, not with anything the user can reproduce in vanilla GB so don't think it'll get much attention.

@autumnfjeld autumnfjeld removed their assignment Sep 10, 2020
@nerdysandy
Copy link
Contributor

@p-jackson Hi 👋 Do you think it is still relevant with the Nav unification being rolled out soon? This is how the new workflow would be https://d.pr/i/vitaeA

@p-jackson
Copy link
Member Author

p-jackson commented Feb 11, 2021

@nerdysandy Does that mean that the in-editor sidebar is being removed as part of nav unification? My understanding was that the current in-editor sidebar would be replaced by a new one as part of full-site editing, as seen here pbAok1-1oJ-p2#comment-3154

@nerdysandy nerdysandy added the Triaged To be used when issues have been triaged. label Feb 14, 2021
@nerdysandy
Copy link
Contributor

Ah, I didn't know about that bit. In that case, we can leave it open :)

@github-actions
Copy link

This issue is stale because it has been 180 days with no activity. You can keep the issue open by adding a comment. If you do, please provide additional context and explain why you’d like it to remain open. You can also close the issue yourself — if you do, please add a brief explanation and apply one of relevant issue close labels.

@p-jackson
Copy link
Member Author

Most of the a11y issues initially identified with the nav sidebar were fixed, and this was the last remaining one. It only happens on Mac FF. It's an issue with Gutenberg modals in general and a fix would come with tradeoffs e.g. one option is to use <button> instead of <a>, which would worsen a11y in favour of improving a11y in this way.

I think the proper solution to this is to replace our sidebar component with the one that's built into the site editor. That way we're tying our a11y fortunes to core. I originally implemented this wpcom specific nav sidebar and always assumed that eventually the component would be replaced by the GB component (which didn't exist at the time).
Switching to the GB sidebar component in the wpcom post/page editor isn't on any team's roadmap as far as I can tell though @simison.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants