Sidenav toggle button triggers on spacebar press

[jmuzina]

Done

Changes all uses of a.p-side-navigation__toggle and a.p-side-navigation__toggle--in-drawer to <button> elements and removed href attribute

Fixes #4559, WD-11071

QA

• Open a sidenav
  • Accordion / Dark
  • Accordion
  • Side navigation dark
  • Side navigation default
  • Side navigation docs
  • Expandable dark
  • Expandable
  • Icons
  • Paper
  • Raw HTML
  • Sticky
  • Vanilla Docs
• Small / Medium breakpoints:
  • Verify that the side-navigation toggle button visually appears as it does in the current production release (button styling)
  • Verify that you can open/close the side-navigation using all of the following methods:
    • Click / tap
    • Enter key
    • Space key (new functionality)
  • Verify that the open/close side-navigation button is read to you by a screen reader such as ChromeVox.
• All breakpoints:
  • Verify that the side-navigation content/structure is read to you by a screen reader such as ChromeVox.

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

• PR should have one of the following labels to automatically categorise it in release notes:
  • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
• Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
  • if CSS class names are not changed it can be bugfix relesase (x.x.X)
  • if CSS class names are changed/added/removed it should be minor version (x.X.0)
  • see the wiki for more details
• Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

[webteam-app]

Demo

Jenkins

demos.haus

[bartaz]

I had a suspicion there was a reason why we used a link instead of the button there:

vanilla-framework/scss/_patterns_side-navigation.scss

Lines 120 to 121 in e6d414d

	.p-side-navigation:target &,
	[class*='p-side-navigation--']:target &,

So it turns out originally we did have a button to toggle side navigation, but it was changed to links so that it can be opened just via CSS (:target) in #3065

So it seems we have 2 conflicting accessibility concerns here: do we want to make it accessible without JS, or do we want to make it accessible via required keys for button, which requires JS.

[bartaz]

So, how I see our options:

A) using links (leave as is) - it's accessible without JS (assuming people build it correctly with link opening it having the id of the drawer in href), but it's not fully accessible via keyboard (as it doesn't support space key).

B) using buttons (this PR) - it's fully accessible via keyboard natively, but requires JS. For users without JS side navigation will be unavailable on small screens (they will not be able to expand it).

C) using links with additional JS to make space key work - it's accessible without JS (assuming href/id are set up correctly) and via additional JS works with keyboard as well (although not natively).

While the C) solution seems to cover the most cases, it's also most complicated and relies on many assumptions (href/id properly linked, and JS needed to handle keyboard)

I looked around for some reference, but (as usual in accessibility) there is no definitive answer. This article (https://webaim.org/techniques/javascript/#reliance) suggests that most of users have JS enabled, so the JS version needs to be accessible for sure. So it seems we would be doing a better job by using option B) with a button, and making it accessible for most of the cases.

The question remains, what to do with noJS users. And again, it's not that side navigation is completely unavailable without JS - it's still in the document, visible on large screens. Only on small screens, when it's collapsed, it would not be opened (without JS). So switching to button would solve one issue (keyboard accessibility), and introduce another one (arguabely smaller or less common issue), of expanding side nav without JS.

[bartaz]

@petermakowski Hey, any opinions on this from accessibility standpoint?

[petermakowski]

@petermakowski Hey, any opinions on this from accessibility standpoint?

TLDR; I also think that using a button (and relying on JS) seems like the best choice.

Using a link with added JavaScript to handle spacebar on small screens (most likely a touchscreen device without a keyboard) is definitely not worth additional complexity.

The percentage of users with JavaScript disabled on small screens is likely very low. It's hard to find exact stats, but in various studies it comes at an average of 1% (but that's for all screen sizes). For mobile devices is probably even lower, the vast majority of mobile browsers have JS enabled by default.

[bartaz]

Let's remove the CSS that uses :target selector then, and keep it as a button.

[petermakowski]

Suggested change

	// :target can be used to control the sidenav with an anchor element, but this was removed in PR #5084.
	//.p-side-navigation:target &,
	//[class*='p-side-navigation--']:target &,

[petermakowski]

Suggested change

	// :target can be used to control the sidenav with an anchor element, but this was removed in PR #5084.
	// .p-side-navigation:target .p-side-navigation__drawer,
	// [class*='p-side-navigation--']:target .p-side-navigation__drawer,

[petermakowski]

I'd recommend to remove commented out code, changes and relevant PRs can be tracked using git history.

Suggested change

	// :target can be used to control the sidenav with an anchor element, but this was removed in PR #5084.
	// .p-side-navigation:target &,
	// [class*='p-side-navigation--']:target &,

[bartaz]

I agree. There is no point of keeping commented code. There is a clear reason to remove that.

[petermakowski]

I can confirm this fixes the issue I raised canonical/juju.is#431 (comment)

[jmuzina]

I have a commit ready to remove those comments, just holding the push until @bartaz performs code review on his return (I'd rather not kick off a Percy build from just removing some sass comments) :)

[bartaz]

Looks good, but let's remove the commented out code.

[jmuzina]

Removed comments & will merge after checks pass