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

Toolbar Example: Bugs with keyboard support, aria-disabled, and aria-label #847

Closed
7 tasks done
mcking65 opened this issue Aug 20, 2018 · 4 comments
Closed
7 tasks done
Assignees
Labels
bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern Feedback Issue raised by or for collecting input from people outside APG task force regression-testing Related to AVA regression tests of example pages or AVA framework implementation within repo

Comments

@mcking65
Copy link
Contributor

mcking65 commented Aug 20, 2018

As noted in issue #539, the toolbar example has the following bugs:

  • Missing aria-label="Example Toolbar" on div with role="toolbar"`
  • Missing aria-disabled="true" and appropriate greyed out styling on one of the button elements (add to second button element in toolbar). As documented in accessibility features section, the disabled button should remain focusable.
  • Home key does not move focus to first element.
  • End key does not move focus to last element.
  • Right Arrow key does not wrap focus from last to first element.
  • Left Arrow key does not wrap focus from first to last element.
  • Elements in toolbar do not implement their respective patterns, e.g., enter and space do not activate buttons. Each element should execute an action, e.g., buttons and links open alert dialog. This let's users know the element was activated and prevents default action, e.g., scrolling with space.
@mcking65 mcking65 added bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern Feedback Issue raised by or for collecting input from people outside APG task force regression-testing Related to AVA regression tests of example pages or AVA framework implementation within repo labels Aug 20, 2018
@mcking65 mcking65 added this to the 1.1 APG Release 3 milestone Aug 20, 2018
@mcking65 mcking65 self-assigned this Aug 26, 2018
@mcking65
Copy link
Contributor Author

@tatermelon is planning to submit a PR for this issue.

@jongund
Copy link
Contributor

jongund commented Aug 27, 2018

I am planning on submitting a pull request on an updated toolbar example later this week. See the following link:
https://rawgit.com/jongund/aria-practices/toolbar/examples/toolbar/toolbar.html

Mostly need to clean up some CSS best practices and focus styling issues with my student.

@jongund jongund closed this as completed Aug 27, 2018
@mcking65
Copy link
Contributor Author

@jongund, the WIP you have looks like a more realistic toolbar. I didn't know you were working on that. With a few changes, it could resolve issue #541. That is something we should discuss. In fact, that would be a good way to frame the PR.

For project management reasons, I don't want this issue closed until we have the resolution committed. So, I am re-opening it.

The contract with Bocoup referenced the current toolbar; this one is significantly more complex. The tests for the current toolbar are merged into the Bocoup branch, The tests that fail due to the bugs in the current example are commented out. This example would drive rework on that set of tests, which @spectranaut may not be able to contain.

Timing will also be a factor. I see quite a few issues that need resolution and possibly discussion in this new example. So, I am not completely confident it can be done before the Bocoup contract ends. The current target completion date for test development is sep 24.

If you are confident you will have the resources to complete this toolbar along with necessary test revisions (if needed) before December as well as the other stuff we already have in the release 3 plan, I'm OK with adopting issue #541 as the resolution plan for this issue. I'll take it off @tatermelon's plate for now, and we can discuss the plan in the september 10 meeting.

Some challenges I see in the current implementation of the new toolbar are ...

It has toggle buttons that do not follow the toggle button pattern; once pressed, they cannot be unpressed. This is because they are toggles pretending to be radios. I am not sure how to best resolve that. This is a fairly common type of design, and one that is not easily resolved without tweaking existing ARIA patterns.

I do not think we want toggles that cannot be toggled. Yes, you could add aria-disabled="true" to the button after pressed, but I feel like that is a potentially confusing paradigm. The user will here "Aligned left button pressed disabled". will the typical user really understand that pressed means that the text is aligned left? Or, will they wonder if it is not aligned because they hear "disabled"? Will they wonder how to enable the option? The more ways of expressing the exact same concept, the radio concept, the more confused users can get. It is better to minimize the number of ways of expressing the same semantic whenever possible.

The only real barrier to using the existing radio group pattern in a toolbar is that the checked state cannot follow focus like in normal radio groups because the arrow keys are needed to navigate to other elements in the toolbar. That means requiring the user to press space to check the radio. Maybe that is OK. I'd recommend implementing that approach and bringing it to the group for review.

I noticed that the toolbar label is "toolbar" ... It should probably be "Text format". Remember, never use the role name in the label.

The buttons and button groups are not labeled.

The font menu button says it is a font menu before a font is chosen, but does not say that it is the font menu after a font is chosen; it does tell the name of the current font, but the label should be something like "Font FONT_NAME". Even on page load, the label should probably reflect the current font.

I'm putting this feedback here for now in response to your most recent comment. For follow-on discussion of work on an enhanced toolbar, however, we should do it in a new PR or a different issue. Issue #541 would be more appropriate since that is the issue that calls for enhancements to make the existing toolbar more illustrative and realistic.

@mcking65
Copy link
Contributor Author

mcking65 commented Dec 3, 2018

With commit 599d2de from pr #887, the issue541-toolbar-redesign feature branch now contains all changes needed to resolve this issue. Further review will be managed in issue #539.

@mcking65 mcking65 closed this as completed Dec 3, 2018
@mcking65 mcking65 modified the milestone: 1.1 APG Release 3 Jan 3, 2019
mcking65 pushed a commit that referenced this issue Jan 15, 2019
…#pull 950)

completes work to:

* Resolve keyboard, aria-disabled, and aria-label bugs listed in issue #847.
* Add additional widget types requested in issue #541, including toggle, checkbox, spinner, menu button, radio group, and link.
* The above are all issues discussed in toolbar review issue #539.

Note: missing regression tests are tracked by issue #965.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Code defects; not for inaccurate prose Example Page Related to a page containing an example implementation of a pattern Feedback Issue raised by or for collecting input from people outside APG task force regression-testing Related to AVA regression tests of example pages or AVA framework implementation within repo
Development

No branches or pull requests

2 participants