patternfly-elements icon indicating copy to clipboard operation
patternfly-elements copied to clipboard

fix(switch): implement internals controller

Open zeroedin opened this issue 1 year ago • 4 comments

What I did

  1. Implementation of InternalsController

Testing Instructions

Notes to Reviewers

zeroedin avatar Mar 05 '24 17:03 zeroedin

🦋 Changeset detected

Latest commit: 12a308fea6ef68887045317a73b3c7d3eb38f3bb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@patternfly/elements Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar Mar 05 '24 17:03 changeset-bot[bot]

Deploy Preview for patternfly-elements ready!

Name Link
Latest commit 0bfeb3b2909cb2621a23263f4901b998755451d2
Deploy Preview https://deploy-preview-2702--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Mar 05 '24 17:03 netlify[bot]

So, here's the current a11y state of the switch at patternflyelements.org (PFE) and in the deploy preview (DP). It's kind of a pick-your-poison with them.

TLDR:

  • Both work great with keyboard, mouse, and touchscreen for non-users of assistive tech (though PFE needs to quell the page scroll upon spacebar press).
  • DP version works just as expected with Mac and iOS Safari/VoiceOver. PFE version has label issue (reads both the on and off versions of the label).
  • Both operate pretty well with Android Chome/Talkback, but the edge goes to PFE for announcing toggle state changes.
  • Both have issues with Windows browser and screen reader combos. Neither NVDA nor JAWS recognize the switches as form elements. The PFE version is slightly better with NVDA once coerced into working, but both still have issues.

patternflyelements.org

Without screen reader

  • Mouse: works as expected in all browsers.
  • Keyboard: pressing space activates switch (good) and scrolls page (bad).

Mac Safari/VoiceOver

  • Good: Keyboard operable, announces checkbox role and state.
  • Issue: Always announces both labels: "Message when on. Message when off."

Win Chrome/JAWS

  • Issue: Neither labeled nor unlabeled versions work by default. JAWS does not enter forms mode upon the switch receiving focus. There are ways to force JAWS into forms mode (by using the next/previous checkbox hotkeys). However, even in those cases, the switch toggles visually but does not announce on/off state.

Win Firefox/NVDA

  • Good: Keyboard operable, announces checkbox role and state when receiving focus.
  • Issue: Only announces state change when switched on (checked), and not when turned off (unchecked).

iOS Safari/VoiceOver

  • Issue: Same issue as Mac safari with the dual labels, but otherwise works well.

Android Chrome/Talkback

  • Good: Works as expected. Touch operable and announces checkbox role, state, and label.
  • Issue: Announces the SVG in the switch (which should be hidden from assistive tech) as an unlabeled image.

Current deploy preview

Without screen reader

  • Mouse: works as expected in all browsers.
  • Keyboard: works as expected in all browsers.

Mac Safari/VoiceOver

  • Good: Works as expected. Keyboard operable and announces switch role, state, and label.

Win Chrome/JAWS

  • Issue: Neither labeled nor unlabeled versions work by default. JAWS does not enter forms mode upon the switch receiving focus. There are ways to force JAWS into forms mode (by using the next/previous checkbox hotkeys). However, even in those cases, the switch toggles visually but does not announce on/off state.

Win Firefox/NVDA

  • Good: No label version is keyboard operable, announces role, and announces when switched on and off (except when it's on by default and you switch it off. In this case, it does not announce the very first switch off, but does subsequent ones.)
  • Issue: The labeled version doesn't work by default. Placing focus on the switch causes NVDA to announce the "section" role. Switch can then only be triggered after manually setting NVDA to forms mode. So NVDA is not recognizing this element as an interactive form element.

iOS Safari/VoiceOver

  • Good: Works as expected.

Android Chrome/Talkback

  • Good: Touch operable and announces checkbox role, state, and label upon focus.
  • Issue: Does not announce state change on toggle, though it does occur visually.

hellogreg avatar Mar 22 '24 18:03 hellogreg

Also wanted to mention that the DP version is better than PFE in Mac/iOS Safari because of some updates @zeroedin made yesterday. If needed (as in, if we're not yet ready to go with this new version of switch), maybe those same fixes could be made to the live PFE version, too--fixing the label issue, the page scroll issue, and hiding the SVG from assistive tech.

hellogreg avatar Mar 22 '24 19:03 hellogreg

@hellogreg did https://github.com/patternfly/patternfly-elements/pull/2702/commits/5dbdb14da9b3c10c46dabb5206c81b9e54738950 help?

bennypowers avatar Mar 24 '24 08:03 bennypowers

Looks like it could be getting a little closer, but still has issues.

Still works fine in Safari/VO.

Chrome/Jaws:

  • The labeled switch can be activated via keyboard after initially receiving focus. However, the screen reader doesn't announce state change. It just announces the key that was pressed ("space" or "enter").
  • The unlabeled switch does not work on the demo page. Activating it either does nothing or occasionally toggles the labeled switch. Update: Just realized that part of this issue may be a current Chrome bug with screen readers.

Firefox/NVDA

  • The unlabeled switch works as expected, toggling and announcing state change.
  • Pressing space or enter on the labeled switch always toggles the unlabeled switch. The inverse problem of the issue that sometimes occurs with Chrome/JAWS!

hellogreg avatar Mar 25 '24 21:03 hellogreg

This should get a major changeset because the label API changed, no? unless i'm mistaken and we already did that

You are correct we changed the label API by adding the <span> instead of multiple labels. This should get a major.

zeroedin avatar Mar 27 '24 15:03 zeroedin

@nikkimk or @hellogreg if we could get a final sign off please

bennypowers avatar Mar 27 '24 19:03 bennypowers