Ticket #5647 (closed New Feature: fixed)

Opened 4 years ago

Last modified 3 years ago

Toolbar a11y enhancement

Reported by: garry.yao Owned by: fredck
Priority: Normal Milestone: CKEditor 3.6
Component: Accessibility Version:
Keywords: Cc: satya_minnekanti@…

Description

The following toolbar accessibility improvement are suggested by IBM a11y experts:

  1. Navigating the toolbar should allow skipping button groups. When in the toolbar TAB can be used to skip from one button group to the next, when at the end, TAB wraps back to the beginning. ARROWS can be used to cycle between buttons within a group, wrapping focus within the same group. This feature would enhance accessible usability of the editor.
  2. The sub-groups in the toolbar should have the ARIA role of "group", it is possible to nest "groups" if required. This will enable a JAWS user to see that they are navigating through related functions. The Kama skin clearly shows group delineation in the UI and this should be reflected to JAWS users. At the moment, groups in the toolbar are marked as role="presentation".
  3. Toolbar groups as mentioned in (2) should have labels provided. This requires a new way to define groups in the configuration and allow labels.

Attachments

5647.patch (15.5 KB) - added by garry.yao 4 years ago.
5647_2.patch (16.4 KB) - added by Saare 4 years ago.
5647_3.patch (17.6 KB) - added by garry.yao 3 years ago.
min_editor_width_for_visible_toolbar.png (48.3 KB) - added by garry.yao 3 years ago.
5647_4.patch (4.9 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 4 years ago by garry.yao

  • Keywords HasPatch added

Use config.toolbar = 'Full_Group' to enable the toolbar definition with groups.

Changed 4 years ago by garry.yao

comment:2 Changed 4 years ago by Saare

How about specifing the labels in the configuration instead of the language file? That would bring more flexibility to create groups without using hacks like 'CKEDITOR.lang.en.toolbarGroup.myGroup'.

comment:3 Changed 4 years ago by fredck

  • Keywords Discussion removed
  • Status changed from new to confirmed
  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

We may even propose an easy way to define group labels in the configuration, but by default all textual resources must come from the language file, to support localization.

comment:4 Changed 4 years ago by fredck

  • Component changed from UI : Toolbar to Accessibility

comment:5 Changed 4 years ago by fredck

  • Milestone changed from CKEditor 3.4.1 to CKEditor 3.5

comment:6 Changed 4 years ago by Saare

  • Owner set to Saare
  • Status changed from confirmed to assigned

Changed 4 years ago by Saare

comment:7 Changed 4 years ago by Saare

  • Status changed from assigned to review

comment:8 follow-up: ↓ 13 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

The patch is outdated, besides, I don't quite agree with this which might become a usability issue for arrow keys:

ARROWS can be used to cycle between buttons within a group, wrapping focus within the same group

comment:9 Changed 3 years ago by Saare

  • Keywords Discussion added

comment:10 Changed 3 years ago by Saare

  • Status changed from review_failed to new
  • Priority changed from Normal to High
  • Owner Saare deleted
  • Milestone CKEditor 3.5 deleted

comment:11 Changed 3 years ago by Saare

  • Priority changed from High to Normal
  • Status changed from new to confirmed

comment:12 Changed 3 years ago by fredck

  • Keywords Discussion removed
  • Milestone set to CKEditor 3.5

comment:13 in reply to: ↑ 8 ; follow-up: ↓ 15 Changed 3 years ago by fredck

Replying to garry.yao:

The patch is outdated, besides, I don't quite agree with this which might become a usability issue for arrow keys:

ARROWS can be used to cycle between buttons within a group, wrapping focus within the same group

This is an ARIA recommendation best practice. The thing is that we'll be now grouping the buttons, each one forming a so called "widget". The best practices says that the TAB key must be used to navigate between widgets. Then, other keys, like arrows, can be used within the widget for navigation. So here we have the buttons cycling.

I understand your point that this has a usability impact for the non visually impaired users. My position on this is that we must give priority to visually impaired when working on keyboard navigation. Other users have the advantage of the mouse alternative.

It's also true that still many people don't care (even if unaware) about accessibility, so they may blame on us for the solutions we'll propose. Because of this, I was thinking about making this option configurable, if it'll not bloat our code that much. This would also help us maturing this new feature on successive releases.

comment:14 Changed 3 years ago by garry.yao

  • Keywords Discussion added

I don't think we should provide the default group configuration into core, as well as the language entries, it should be all handled by the user, following our documentation.

comment:15 in reply to: ↑ 13 Changed 3 years ago by garry.yao

Replying to fredck:

This is an ARIA recommendation best practice. The thing is that we'll be now grouping the buttons, each one forming a so called "widget". The best practices says that the TAB key must be used to navigate between widgets. Then, other keys, like arrows, can be used within the widget for navigation. So here we have the buttons cycling.

I agree with you in the sense of following standards, while one reality that contribute to the negative side is because of #5669, arrow keys is of less use.

comment:16 Changed 3 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

Changed 3 years ago by garry.yao

comment:17 Changed 3 years ago by garry.yao

  • Owner changed from tobiasz.cudnik to garry.yao
  • Keywords Discussion removed
  • Status changed from assigned to review

The following decision were made for the discussion:

  1. Having grouped toolbar by default (replacing the old toolbar_Full);
  2. Introducing "toolbarGroupCycling" to provide an alternative keyboard navigation solution to ignore groups.

comment:18 Changed 3 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

Looks like everything is working fine here.

comment:19 Changed 3 years ago by garry.yao

Committed to toolbargroup branch for a11y testing.

comment:20 Changed 3 years ago by fredck

  • Status changed from review_passed to new
  • Keywords HasPatch removed
  • Owner garry.yao deleted
  • Milestone changed from CKEditor 3.5 to CKEditor 3.6

Delaying the public release to the 3.6. In this way we'll have time to test and have feedback regarding this feature into the svn branch directly.

comment:21 Changed 3 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to review

comment:22 follow-up: ↓ 25 Changed 3 years ago by fredck

The way the toolbar has been implemented broke the office2003 and v2 skins. There is no support anymore for the '-' separator. Additionally, the toolbar "start" and "end" are not anymore considered.

I understand that the kama grouping has been taken in consideration, but much probably it's the wrong way for us to come with this feature. In fact, there are many groups with 1 to 3 buttons only on them, which makes the tab navigation less productive.

We could come with a new way of rendering kama now, adding the '-' separator feature for it. In this way, we can have the same kind of grouping we have now on the editor, where things like ['Source','-','Save','NewPage','Preview','-','Templates'] are considered a single group. By doing so, the new navigation will work for the office2003 and v2 skins as well, eventually even helping kama to be less cluttered.

comment:23 Changed 3 years ago by fredck

There is a navigation problem with "ungrouped" items. They can be navigated with arrows, without cycling, which makes the navigation confusing.

One of the ideas is forcing all toolbox items to be inside toolbars, even if it'll represent a toolbar with a single item.

Following this feature, the 'Styles', 'Format', 'Font' and 'FontSize' items can also be grouped into a single toolbar.

comment:24 Changed 3 years ago by fredck

  • Status changed from review to review_failed

comment:25 in reply to: ↑ 22 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

Replying to fredck:

The way the toolbar has been implemented broke the office2003 and v2 skins. There is no support anymore for the '-' separator. Additionally, the toolbar "start" and "end" are not anymore considered.

The "separator" and "start/end mark" are used (in old configuration) to logically mark a toolbar group and a toolbar correspondingly, while we'd avoid having them presented in the new toolbar config format, as the array structure already defines the group, what is missing now is just to present the (logic) group visually in v2 and office skin. For "start/end mark" I vote for change it, as the old assignation like ['Source','-','Save','NewPage','Preview','-','Templates'] makes less sense as those commands are not really logically grouped, in new configuration they should be just displayed once a row to indicate the toolbar, so we just have 3 level of grouping:

  1. Toolbox - The container
  2. Toolbar - A visual row
  3. Toolgroup(optional) - logical grouped items

I understand that the kama grouping has been taken in consideration, but much probably it's the wrong way for us to come with this feature. In fact, there are many groups with 1 to 3 buttons only on them, which makes the tab navigation less productive. We could come with a new way of rendering kama now, adding the '-' separator feature for it. In this way, we can have the same kind of grouping we have now on the editor, where things like ['Source','-','Save','NewPage','Preview','-','Templates'] are considered a single group. By doing so, the new navigation will work for the office2003 and v2 skins as well, eventually even helping kama to be less cluttered.

The group (as an ARIA role) should present the functionality semantic (for accessibility) but not the keyboard productivity (for usability) I guess.

I've made [6690] addressing some of the above mentioned issues.

comment:26 Changed 3 years ago by fredck

  • Status changed from review to review_failed
  • I see that the v2 and office2003 layout has been changed, which is not required, nor desirable.
  • As for the last comment, this ticket is more about *usability* than about accessibility, which makes my last comments much pertinent. The basic idea is making accessibility more usable. This is our goal.

There are other minor issues to be fixed. I'm taking over the ticket so we can come to a conclusion to it.

comment:27 Changed 3 years ago by fredck

  • Status changed from review_failed to assigned
  • Owner changed from garry.yao to fredck

comment:28 Changed 3 years ago by fredck

  • Status changed from assigned to review

I've totally reworked the featured into a new branch:
http://dev.ckeditor.com/browser/CKEditor/branches/features/toolbargroup2

comment:29 Changed 3 years ago by fredck

comment:30 Changed 3 years ago by fredck

Ops! I didn't check the last CSS changes on IE, so it's a bit broken there. Please ignore the separator issues there for now, checking all the rest meanwhile. I'll come with a fix for it later on a dedicated commit that can be checked separately.

comment:31 Changed 3 years ago by garry.yao

  • Status changed from review to review_failed

Um...I didn't expected the new toolbar makes kama looks better ordered, so looks like a good move! The codes for new toolbar change are basically good as well.

While it introduce a noteworthy problem to liquid layout, making the toolbar less visible in small screen - toolbar items are not wrapping well, I've attached a comparison of the minimum possible editor width (constraint by toolbar) on kama skin for reference, this degrade would be quite harmful for (large) mobile screens.

Changed 3 years ago by garry.yao

comment:32 follow-up: ↓ 33 Changed 3 years ago by fredck

It'll fit an iPhone at least... anyway, it's not worth to break the style combos into several toolbars just because of this, as we're not targeting this change to mobiles. If one will be interested on it, it'll be enough to redefine the toolbar (which would be in any case reasonable, to not have such tall, unusable editor).

Changed 3 years ago by garry.yao

comment:33 in reply to: ↑ 32 Changed 3 years ago by garry.yao

Replying to fredck:

it's not worth to break the style combos into several toolbars just because of this

It's not a necessarily, the proposing patch fix this issue as well as the separators on IEs, RTL is not tested yet.

comment:34 Changed 3 years ago by satya

  • Cc satya_minnekanti@… added

comment:35 follow-up: ↓ 36 Changed 3 years ago by fredck

  • Status changed from review_failed to review

I don't think it makes sense have both of us working on the same thing.

Anyway, the branch has been updated. Things should work well on all browsers.

There is just one small remaining issue with IE+Quirks/6 where the toolbars with separators are making the toolbars 2px higher than others. I would not like to block the entire review process just because of this, so I'm asking to please proceed with it. This issue will be addressed with a dedicated commit later.

comment:36 in reply to: ↑ 35 Changed 3 years ago by garry.yao

Replying to fredck:

There is just one small remaining issue with IE+Quirks/6 ...

This affects IE7 also, and because of it, toolbar wrapping is a bit broken, I'm not sure we could leave it. There's also FF RTL is broken now by separator.

The above two are updated with [6737].

comment:37 Changed 3 years ago by fredck

It's great to see your fix for that issue Garry. Congrats!

There was a small issue with FF introduced with it though. I've fixed it with [6741].

I would ask to please proceed with the full review now.

comment:38 Changed 3 years ago by garry.yao

  • Status changed from review to review_passed

Oops, thanks for catching it, I find another tiny glitch on IE7 and committed the fix, other than that R+ for the branch.

comment:39 Changed 3 years ago by fredck

  • Status changed from review_passed to closed
  • Resolution set to fixed

Branch merged with [6748].

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy