Opened 14 years ago

Closed 13 years ago

#5647 closed New Feature (fixed)

Toolbar a11y enhancement

Reported by: Garry Yao Owned by: Frederico Caldeira Knabben
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 (5)

5647.patch (15.5 KB) - added by Garry Yao 14 years ago.
5647_2.patch (16.4 KB) - added by Sa'ar Zac Elias 14 years ago.
5647_3.patch (17.6 KB) - added by Garry Yao 13 years ago.
min_editor_width_for_visible_toolbar.png (48.3 KB) - added by Garry Yao 13 years ago.
5647_4.patch (4.9 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 14 years ago by Garry Yao

Keywords: HasPatch added

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

Changed 14 years ago by Garry Yao

Attachment: 5647.patch added

comment:2 Changed 14 years ago by Sa'ar Zac Elias

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 14 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed
Milestone: CKEditor 3.4CKEditor 3.5
Status: newconfirmed

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 14 years ago by Frederico Caldeira Knabben

Component: UI : ToolbarAccessibility

comment:5 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4.1CKEditor 3.5

comment:6 Changed 14 years ago by Sa'ar Zac Elias

Owner: set to Sa'ar Zac Elias
Status: confirmedassigned

Changed 14 years ago by Sa'ar Zac Elias

Attachment: 5647_2.patch added

comment:7 Changed 14 years ago by Sa'ar Zac Elias

Status: assignedreview

comment:8 Changed 13 years ago by Garry Yao

Status: reviewreview_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 13 years ago by Sa'ar Zac Elias

Keywords: Discussion added

comment:10 Changed 13 years ago by Sa'ar Zac Elias

Milestone: CKEditor 3.5
Owner: Sa'ar Zac Elias deleted
Priority: NormalHigh
Status: review_failednew

comment:11 Changed 13 years ago by Sa'ar Zac Elias

Priority: HighNormal
Status: newconfirmed

comment:12 Changed 13 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed
Milestone: CKEditor 3.5

comment:13 in reply to:  8 ; Changed 13 years ago by Frederico Caldeira Knabben

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 13 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 13 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 13 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

Changed 13 years ago by Garry Yao

Attachment: 5647_3.patch added

comment:17 Changed 13 years ago by Garry Yao

Keywords: Discussion removed
Owner: changed from Tobiasz Cudnik to Garry Yao
Status: assignedreview

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 13 years ago by Tobiasz Cudnik

Status: reviewreview_passed

Looks like everything is working fine here.

comment:19 Changed 13 years ago by Garry Yao

Committed to toolbargroup branch for a11y testing.

comment:20 Changed 13 years ago by Frederico Caldeira Knabben

Keywords: HasPatch removed
Milestone: CKEditor 3.5CKEditor 3.6
Owner: Garry Yao deleted
Status: review_passednew

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 13 years ago by Garry Yao

Owner: set to Garry Yao
Status: newreview

comment:22 Changed 13 years ago by Frederico Caldeira Knabben

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 13 years ago by Frederico Caldeira Knabben

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 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

comment:25 in reply to:  22 Changed 13 years ago by Garry Yao

Status: review_failedreview

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 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedassigned

comment:28 Changed 13 years ago by Frederico Caldeira Knabben

Status: assignedreview

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

comment:29 Changed 13 years ago by Frederico Caldeira Knabben

comment:30 Changed 13 years ago by Frederico Caldeira Knabben

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 13 years ago by Garry Yao

Status: reviewreview_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 13 years ago by Garry Yao

comment:32 Changed 13 years ago by Frederico Caldeira Knabben

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 13 years ago by Garry Yao

Attachment: 5647_4.patch added

comment:33 in reply to:  32 Changed 13 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 13 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added

comment:35 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

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 13 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 13 years ago by Frederico Caldeira Knabben

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 13 years ago by Garry Yao

Status: reviewreview_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 13 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Branch merged with [6748].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy