Review toolbar example

[mcking65]

Update as of Dec 3, 2018

The toolbar example published in APG 1.1 Release 2 developed for issue #126 has been redesigned to address the issues listed in the below redesign checklist.

Preview new toolbar in feature branch

The redesigned toolbar in feature branch issue541-toolbar-redesign is ready for review and will be merged to master after issues from this review are resolved.

Toolbar Redesign Checklist

• Issue Toolbar Example: aria-selected should not be used. #540: Toolbar Example: aria-selected should not be used
• Issue Toolbar Example: Use button elements and add other types of elements, such as link and checkbox #541: Toolbar Example: Use button elements and add other types of elements, such as link and checkbox
• Issue Toolbar Example: Bugs with keyboard support, aria-disabled, and aria-label #847: Bugs with keyboard support, aria-disabled, and aria-label
• It's hard to tell the difference between checked and not-checked toggles and radios (Noted by @carmacleod in PR Toolbar example for simple text editor #887).
• Add a title/tooltip to the Bold, Italics, Underline, Align left/center/right, Font type, and size increment/decrement tools (for consistency with Cut/Copy/Paste, and in case any sighted folks need help understanding the icons) (Noted by @carmacleod in PR Toolbar example for simple text editor #887).
• Implement menuitemradio elements as radio elements per issue Need to allow for radiogroup inside toolbar #949.

[jkva]

This is my first review within the APG; please let me know I'm not following correct review protocol.

The toolbar items within the toolbar are div elements with a role of button - why are these not button elements with a type of button?

[mcking65]

Job, thank you for the review. This is exactly the kind of feedback we would like.

@jkva commented:

The toolbar items within the toolbar are div elements with a role of button - why are these not button elements with a type of button?

Probably because I didn't request it to be done that way when @tatermelon did the original coding work. However, given our overall guidance to prefer native HTML where possible while illustrating important features of the pattern, this is a good suggestion.
I have updated issue #541 to capture this enhancement.

[jnurthen]

Adding me to review - I have comments on this.

[annabbott]

Accessibility Features

1. When tabbing into the toolbar, focus returns to the control that last had focus. Focus movement inside the toolbar is managed using roving tabindex.

I believe that sentence refers to a second pass into the toolbar.

Suggested edit:
"When tabbing into the toolbar for a second time, "

[annabbott]

Accessibility Features
2. Since there is only a single disabled control, to ensure screen reader users are aware of its presence, it is focusable. For additional guidance, see Focusability of disabled controls.

There seems to be five disabled controls (Tool 1 through Tool 5), not a single disabled control. However there isn't any visual affordance that indicates these are disabled - they just don't do anything.

[annabbott]

No other issues noted.

[spectranaut]

Hi @tatermelon -- I'm writing tests for this example right now, and I have two questions about the documentation:

1. The attributes table lists aria-disabled -- but I couldn't discover a way to disable one of the toolbar element (by which I mean, it seems as though aria-disabled is never set on any element of the toolbar). Is there a way to disable elements? Should this row just not be included?
2. The attributes table makes me think the aria-label element belongs on div with role="toolbar" -- but it is instead on the role="separator" div, is this intentional?

[spectranaut]

Hi @tatermelon -- while writing tests for the other attributes and keys, I also noticed these two inconsistencies with the table documentation:

1. The "right arrow" and "left arrow" keys do not cycle though the tools -- a "right arrow" to the last element in the toolbar does not move the focus to the first element in the toolbar, for example.
2. the "home" and "end" keys do not move the focus in the toolbox

[tatermelon]

Hey @spectranaut, @mcking65 is the one who wrote the documentation for the examples, so he will know better which is correct. @mcking65 should the example be updated to match the attribute table or should the attribute table be updated to match the way the example is working?

[SuzanneTaylor]

When focus is on any of the first five buttons, if you press space-bar to activate the button, the default action of the space-bar still happens, and the page scrolls. The buttons have no implemented action and so testing and development didn't explore this far, but in terms of having an example we can send people to that covers all common mistakes, it would good to implement some simple function for the buttons and then illustrate .preventDefault()

[mcking65]

Thank you @SuzanneTaylor, I agree. I've added this to issue #847.

[mcking65]

A redesigned toolbar is ready for review in the issue541-toolbar-redesign feature branch.

[mcking65]

In today's APG task force meeting, Michiel posted some useful images during the discussion about making the pressed state of buttons more visually distinct from the not pressed state.

jn: border is fine as long as there is a 3:1 ratio
... border isn't dark enough - is only 2:1
MichielBijl:
https://usercontent.irccloud-cdn.com/file/SMBtoImM/Screenshot%202018-12-03%20at%2020.00.15.png
https://usercontent.irccloud-cdn.com/file/0iW53xB7/Screenshot%202018-12-03%20at%2020.01.55.png
image 1: Current situation
image 2: Preferred situation where the buttons look like one whole group thing

cc: @jongund

[carmacleod]

@mcking65 @jongund I like @MichielBijl's suggestion of making the radio buttons look like "one whole group thing" - this is great to show that they are a radio group and not just a bunch of unrelated toggle buttons.

However, it is still hard to glance at the toolbar and know which tools are selected/checked/on. The unselected white #FFFFFF background is just way too close to the selected light-light-gray #F4F4F4 background. I realize these 2 colors don't actually touch because there's a border between them. But I look at that radiogroup and I see 3 whitish-greyish radio button tools, and I have to stare at it to be sure which one is selected. Maybe it's my monitor, or maybe it's my eyes. ;)

[jongund]

Do you really want bold, italic and underline buttons to have a title attribute or should the title attribute be used for the accessible name instead of aria-label?

[jongund]

I have made most of the updates required in this issue, except for the title attributes on all the controls. I have added two regression tests, there is some flaw in the roving tabindex test code that has left both Evan and I perplexed. Still trying to figure out what the problem is, but we have two passing tests right now.

[jongund]

I have moved my work on this issue to the W3C repository branch:
https://github.com/w3c/aria-practices/tree/issue541-toolbar-redesign

You can view the latest version of the toolbar at:
https://raw.githack.com/w3c/aria-practices/issue541-toolbar-redesign/examples/toolbar/toolbar.html

[devarshipant]

1. Is aria-controls implemented correctly? In the example, aria-controls IDREF (menu1) points to the ul. However, the Usage reads "Provides a reference to the textarea that the toolbar controls."

2. Should the toolbar support Control B / I / U on text highlighted within the textarea?

[jnurthen]

@devarshipant

1. The toolbar references the textarea with aria-controls correctly. The menu button also has aria-controls pointing to its menu as specified in the menu button pattern.
2. No - this is a demonstration of toolbar. It is not a fully formed text editor.

[devarshipant]

On Mon, Dec 17, 2018 at 5:23 PM James Nurthen ***@***.***> wrote: @devarshipant <https://github.com/devarshipant> 1. The toolbar references the textarea with aria-controls correctly. The menu button also has aria-controls pointing to its menu as specified in the menu button pattern. Got it, thanks. I thought aria-controls was only being used on the menu

button and textarea was the controlled element. On a different note, I am not sure if JAWS will work properly on the menu button with aria-controls as it prompts the user (w/ shortcut keys) to the unopened menu. And that could be a vendor issue.

…

1. No - this is a demonstration of toolbar. It is not a fully formed text editor. ok. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHCQkLbG2z7IJSNESsXVKKWqs2pRgPZLks5u6BlWgaJpZM4QqqsX> .

[jongund]

@mcking65 I updated the copyright information in the JS files

[mcking65]

The redesigned toolbar example is a very significan and great piece of work by @jongund. I think it will be an especially useful reference as it demonstrates how to resolve many integration issues in an effective manner. It appears that it will also help surface quite a few assistive technology support bugs.

I’ve now finished my final editorial updates to it so it is, I hope, consistent with our style guide and all the applicable design patterns as well as with other examples. Now it is time for the whole task force to really hammer on it and see if we missed anything important:

• Are there any bugs? We need testing with latest versions of:
  • Firefox on Windows
  • Chrome on both Win and Mac
  • Safari on Mac (iOS is also informative but not essential before release)
  • IE11.
• Are there any accessibility problems?
• Is the documentation accurate and clear?

This is a rather complex example, so I have assigned five people to do formal reviews in PR #950. Please try to complete your review by Thursday Jan 3 so we have enough time to get a fix in by Jan 8 in the event you find a critical problem. Of course, we welcome reviews from anyone. The more hands and brains on it the better.

To make sure we have the browsers covered, it could be helpful if you share with the group what browsers you test with and on which platform.

Example page for review and test:
https://raw.githack.com/w3c/aria-practices/issue541-toolbar-redesign/examples/toolbar/toolbar.html

Please provide feedback in pull request #950

[mcking65]

Moving development of regression tests to a separate issue -- #965.

[mcking65]

@annabbott, @sh0ji, @shirsha, @charmarkk,

Does the newly merged toolbar example address this item in the to-do list for this issue?

It's hard to tell the difference between checked and not-checked toggles and radios (Noted by @carmacleod in PR #887).

[charmarkk]

@mcking65 I think so, it's rather evident to me.

[sh0ji]

Yep, it looks good to me too. I do have a couple points of feedback, though:

• I expect clicking the padding of the toolbar (the gray #ECECEA part) to move focus into the toolbar. I often use my mouse to orient my focus, and then use my keyboard to move around. This isn't strictly a requirement, but I can't be the only one who does this.
• I'd prefer it if all the controls had a descriptive title attribute so that sighted users don't have to infer or discover the function of the controls.

[mcking65]

All feedback has been addressed or is logged in a new issue. Thank you everyone for helping make the toolbar example so good!!