Problem/Motivation

When configuring the Editor of a text format (such as at the path admin/config/content/formats/full_html), the Toolbar components of the CKEditor can be adjusted through a click, drag-and-drop interface.

text-format-config-with-ckeditor.png

It is not possible to configure the CKEditor toolbar using keyboard navigation.

Turning off JavaScript results in no toolbar configuration.

text-format-config-no-ckeditor-toolbar-config-present-2.png

Proposed resolution

The drag-and-drop click UI should be driven from an HTML form that underlies the more complex UI. The complex UI could be augmented to respond to keyboard input -- this would be accessible. The non-js version form is desirable because it does not leave a site builder completely stranded if their UA cannot produce the complex interaction.

One of the first steps is to wrap the <img> tags that are used to represent CKEditor buttons in <button> tags so they can be accessed via keyboard input.

Screenshot of a text format configuration screen. The Editor option is set to CKEditor. The CKEditor toolbar configuration is shown in a fieldset below. The Chrome Dev tools show that a CKEditor button is represented by a bare img tag.

Once the CKEditor toolbar buttons can be accessed via keyboard input, we'll need to develop a way for users to mark a button as a move candidate, tab to a drop slot and activate a move of the candidate into the slot.

Remaining tasks

Determine a solid implementation through discussion.

User interface changes

The configuration of a toolbar should be achievable through keyboard input alone. Ideally, a non-js fallback will be provided.

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)

CommentFileSizeAuthor
#178 Advanced_and_Basic_HTML___Drupal_D8_Dev.png81.21 KBjessebeach
#175 Windows_7_64__Running_-2.png37.04 KBjessebeach
#175 Basic_HTML___Drupal_D8_Dev.png59.66 KBjessebeach
#175 ckeditor-configuration-a11y-1872206-175.patch115.02 KBjessebeach
#175 interdiff.txt1.75 KBjessebeach
#170 Basic_HTML___Drupal_D8_Dev.png63.7 KBjessebeach
#170 interdiff.txt603 bytesjessebeach
#170 ckeditor-configuration-a11y-1872206-170.patch114.91 KBjessebeach
#164 Windows_7_64__Running_.png25.9 KBjessebeach
#164 interdiff.txt2.21 KBjessebeach
#164 ckeditor-configuration-a11y-1872206-164.patch114.77 KBjessebeach
#162 Screen Shot 2013-10-17 at 11.57.48.png10.32 KBWim Leers
#161 interdiff.txt7.25 KBjessebeach
#161 ckeditor-configuration-a11y-1872206-161.patch115.28 KBjessebeach
#157 Screen Shot 2013-10-15 at 8.54.19 PM.png55.59 KBwebchick
#157 Screen Shot 2013-10-15 at 8.59.40 PM.png58.37 KBwebchick
#155 ckeditor-configuration-a11y-1872206-155-do-not-test.patch114.68 KBjessebeach
#155 interdiff.txt6.57 KBjessebeach
#148 Basic_HTML___Drupal_D8_Dev-2.png45.76 KBjessebeach
#148 ckeditor-configuration-a11y-1872206-148.patch113.1 KBjessebeach
#148 interdiff.txt1.33 KBjessebeach
#143 Screen_Shot_2013-10-15_at_11.56.05_AM.png91.63 KBjessebeach
#142 ckeditor-configuration-a11y-1872206-142-do-not-test.patch112.91 KBjessebeach
#137 voiceover-ckeditor.png220.16 KBwebchick
#137 ckeditor-drag.png122.64 KBwebchick
#137 ckeditor-black-screen-of-death.png131.92 KBwebchick
#131 ckeditor-configuration-a11y-1872206-131.patch114.08 KBjessebeach
#131 interdiff.txt5.02 KBjessebeach
#129 interdiff.txt5.02 KBjessebeach
#129 ckeditor-configuration-a11y-1872206-129.patch5.02 KBjessebeach
#126 interdiff.txt3.34 KBjessebeach
#126 ckeditor-configuration-a11y-1872206-124.patch3.34 KBjessebeach
#125 Button Group name required.png613.05 KBmgifford
#125 Ungrouped Items.png84.92 KBmgifford
#125 Button Group in row 3 of 2.png623.22 KBmgifford
#125 Button Group in Row 2 of 2.png625.67 KBmgifford
#123 ckeditor-configuration-a11y-1872206-123.patch112.82 KBmgifford
#122 ckeditor-float.png183.28 KBquicksketch
#119 ckeditor-configuration-a11y-1872206-119.patch112.81 KBjessebeach
#119 interdiff.txt6.49 KBjessebeach
#118 ckeditor-configuration-a11y-1872206-118.patch113.31 KBWim Leers
#118 interdiff.txt5.72 KBWim Leers
#117 Screen Shot 2013-10-08 at 10.18.45 AM.png52.3 KBmgifford
#116 ckeditor-configuration-a11y-1872206-116.patch112 KBWim Leers
#116 interdiff.txt29.22 KBWim Leers
#113 ckeditor-configuration-a11y-1872206-113.patch108.25 KBjessebeach
#113 interdiff.txt38.48 KBjessebeach
#112 interdiff.txt16.79 KBjessebeach
#112 ckeditor-configuration-a11y-1872206-112-do-not-test.patch92.55 KBjessebeach
#110 ckeditor-configuration-a11y-1872206-110-do-not-test.patch88.02 KBjessebeach
#110 interdiff.txt25.73 KBjessebeach
#109 interdiff.txt1.78 KBjessebeach
#109 ckeditor-configuration-a11y-1872206-109-do-not-test.patch78.55 KBjessebeach
#106 ckeditor-configuration-a11y-1872206-106-do-not-test.patch77.59 KBjessebeach
#106 interdiff.txt17.7 KBjessebeach
#105 interdiff.txt39.82 KBjessebeach
#105 ckeditor-configuration-a11y-1872206-105-do-not-test.patch71.26 KBjessebeach
#104 ckeditor-configuration-a11y-1872206-104-do-not-test.patch47.59 KBjessebeach
#104 interdiff.txt6.56 KBjessebeach
#103 interdiff.txt10.07 KBjessebeach
#103 ckeditor-configuration-a11y-1872206-103-do-not-test.patch45.41 KBjessebeach
#101 ckeditor-configuration-a11y-1872206-101.patch39.62 KBjessebeach
#101 interdiff.txt4.59 KBjessebeach
#97 ckeditor-configuration-a11y-1872206-97-do-not-test.patch39.94 KBjessebeach
#96 ckeditor-configuration-a11y-1872206-96-do-not-test.patch32.92 KBjessebeach
#89 interdiff.txt9.94 KBjessebeach
#89 ckeditor-toolbar-a11y-1872206-89.patch9.94 KBjessebeach
#64 ckeditor_toolbar-a11y-1872206-63.patch9.82 KBquicksketch
#57 ckeditor-a11y-1872206-57.patch4.53 KBjessebeach
#40 1872206_ckeditor-a11y_40.patch25.85 KBjessebeach
#39 1872206_ckeditor-a11y_39.patch25.85 KBjessebeach
#39 interdiff_38-39.txt7.58 KBjessebeach
#38 1872206_ckeditor-a11y_38.patch22.99 KBjessebeach
#31 1872206_ckeditor-a11y_31.patch17.31 KBWim Leers
#26 1872206_ckeditor-a11y_26.patch16.91 KBjessebeach
#26 interdiff.txt9.2 KBjessebeach
#21 ckeditor_a11y.patch15.48 KBquicksketch
#20 ckeditor_a11y.patch15.48 KBquicksketch
ckeditor-toolbar-button-as-an-image.png81.32 KBjessebeach
ckeditor-toolbar-config.png37.68 KBjessebeach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

One could of course argue that since JS is an absolute requirement to be able to use CKEditor in the first place, that a no-js version is a nonsensical request.

I assume you're approaching this from the POV of a visually impaired user who is using a no-js browser, and that user should still be able to set up a WYSIWYG editor for his/her visitors?

quicksketch’s picture

Once the CKEditor toolbar buttons can be accessed via keyboard input, we'll need to develop a way for users to mark a button as a move candidate, tab to a drop slot and activate a move of the candidate into the slot.

I've thought about how to solve this problem, and my suggested approach would be to simply use the arrow keys on the keyboard to move items around. Up/down arrows activate/deactivate any given button (or move them between rows) and the left/right arrows position the button into the correct location. Does that seem like it would be adequate?

One could of course argue that since JS is an absolute requirement to be able to use CKEditor in the first place, that a no-js version is a nonsensical request.

I agree with this statement. We should focus on making the JS-version accessible. There's no point in making a non-JS version at all.

quicksketch’s picture

One followup on my statement here:

There's no point in making a non-JS version at all.

There *is* already a non-JS version (though you have to save the text format first to see it). It's just a plain text area containing JSON configuration of the toolbar. However this textarea is certainly not usable for any normal human and it shouldn't really be considered an alternative at all.

Wim Leers’s picture

Title: Toolbar configuration for a WYSIWYG editor on a text formatter is not keyboard accessible; a non-JS version is not provided » Toolbar configuration for a WYSIWYG editor is not keyboard accessible; touch suport missing; non-JS version not provided

moshe pointed out in #1833716-82: WYSIWYG: Introduce "Text editors" as part of filter format configuration that touch support is also missing.

I don't think it's an absolute requirement to make this work on smartphones; but it should work on tablets.

webchick’s picture

Priority: Normal » Critical

This is going to be a commit-blocker, so raising to critical.

falcon03’s picture

@quicksketch#2: Oh, I would like that things were so easy. If I understand correctly, we're talking about making a drag-n-drop accessible. Technically it should be possible, but practically I think it won't be so easy.

If we decide to work on making this drag-n-drop accessible, though, maybe we should focus on making drupal's drag-n-drop system accessible and then use it for CkEditor. Or am I missing something?

Anyway, does the demo at
quicksketch.org/wysiwyg
allow to test this behavior?

falcon03’s picture

Issue tags: +Accessibility

For example, here:
https://www.ssbbartgroup.com/blog/2011/12/12/accessible-drag-and-drop-wh...
there are two links to two accessible drag-n-drop demo: the problem is that the first one works quite well with Voiceover, but the second one does not.

Anyway, tagging for our accessibility team.

quicksketch’s picture

If I understand correctly, we're talking about making a drag-n-drop accessible. Technically it should be possible, but practically I think it won't be so easy.

I wrote the table drag functionality that has been present since Drupal 6 (e.g. blocks, menu item, and taxonomy term reordering). It is also keyboard accessible via the arrow keys. I don't think this would be any more difficult. We're not actually making jQuery's drag and drop accessible, we're providing an alternative to drag and drop via the arrow keys.

Regarding mobile/touch support for drag and drop, I think jQuery UI will handle that problem eventually, if it doesn't already. As I understand it, adding touch support is (almost) as simple as adding additional bindings for touchstart and touchend instead of mousedown and mouseup.

falcon03’s picture

@quicksketch: Ok, I am very happy to be wrong regarding the difficulty. However, as a blind user, after dragging an element (e.g. a button) I need to receive feedback from my screen reader to know where I am going to drop it. This will be the hardest part, I think.

However, when things wil start to become "code", I'll review the eventual patch and give my impressions! ;)

quicksketch’s picture

Thanks for that feedback @falcon03. I had been thinking that you were referring to accessibility via keyboard manipulation, rather than drag and drop through a screenreader. Do you have any suggestions on how you might expect to handle an administrative situation where you have multiple rows of buttons that need to be put into a specific order?

Using the keyboard arrow keys could probably make it possible for a blind user to move buttons around, but I wonder how we could provide feedback on the current position. Ideally, I'd like to have a screenreader first announce the availability of the action, such as "Bold button. 3rd position, 1st row. Use the arrow keys to reorder this button." And then after the action announce the position such as, "The 'Bold' button is now in the 3rd position, 1st row." Unfortunately I'm not familiar with how to make screen readers announce such a thing.

Moving the button down (and therefor out) of the toolbar would disable the button. Moving a disabled button up would move it into the toolbar and enable it. Perhaps we can describe this simply through description text above toolbar configuration.

Wim Leers’s picture

#10: Jesse Beach has experience with this and she might be of help here. Unfortunately, she's on medical leave for a few weeks right now. What you describe should definitely be possible. I think you're on a Mac, so: 1) enable VoiceOver, 2) use Safari or Chrome to browse a Drupal 8 site with the Edit module enabled, 3) you should be able to tab to enable Edit mode and then tab through the different editables on the page and edit them.
At appropriate times, you will hear announcements such as the one you're describing.

(I'm only posting this to indicate that this is possible; I'm by no means an expert — please always consider falcon03's feedback over mine :).)

falcon03’s picture

@Wim Leers: don't worry, you're right! :D
Just two facilities for Mac OS X users: (1) if you want to enable Voiceover, simply enter "Command + F5" (entering this hotkey again will turn off it) and (2) If you need a better quality voice (Fred is terrible, don't you think so? ;)) you can get one for free by opening the Voiceover Utility (shortcut: Control + Option + F8), then browse to voices from the settings category list and then select "Ad hoc..." from the "default" listbox.

If anyone needs support with a screen reader, feel free to ping me! :D

@quicksketch: well, currently I have no demo of an usable drag-n-drop for blind users. ARIA should have some roles/attribute to help us to deal with it, but from what I've read ARIA is not enough. Let me find some sources and I'll post them here during this week! :D

falcon03’s picture

Well, the purpose of this comment is to bring up to the table some details on how a blind user could expect a drag-n-drop to work. These are my opinions and they haven't been discussed by the whole accessibility team yet (I'll ensure that this will be discussed soon).

The first thing we need to figure out is the best way to organize draggable items so that they can be dragged and dropped easily and in a logical way. I think we have two possibilities:

  1. Organize the draggable buttons in a table, so that each column will represent a menu. We can add table headings to let blind users understand which menu (column) the cursor is placed in and we can use table footer for disabled buttons. This way, it will be super-easy to adjust the toolbar look to suit your needs, since screen readers allow blind users to navigate a table by rows, columns or cells.
  2. Render the toolbar while configuring it, using CkEditor Aria Roles and adding a "Drag-n-drop" feature to buttons. I think this is not the current approach for the editor, and I think this won't be as intuitive to use for blind users and easy to develop as the first possibility.

Once we figure out the best way to organize draggable items, we need to figure out the effective workflow to drag and drop an item using keyboard and a screen reader. We have two possible implementations:

  1. Use screen readers built-in support for mouse drag and drop emulation. I won't recommend this implementation, since IMO the results can be unpredictable and screen readers don't support mouse drag and drop emulation very well.
  2. Use an anchor to render draggable/droppable items, so that clicking an item (pressing "Enter" for Windows or "Vo + spacebar" for Mac OS X screen reader users) can "fire" the drag or the drop action.

This second implementation needs further details, but I need to think a bit more about them. Also, I mentioned wrapping draggable items into anchors, but I don’t know if we can wrap them into something better.

Wim Leers’s picture

Note that at #1833716: WYSIWYG: Introduce "Text editors" as part of filter format configuration, for all rerolls of the patch posted last week, it's already the case that when you're using the page without JS, you get a "Configure" button, which when clicked will cause the relevant configuration form for the currently selected text editor to appear. So one (tiny) aspect has already been solved.

Bojhan’s picture

Just trying to help out here, but the scope of this issue seems a bit large. And I doubt its all soo critical it needs to be fixed pre-commit of CKEditor. For example, touch support, non-JS version are both not mission critical functions.

Wim Leers’s picture

I've discussed this with the CKEditor guys (with Piotrek Koszuliński and Wiktor Walc) and asked for feedback.

As per http://docs.ckeditor.com/#!/guide/dev_toolbar, there are two ways of configuring the toolbar: item-by-item or group-based.

We're currently doing item-by-item. But the problem with our current approach is that there are no "names" for each user-defined group which would be translated into "voice labels" for each group, which is better for a11y. We could add this, but this would make the UI too complicated. Of course, there would still be voice labels for each individual button, so it wouldn't be an a11y disaster.

If we'd switch to a group-based toolbar configuration, then the user would have less control, but also less to worry about. There would be a predefined set of groups (the one that CKEditor ships with; all plugins define default locations for their buttons to be placed in a certain position in one of those groups), users would then be able to click/press each button to either enable or disable it and the button would automatically appear in its predefined group, in a predefined location (with some kind of highlighting indication because otherwise the user wouldn't immediately see where the button had appeared). That's also the downside: it's harder to see (for me at least) how to make this accessible

  • Item-by-item configuration advantages: full control.
  • Item-by-item configuration disadvantages: hard to make a simple UI that allows names/voice labels to be defined for the groups, full control forces users to think about which groups and the order of each button within each group.
  • Group-based configuration advantages: the user doesn't have to think about which groups make sense, more consistency on the web/among Drupal sites, names/voice labels are a non-issue (they're predefined), toggling instead of dragging (better for touch devices)
  • Group-based configuration disadvantages: all buttons of all available plugins are enabled by default (we'd have to use removeButtons to exclude buttons the user doesn't want to use), only reordering of groups is allowed, accessibility of the toolbar configuration UI itself may be problematic.

Note: I may have forgotten a few minor points because I had to rewrite this from scratch due to browser data loss :/

Wim Leers’s picture

Issue tags: +Spark, +CKEditor in core

Tagging; this helps the CKEditor folks track the "CKEditor in core" status on the Drupal side. On the CKEditor side, see http://dev.ckeditor.com/query?keywords=~Drupal.

quicksketch’s picture

Assigned: Unassigned » quicksketch

I'm working on providing a keyboard and screen-reader accessible that I should be able to provide later today.

Wim Leers’s picture

Woot!

quicksketch’s picture

FileSize
15.48 KB

Okay, here's a reroll that makes this interface as accessible as I know how to make it:

  • ARIA roles and attributes used on each toolbar row, button, and add/remove links
  • Each button now can receive focus, both by the keyboard and by clicking on the button.
  • Clicking on the button will announce the button position, as well as reiterate help text for enabling/disabling buttons, via an alert box.
  • Additional help text is displayed giving the basics of usage, and this text is uses as the aria-description on each button. This has the effect of being read aloud by VoiceOver after a slight delay when any button has received focus.

Because the additional JavaScript needed to take into account right-to-left movements (left and right arrows have flipped behaviors in RTL languages), this patch also includes CSS files for RTL support in the admin interface (we already have RTL CSS files for the rest of the module).

I'll work on getting this patch up on my demo site for front-end testing. @jessebeach and @WimLeers, if you guys would give it a code review that would be great.

Remaining issues I don't think should be directly addressed by this module:

  • Non-JS version of the interface: Like Wim said, it's rather non-sensical to require a non-JS version for administering a JS-only feature. Changes to Edit module already made it possible to change the active editor and switch configuration forms without JS. The toolbar configuration for CKEditor itself shouldn't need a non-JS version (though it is still accessible via a very unfriendly textarea with JSON in it).
  • Touch support: jQuery UI itself is missing touch support for its draggable/droppable/sortable libraries. It's been an long-delayed feature. Here's the official issue. It looks like a common solution right now is to install Touch Punch, a hack that adds global support for touch to jQuery UI. If either jQuery UI adds support or Touch Punch is included on the page, this interface will immediately work on touch-devices. I don't think this is a problem for this interface specifically to solve.
quicksketch’s picture

Status: Active » Needs review
FileSize
15.48 KB

I had uploaded a slightly less polished patch. This one adds a few lines of additional documentation and removes a @todo for touch (since like I said above, this should be handled separately from CKEditor). Patch is relative to Drupal root, since this is written against the core patch version.

Wim Leers’s picture

Very nice — it's already working very well :)

- Are you sure that using alert() is a good idea? (Jesse will probably know this.)
- There's no help yet on the minus and plus buttons (add/remove row).

+++ b/core/modules/ckeditor/css/ckeditor.admin.cssundefined
@@ -29,8 +29,8 @@ ul.ckeditor-buttons {
+  float: left; /* RTL */
+  clear: left; /* RTL */

I think this should be LTR? :)

Same elsewhere.

+++ b/core/modules/ckeditor/js/ckeditor.admin.jsundefined
@@ -20,11 +20,29 @@ Drupal.behaviors.ckeditorAdmin = {
+      $toolbarAdmin.on('keyup.ckeditorMoveButton', '.ckeditor-buttons a', adminToolbarMoveButton);
+      $toolbarAdmin.on('keyup.ckeditorMoveSeparator', '.ckeditor-multiple-buttons a', adminToolbarMoveSeparator);
+
+      // Add click for help.
+      $toolbarAdmin.on('click.ckeditorClickButton', '.ckeditor-buttons a', { type: 'button' }, adminToolbarButtonHelp);
+      $toolbarAdmin.on('click.ckeditorClickSeparator', '.ckeditor-multiple-buttons a', { type: 'separator' }, adminToolbarButtonHelp);

AFAIK it is customary to namespace events not on a "per-action" basis, but on a "per-module" basis. I.e. not "ckeditorSOMETHING", but just "ckeditor".

quicksketch’s picture

- Are you sure that using alert() is a good idea? (Jesse will probably know this.)

No I'm not certain, but it works very well with screen readers and it's very clear that it's a popup (the button announces it is a popup on focus and then on click it announces that an alert has opened). I think it's unlikely that users utilizing drag and drop or not using a screen reader will ever know they can click on the buttons directly, since the indication given by the cursor icon (with the "move" icon) is so great. I think we can consider the alert to be seen only by screen reader users, except perhaps on occasion where a user doesn't realize the toolbar is "real" or accidentally clicks on a button. So it's certainly better than doing nothing on click.

- There's no help yet on the minus and plus buttons (add/remove row).

Two reasons: 1) it doesn't need additional help. The primary reason for the help is to announce the row number and position of a button. These buttons never change position and they already have a label announced by screenreaders as to their purpose 2) if you click on them, they need to add/remove a row, not show help. :)

I think this should be LTR? :)

Weird, I could have sworn we used RTL to indicate something had a matching RTL style. But you're right. No idea where I got that impression.

AFAIK it is customary to namespace events not on a "per-action" basis, but on a "per-module" basis. I.e. not "ckeditorSOMETHING", but just "ckeditor".

I'm not sure that custom is something that needs to be enforced, as it'd be perfectly common for a single module to attach two events to the same button if they needed to bind/unbind one (or more) of the events dynamically. In such a case, the module would need to provide more than just the module name. But in this situation, sure, we could easily change this to just "ckeditor", though I personally prefer that the namespace describes the action, as it makes reading the list of bindings on a DOM object clear what each binding does.

So from these, (assuming the alert() is okay), I think the RTL/LTR thing is the only fix that is definitely needed.

Wim Leers’s picture

RE: alert: yeah, it *might* be okay, but it's far more intrusive than how Jesse did the a11y stuff for Edit.

RE: RE: remove/add remove buttons. HAH! Stupid me! :)

RE: event namespacing: it's definitely possible to do it your way. I'll ask nod_ to chime in on that.

And yes, the RTL/LTR thing is the only "definitely fix" thing from my feedback.

nod_’s picture

for custom events, yes it's possible that a module might want to have several namespaces, but events can have several namespaces too. might want to combine one or two to make that more digest. Also we talked about that on this issue #674716: We need guidelines on usage of DOM events and jQuery's namespaced events even if it's more custom-event oriented.

but here the click is clearly redundant, it's the event type :) needs some improving.

jessebeach’s picture

I went through the patch and made a few changes to the placement of ARIA roles and the some HTML. I tried not to change anything else about the HTML, CSS or JS that doesn't concern accessibility.

The changes facilitate the screen reader to interject with information about the button configuration form. Using ChromeVox will illustrate what I mean. My goal was to make sure a screen reader user knows what the form does and what the current state of the form is. I don't think it's 100% there, but I'd like some feedback on it.

The other major change is I removed the alerts and replaced them with an aria-live region so the state of each button is read without arresting the page with an alert or injecting visual elements.

mgifford’s picture

Is this still in sync with #1890502-41: WYSIWYG: Add CKEditor module to core I had trouble applying it against ckeditor_module-1890502-41.patch

Wim Leers’s picture

#27: No, it is not; the patches here at #20, #21 and #26 are against #1890502-21: WYSIWYG: Add CKEditor module to core.

jessebeach’s picture

Title: Toolbar configuration for a WYSIWYG editor is not keyboard accessible; touch suport missing; non-JS version not provided » Toolbar configuration for a WYSIWYG editor is not keyboard accessible; touch suport missing

Removed "; non-JS version not provided" from the title. A non-JS configuration for a JS-only feature doesn't make sense.

Bojhan’s picture

Please provide a demo website, for a11y people to review.

Wim Leers’s picture

Reroll of #26 against #1890502-41: WYSIWYG: Add CKEditor module to core.

UPDATE: I just noticed that this patch, in combination with #1890502-41: WYSIWYG: Add CKEditor module to core introduces visual glitches. But that's not relevant right now; this issue is about the a11y; the visual glitches we can still fix.

falcon03’s picture

Oh, simply wonderful to see progress here!

Sorry I haven't had time to get on this before. Do I need to apply the CkEditor integration patch (I know the library is already in core) before applying this patch?

Anyway, maybe a demo site could help us getting more eyes on this: given the importance of this patch, I could make some blind (non-developers) screen reader users trying this feature to get more feedback...

Wim Leers’s picture

#32: Applying #1890502-41: WYSIWYG: Add CKEditor module to core (i.e. CKEditor.module) and #31 (i.e. the latest patch in this issue that makes the toolbar builder more accessible) should do the trick. And: thanks! :)

mgifford’s picture

The patch seemed to apply fine.

I'd like a working public site to apply both patches too though.

When I tab through the edit page, it still misses the toolbar.

When I try to use Opera Mobile to browse it as a Galaxy 2 smartphone I can select the buttons, but not after I I've selected the text. I can create a link but not hit OK to actually add it.

I think something's wonky, but I'm not 100% sure I've got a working environment to test. I just know I've applied both patches, cleared cache & am editing a full html page with ckeditor's toolbar displayed on it.

mgifford’s picture

didn't mean to remove

falcon03’s picture

I had a look at the patch. Things are getting better, but we haven't reached a good point yet, unfortunately.

I tried using the modified interface with Mac OS X and Voiceover, but not with Jaws or NVDA.

After dragging a button and moving it around with arrow keys the only thing I hear is the label of the button that I am dragging and I don't know where I am going to drop it.

Another problem is that I am not able to understand where a row starts and where it finishes, so I am not able to understand which buttons are in a row and which ones are in another one. Could you implement the suggestion in #13 of grouping buttons in a table?

I confirm mgifford's issue: it's not possible to reach the toolbar configuration object by tabbing (btw, the whole toolbar configuration form seems to be wrapped in a particular object that I have to interact with before altering the toolbar configuration) and I have to say that we should improve screen reader feedback when dropping an item. In fact, a blind user needs to explore the new toolbar to understand that the button has been dropped. We could solve this problem by simply making the screen reader say something "X button has been dropped" (don't know the exact string, though).

Finally, I think that the whole mechanism to add/remove a row of buttons is not accessible: when I press one of these buttons I cannot hear anything to understand that something happened!

BTW: it's a bit unrelated (not too much), but is there any chance that we could apply these drag-n-drop improvements to the blocks drag-n-drop interface to make it accessible too?

quicksketch’s picture

Thanks everyone for the reviews! I had attempted to get a sandbox up and running, but with our 3 different versions of the module and troubles with D8, I couldn't get it all together easily. The next week we're launching a site, so I personally am going to be too busy to give this significant effort until after that is done.

When I tab through the edit page, it still misses the toolbar.

In Mac OS X, in order for elements other than forms to be selectable you must change the "Full Keyboard Access" setting in the Keyboard preference pane to "All controls". This makes it so that the tab key includes links in web browsers instead of just form elements. Very little of the web is keyboard accessible if you can't click on links with the keyboard.

After dragging a button and moving it around with arrow keys the only thing I hear is the label of the button that I am dragging and I don't know where I am going to drop it.

I haven't tried out the latest version of the patch, but assuming it works in a way similar to the previous iterations, clicking on the button will announce its current position and row. It sounds like this might be too ambiguous of an option however. Perhaps we should include the position and row as part of the ARIA label and update this in all buttons as a button is moved around.

As for the suggestion of using table rows, it sounds like this would alleviate the problem too, since tables typically have built-in support for row/column position. Unfortunately from a technical implementation, drag and drop in between tables presents a significant challenge. Existing drag and drop libraries do not work well with moving table cells, and if we moved things within table cells, you'd have issues trying to dynamically add/remove cells as needed. Keeping them as lists (especially since they're already working as such), will be a lot easier.

BTW: it's a bit unrelated (not too much), but is there any chance that we could apply these drag-n-drop improvements to the blocks drag-n-drop interface to make it accessible too?

The changes we're doing here are specific to this interface, but the approaches are generic. This might serve as a good example to reference when building other accessible drag and drop interfaces, but it's unlikely that any of the code will be directly applicable to other interfaces.

jessebeach’s picture

This patch is a reroll of #31 against #1890502-66: WYSIWYG: Add CKEditor module to core.

jessebeach’s picture

falcon03, thank you for the feedback. I hope this update gets closer to the UX behavior that you described in #36. I updated the patch with the following behaviors:

  1. Added tab-indexes to the toolbar buttons in the configuration fieldset. I hope that makes them focusable regardless of keyboard settings.
  2. When tabbing into a toolbar, the state of the toolbar buttons (active or available) and the row number are announced.
  3. When moving a toolbar button, the state of the toolbar the button is moved into (active or available), the row number and the position of the button are announced.
  4. When adding or removing a toolbar, the number of remaining toolbars is announced.

I will set up a testing site in a moment so these changes can be reviewed quickly. Check the following comment for the location and credentials of the testing site.

jessebeach’s picture

Found a small issue while setting up the demo site. I need to attach a new patch.

The demo site is available at the following address with credentials

URL: http://ckeditor.qemistry.us
user: tester
password: tester
page: http://ckeditor.qemistry.us/admin/config/content/formats/full_html

This site will be available for a few days, then I will take it down.

mgifford’s picture

I tried doing this locally with these patches:
ckeditor_module-1890502-66.patch
1872206_ckeditor-a11y_40.patch

And also on the demo site (thanks!), but I couldn't tab to CKEditor let alone within it.

I might be missing something, but at this point I don't see how to access the WYSIWYG without a mouse.

bryancasler’s picture

@mgifford, I was able to use the keyboard arrows to customize the editor....

Toolbar buttons may be moved by drag and drop or with the keyboard arrow keys. Move a button up into the active toolbar to enable it, or down into the available buttons list to disable it. Dividers are available to create logical button groups.

mgifford’s picture

Hi @animelion - thanks for reaching out. I do think that more data is required though. I've tried on a Mac with FF & Safari. In our tests you can't TAB onto the editor toolbar, getting into the text area is fine, but from there it jumps to the next form elements, not into the toolbar. Now if there were an alert that pops up to remind folks that they can use the WYSIWYG that would be useful.

I brought this up in the Accessibility Skype chat and several others were having trouble using it, but we're now much further ahead.

First there are here are keyboard shortcuts e.g. CTRL+b for bold CTRL+i for italic and CTRL+L does the link. The title on the the link icons can also remind folks about the keyboard shortcuts.

ALT+0 provides accessibility help
ALT+F10 to access the editor & navigate to tab-list in the dialogue

I had to dig for this, but on the Mac it's: FN+ALT+F10 (or fn+option+F10). This probably needs to be updated in the help as otherwise it's just going to be annoying for Mac users.

It seems to work, but this needs to be more discoverable. If you've got 3 Drupal accessibility experts who have no idea how to get into the admin menu, then the general content admin will have no idea.

EDIT: Lots of info on this page here (though seems to be 2 years out of date) - http://ckeditor.com/blog/CKEditor-WAI-ARIA-Usable-Accessibility

jessebeach’s picture

Ok, I think I understand now mgifford. You're talking about the usability of the CKEditor wysiwyg editor itself. That's a fair point. My experience with it was the same in that I tabbed into the edit area and then right out. I had to go read the documentation to learn the ALT + F10 trick to get into the toolbars.

But this issue is about configuring a CKEditor toolbar, not using the CKEditor wysiwyg.

This is a demonstration video of me tabbing through the configuration UI: http://screencast.com/t/16fnfbcpH

We want to pass judgement on the configuration experience in this issue, not using the editor. Using the editor is also an issue, but one we should kick over to the CKEditor team.

mgifford’s picture

Status: Needs work » Needs review

There are things that can be done to improve the accessibility of the CKEditor integration and how it is presented to Drupal users, but we can open those up as a new issue. These wouldn't be big things, but more polish.

I think we can say the known accessibility issues are sufficiently addressed for this issue.

Now what needs to be done to make this RTBC?

EDIT: Also, I did do some testing of the editor from my iPhone. Worked pretty well there too.

EDIT2: Actually I was wrong on that. Or maybe I just haven't figured out how to use it with my iPhone.

falcon03’s picture

I would like to test a bit deeper the patch before marking it RTBC.

Firstly, given the importance of the patch, I think we should test it using Windows + Jaws and NVDA. I'll do this ASAP.

Also, I have a tabbing issue in the toolbar configuration form (the wrapper for the toolbar configuration is not focused by tabbing), so IMO the issue needs a bit more work.

I agree with jessebeach: we should avoid talking about two different aspects of the toolbar into the same issue. The problem outlined by mgifford exists, and we must solve it, but in a follow up (IMO).

Wim Leers’s picture

The patch at #40 has now been merged into the main CKEditor module patch at #1890502-78: WYSIWYG: Add CKEditor module to core, as per mgifford's comment at #45.

mgifford, please remove the "needs accessibility review" tag at #1890502: WYSIWYG: Add CKEditor module to core when you deem it ready.

jessebeach’s picture

Getting a "good but not perfect" version of CKEditor into core will make the development, testing and review process much much easier. We will no longer need to build the patch in this queue off the patch in another queue and reroll as the dependency patch changes.

We will continue to improve the UI, visual and aural, after feature freeze. That is not in question. Right now we want to get to a place where we know the limitations and we decide we can live with them for a short time because the resolutions are documented or the problems are at least defined.

Having CKEditor in core will accelerate the integration and bug finding/fixing efforts. We should balance perfection with progress.

mgifford’s picture

My discovery issue is listed here - #1904986: Make Keyboard Controls Discoverable in CKEditor related to my post in #43 above.

falcon03’s picture

Status: Needs review » Needs work

Sorry, but I think I have to change status to "NW".

Using Firefox 18.0.1 and NVDA 2012.3.1 on a Windows XP SP2 machine I cannot drag any button using the arrow keys.

But unfortunately this is not my abitual windows testing environment, so I'll give more details about the problem later.

BTW, I noticed that this patch has been merged into the CKEditor module addition one.
But I wonder if we'd not better keeping this issue separate from the CKEditor module addition one. This way this issue won't block the CKEditor module addition one and, at the same time, will make this patch easier to test/reroll/work on (IMO).

falcon03’s picture

Oh, and I forgot: can we disable the overlay module from the demo site for this issue? Overlay is not so screen reader friendly.

mgifford’s picture

Status: Needs review » Needs work

I wonder if this is a windows problem in general? I haven't tested this in windows yet for keyboard only users.

Does JAWS give you any errors? How does it differ from the experience with VoiceOver?

Someone disabled the overlay which is good.

Wim Leers’s picture

OT: I thought Overlay was pretty good or at least better than dialogs in terms of a11y? You may want to chime in at #1889150: Simplify overlay look, *only use for contextual operations* regarding Overlay.module versus jQuery Dialog in terms of a11y.

Bojhan’s picture

@WimLeers Its both crap atm :) So there is little to discuss, untill jQuery releases their improvements to jQuery dialog a11y which overlay needs to adapt.

falcon03’s picture

Project: CKEditor for WYSIWYG Module » Drupal core
Version: 8.x-1.x-dev » 8.x-dev
Component: Code » ckeditor.module
jessebeach’s picture

As Falcon03 mentioned in #51, much of the code in this thread was incorporated into the ckeditor patch that went into core.

I've rerolled this patch, mainly just replacing the custom ARIA live implementation with Drupal.announce. We can continue improving the keyboard/screen reader support from here.

falcon03’s picture

Status: Needs work » Needs review

Setting as "needs review" for testbot. I'll try to give a detailed feedback soon.

quicksketch’s picture

Assigned: quicksketch » Unassigned

This looks cool and I'll review it when I get the chance, though I probably shouldn't be assigned the issue.

mgifford’s picture

Definitely wanting to get @falcon03's & @quicksketch's view on this.

In my testing it seems to be much better. Unfortunately in moving a wysiwyg button with VoiceOver all I get is the name of the block repeated. There's no way of knowing what has been done or where that button is sitting now. It simply repeats the name of the button. Simply repeating "Outdent button" is no where near as useful as "Outdent button moved 2 down list"

I was testing here:
simplytest.me/admin/config/content/formats/full_html

catch’s picture

Category: task » bug
mgifford’s picture

mgifford’s picture

quicksketch’s picture

Title: Toolbar configuration for a WYSIWYG editor is not keyboard accessible; touch suport missing » Improve CKEditor toolbar configuration accessibility
Priority: Critical » Normal
FileSize
9.82 KB

Here's a reroll with the following changes:

  • I'd found that keeping track of the current button focus was difficult because the outline around the button disappeared when hovering the mouse on a button. normalize.css apparently removes outlines from links on hover, so I added specific CSS to keep the outline in place all the time on the current button.
  • There were multiple announcements happening on the same events, resulting in only the last announcement winning. In response to moving a button into a new row, the page would only announce "Active button row 2" instead of the more helpful, "Moved to active button row 2, position 1 of 10". To fix this I repurposed grantRowFocus() into announceButtonPosition() so its responsible for announcing all button positions, rather than just the row number. It's only triggered on button focus now, instead of row focus.
  • The adminToolbarButtonHelp() function is largely redundant with the new ARIA-based messages. There's no need to announce the button position on click when the button position is already announced on focus. So the click event no longer serves any usefulness for accessibility. To prevent confusion for all users, I converted the click event *back* to using alert(), so that the button does something when it's clicked (as opposed to doing nothing at all). If we decide that we don't want an alert on clicking, we should remove adminToolbarButtonHelp() entirely.
  • Fixed up a few places where we were using Drupal.t() inappropriately, switching us to use Drupal.formatPlural().

I'm also downgrading this issue to "normal" because the current system is already accessible in some way. A lot of our improvments were integrated into the original patch that got committed. This new patch is only improving upon the current work.

quicksketch’s picture

Wim Leers’s picture

Yay, quicksketch++ :)

@mgifford: curious what your a11y feedback to #64 is! :)

falcon03’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

I don't know the reason why, but the latest patch brings the toolbar configuration form to the point where no feedback about dragging and dropping was given by screen readers.

Anyway, from my blind-user perspective I find really hard to use this configuration form. The main problem is that I find really, really hard to understand which group a button belongs to. So it's hard to understand how the toolbar is going to look like after dragging and dropping some buttons (including dividers). Unfortunately hearing row and column position of a button doesn't give me the context I need to know exactly what I'm doing.

When CKEditor show its toolbar with a rich Textarea, it does something great. I don't know what markup it uses, but it shows a list of groups (toolbar menus) and I can easily surf through them. Once I find the group that I am interested in, I can easily surf its buttons (and not buttons that belong to other groups if I do not want to). I think that using the same approach in this configuration form would give me the context I need.

Long story short, I think the configuration form should work like this: the CKEditor toolbar should be rendered the same way it is rendered when it appears next to a rich textarea. Clicking a button should activate the "drag this element" event. After dragging this event, I should be able to surf the toolbar jumping from a group to another and/browsing buttons that belong to a group. When I do this, I would like to hear the name of the group or the label of the button that the cursor is placed on and, if I am dragging a button, something like "you're dragging x button" (where "x" is the label of the button that I am dragging). When I am dragging an element and I click on a button it should be dropped into the cursor position and I would like to hear something like "X button has been dropped".

Last (but not least), we shouldn't bind dragging and dropping buttons to using arrow keys.

Finally, I have to say that I am not sure this text is clear. Let me know if you need further details and/or have some questions!

quicksketch’s picture

Thanks @falcon03 for your feedback. Could you describe what method you use to drag and drop the buttons? I've been putting effort into ensuring that the interface is navigable via the keyboard (that should give you feedback after every button movement at least), but it sounds like you can organize a toolbar via a mouse even without seeing the position of cursor or the buttons. Do you use an equivalent of holding down the mouse button, moving the button to the new position, and then releasing the mouse button?

Matching the markup CKEditor provides normally is technically challenging because the groups don't exist yet while you are creating the toolbar. The way we're assembling groups currently is by letting the user move the group dividers to any position they want, and it will make the buttons on the left into one group and the buttons on the right into a second group. This makes for a simple implementation where everything is all in one list together, rather than having nested draggable areas.

quicksketch’s picture

Last (but not least), we shouldn't bind dragging and dropping buttons to using arrow keys.

On this last point, we don't think we can remove the keyboard support. It's fine if you don't use the keyboard support, but navigation via the keyboard is still a large factor in overall accessibility. Why would we want to remove it entirely?

falcon03’s picture

@quicksketch: My reply is between your questions.

You wrote:
Thanks @falcon03 for your feedback. Could you describe what method you use to drag and drop the buttons? I've been putting effort into ensuring that the interface is navigable via the keyboard (that should give you feedback after every button movement at least), but it sounds like you can organize a toolbar via a mouse even without seeing the position of cursor or the buttons. Do you use an equivalent of holding down the mouse button, moving the button to the new position, and then releasing the mouse button?

Technically emulating a mouse with a keyboard and a screen reader is possible, but it does not work well. When I said "we shouldn't bind the dragging and dropping to arrow keys" I meant that e.g. on Mac OS X i can browse the page using arrow keys, but also using some other shortcut provided by Voiceover (e.g. "Control + Option + Right arrow" to go to the next page element or "Control + Option + Left arrow" to go to the previous page element). I can also browse the page with a touchscreen mobile device (e.g. an iPhone) or with the Macbook trackpad, as well as tabbing through various page elements. After dragging a button, I would like to move it around with these other shortcuts, not only using arrow keys. But talking to @jessebeach in the Skype Accessibility group about another issue I found out that can be hard, if not impossible, to manage this kind of navigation. What do you think about?

You wrote:
Matching the markup CKEditor provides normally is technically challenging because the groups don't exist yet while you are creating the toolbar. The way we're assembling groups currently is by letting the user move the group dividers to any position they want, and it will make the buttons on the left into one group and the buttons on the right into a second group. This makes for a simple implementation where everything is all in one list together, rather than having nested draggable areas.

Ok, I can understand. But this is the most important issue I found: while for sighted user this mechanism can be obvious, I'm afraid that for blind users it cannot. Additionally, this mechanism has another issue: in CKEditor (but not when it is used by Drupal, see #1900246: Menu item labels have been lost and comment #3 of that issue for steps to reproduce), each group has a "title" (or "name" or "label", I don't know how it is called). When browsing between groups or between buttons that belong to different groups, I can hear the name/title/label of the group. Let's say that I want to add a new group: how can I set a label/title/name for the new group with our configuration form?

Wim Leers’s picture

The most important aspect you found, falcon03, is #1900246: Menu item labels have been lost if I understand you correctly.

The answer is simple: http://docs.ckeditor.com/#!/guide/dev_toolbar. There are two types of configuration:

  1. groups configuration. Strongest advantage: better a11y (because each group has a name that is pronounced by screen readers). (Also advantage: newly installed plugins' buttons automatically appear, because they come with predefined button-group associations, and this is the real reason why this exists.) Strongest disadvantage: impossible to control precisely where each button appears in the CKEditor toolbar. (Also strong disadvantage: much, much harder to make an easy to use UI for this.)
  2. item by item configuration. Strongest advantage: precisely control button placement (and easier to make this UI). Strongest disadvantage: worse a11y.

Now, it is possible to define names for any set of subsequent buttons when using item by item configuration (so: user-defined groups) — see the first example at http://docs.ckeditor.com/#!/guide/dev_toolbar-section-2. It's also possible to implement this. But it will make the UI inherently much more difficult to use, because the person that configures the CKEditor toolbar will have to think about assigning names, whereas now that's not necessary. Worse, the current CKEditor toolbar configuration UI (which is as minimal as possible) would have to be extended to somehow somewhere allow these names to be entered.

(I discussed this with the CKEditor developers and our current approach seemed to be the best compromise, at least initially: CKEditor is still fully accessible, only without the button groups.)

I hope this explanation was mostly clear.

Wim Leers’s picture

As per #1900246-6: Menu item labels have been lost, marked #1900246 as a duplicate of this one. That issue is about the symptoms/consequences of the choices we make on the CKEditor toolbar configuration side.

falcon03’s picture

@Wim Leers: thank you very much for this explanation. It was really clear and I can understand perfectly the situation now.

Unfortunately I have to say that group names are really important. As I have already said, group names allow me to skip easily buttons that I am not interested in, since I can browse through groups before browsing through buttons that belong to a group.

You can say that there are hotkeys to do quickly what I want, and you're definitely right. But people don't always remember hotkeys or know about their existence.

So I hope that we can refactor this configuration form to use group names (or maybe delete it at all, providing just a default toolbar configuration).

Wim Leers’s picture

Assigned: Unassigned » quicksketch

So I hope that we can refactor this configuration form to use group names

This is not a simple refactoring. That would require a rewrite (mostly). The UI will have to work completely differently, because of this entirely new concept of groups.

(or maybe delete it at all, providing just a default toolbar configuration).

This is absolutely unacceptable, because it'd make it impossible to customize the CKEditor toolbar configuration.

The only choices we have are: 1) rewrite, 2) to enter groups in the current UI: disable JS and enter JSON configuration for the CKEditor toolbar in the format that CKEditor expects (i.e. the current UI is JS-only), 3) live with slightly lesser a11y.

I think the only sane choice is #1 (rewrite), but there's of course only so much time and we want to move forward on other fronts too.

I sincerely hope quicksketch thinks I'm being silly and that the ability to set group names isn't synonymous to an (almost complete) rewrite. He's also the person who wrote the current drag-and-drop UI so he's definitely in a better position to judge this.

quicksketch’s picture

I agree with Wim that I don't think a default-only toolbar configuration would be a good fit for Drupal. We're now hinging (or hoping to hinge) other functionality on the toolbar, such as the allowed tag list, since it's easier for an end-user to place a button for bold than it is to enable a <strong> tag.

I can't immediately think of a good solution for naming individual groups. If you named a group, you'd also probably need to be able to move that entire group to a new position at the same time. Say if you named the first group "Formatting" that contained bold, italics, and strikethrough, but then you decided you wanted the first group to be "Media" instead, which contained the Image button. You'd need to be able to swap the positions of the entire groups at the same time. If you just moved the buttons, the group labels would now be mismatched with the buttons inside of it. At the same time, you still need to be able to move the individual buttons within groups. Like I said in #68, I don't think nested draggable objects is going to be a good solution.

Regarding places to enter a group label, here are a couple of ideas:

  • We could make some kind of inline-presentation of group labels. This would increase the noise on the configuration form quite a bit and it would make a new set of accessibility problems. I also don't know how we would fit inline fields in a mobile situation. Despite these problems, inline fields for group labels seems like it'd be the best option to me, assuming we could figure out the UX challenges.
  • We could put group configuration in a modal. Some kind of trigger would need to be added to open the modal, for which I don't have good ideas either. Something like a tab that extends from a group is a possibility, or maybe just clicking on any button in the group(?)
  • The simplest solution: we just but a bunch of textfields after the toolbar configuration. :P
  • We could add a toggle to the entire toolbar that replaces the display of the buttons with the display of the group names, with textfields in the place of the individual buttons. This would disable drag and drop and only allow naming of groups with the current button positions.
Wim Leers’s picture

We could add a toggle to the entire toolbar that replaces the display of the buttons with the display of the group names, with textfields in the place of the individual buttons. This would disable drag and drop and only allow naming of groups with the current button positions.

I think this is the most elegant solution. It's still a new set of accessibility problems though.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Needs work

I like that the patch in #64 keeps the CSS outline. I do think we should re-roll the patch to try it with the toggle as recommended by @Wim & @quicksketch

quicksketch’s picture

This issue has me worn down. The current patch is an improvement on the accessibility (though apparently not good enough yet). Adding support for named groups of buttons sounds like a feature enhancement and should probably be a followup. The end result of this issue is making me feel like writing a friendly UI for 99.9% of Drupal audiences isn't worth the disproportionate amount of effort to support the 0.1%. We've literally spend 5 times as long making the UI more accessible as it took to write it in the first place. As far as an effort/return investment goes, spending more time increasing the usability of a UI that administrators will need to configure once per site is not worth it.

Bojhan’s picture

@quicksketch I think thats understandable, a11y issues have worn me down in the past as well. But its likely there are some unique scenario's here at play, that make it hard for this to be easily accessible. I think its common when we invent new interaction patterns, that it comes with a significant a11y burdon in both figuring out the best approach and implementing it, sadly there is often no clear roadmap to fixing a11y issues. In the end we are doing this for the cause, there is no return on investment calculation you can make, I think thats important to remember even when you are demotivated by the substantial time investment in issues like these.

I think it makes sense for you and WimLeers to perhaps skype/chat with falcon03, because the propositions here seem to indeed carry way too much overhead that will negatively impact ux and code. I think we need to take a step back and revaluate our direction here. I don't think that manually assigning names to groups of buttons is an feasible option, you cannot introduce such overhead on the sitebuilder for any purpose.

Is it possible to do this should be done automatically - maybe those groups wil be non-sensical but I am quite sure for the majority of buttons this approach would be fine. In the cases where one wants very specific group names, because maybe there are dozens and dozens of special buttons - then contrib could provide this solution.

falcon03’s picture

@quicksketch: When I reported these issues I didn't want to demotivate you... I know that fixing them is hard, but unfortunately these issues exist and can give blind people a hard time.

As @Bojhan said, maybe we'd better chat to discuss these issues in detail. Let me know if you, @Wim Leers and anyone else are interested in this proposal, so that we can establish a good time for anyone (I'm from Italy). There is a Skype accessibility group, but we could also have a private chat on Skype or IRC.

We should figure out a solution as soon as possible, maybe before Drupalcon Portland; otherwise, as @mgifford suggested in the Skype accessibility group, we can sit at a table at Drupalcon Portland and figure out the best solution! :-)

mgifford’s picture

@quicksketch accessibility is a definitely a huge challenge for any evolving software. It's often a matter of madly rushing around to try to keep up with the really wonderful stuff that folks are developing (but that often introduce barriers for participation).

I was hoping I could find the relative growth of JS in D8 from http://www.ohloh.net/p/drupal but it's not nicely broken up at this point. The front end has gotten way nicer, but also substantially more complicated from a code perspective.

Picking an editor for D8 is a huge step. Sadly all of the editors have had accessibility challenges with them. CKEditor had some good ground initially on making improvements, but there are still lots of known issues:
http://dev.ckeditor.com/query?status=!closed&component=Accessibility

Not that it will be addressed quickly by CKEditor, but is this ultimately something that should be solved upstream in their issue queue? I didn't see it listed there. I am much more certain that this community can fix the problem. It isn't something right now that is even on their radar as far as I could tell.

quicksketch’s picture

The toolbar itself for CKEditor is already quite accessible. The original problem brought up here is that the task of configuring the toolbar is not accessible enough.

A second issue was brought up in #67. We're currently not naming our groups when they are configured. So in addition to making the task of configuring a toolbar accessible, we need to make sure that the resulting configuration is also accessible. (Basically, we need an accessible UI for creating an accessible UI). There aren't any problems with CKEditor that need to be solved here, we just need to figure out how to make a UI for configuring it.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -Needs accessibility review, -Spark, -CKEditor in core
mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Accessibility, +Needs accessibility review, +Spark, +CKEditor in core

The last submitted patch, ckeditor_toolbar-a11y-1872206-63.patch, failed testing.

Wim Leers’s picture

This is for after July 1. It's not a feature, so it's fine to fix it after July 1. We won't have time before then.

Wim Leers’s picture

Issue summary: View changes

Updated issue summary.

quicksketch’s picture

Assigned: quicksketch » Unassigned
jessebeach’s picture

Rerolled against HEAD (99dba0a3fc9880f8da74b897c98cf6265439a219).

Getting ready to start development on this issue again.

jessebeach’s picture

I've read through all the comments and feedback. Thank you all for the reviews and ideas for improvement. I've got a good sense of how we can make the configuration UX better for keyboard users without too much heavy lifting; just some clever usage of ARIA attributes and HTML structure. I should have something to test tomorrow some time.

Wim Leers’s picture

Yay! :D Go Jesse!

jessebeach’s picture

Priority: Critical » Major

I suggest we address this issue in two parts:

  1. Improve the CKEditor authoring experience by plumbing the toolbar group names through to the editor HTML. See the CKEditor documentation on toolbar sections for details on the distinction between a list of buttons and lists of buttons with groups: http://docs.ckeditor.com/#!/guide/dev_toolbar-section-2
  2. Improve the editor configuration experience (essentially what this issue is) after we know what the target configuration structure will be.

I'd like to make first part, improving the authoring experience, the Critical issue. This will arguably affect more people, since creating content is an activity that will happen magnitudes of order more often than configuring an editor.

We would then downgrade this issue (part 2) to Major. The editor configuration is possible with a keyboard, but the usability and UX are terrible. Such a bug is not usually a release blocker, but it shouldn't be a Normal.

I've made #2072927 a Critical and downgraded this issue to Major. I'm going to work on #2072927 first.

falcon03’s picture

@jessebeach: thanks for your efforts. I'm really looking forward to test a patch for the other (critical) issue. From what I understood, it was caused by our current way of configuring the CKEditor toolbar because it does not store which group a button belongs to nor allows an user to type the name of a custom group created through the configuration form.

Wim Leers’s picture

#93: that's correct!

jessebeach’s picture

Issue tags: +sprint

Adding a sprint tag to track the current work for the Spark team.

jessebeach’s picture

Priority: Major » Critical
FileSize
32.92 KB

Ported work from #2072927-8: Improve CKEditor toolbar authoring accessibility here.

Sorry for the issue churn. I'm trying to get these issues addressed hewing as closely as possible to a minimal viable product. It turns out that we can't improve the authoring experience without also improving the configuration experience at the same time, so that the configuration schema integrity is maintained.

Tests are not yet passing. My focus now is to get all the CKeditor tests to pass, which mean updating the CKEditor admin forms to reflect the new configuration schema. This will break the UI at first. Focusing on the tests now will at least bring the functionality back to where it was before the configuration change to introduce toolbar groups.

jessebeach’s picture

Rerolled on e2e00fdae4b088d62ec2a7164ef401f3c84c6967

falcon03’s picture

I know that the patch is not complete yet, but I wanted to give it a try; unfortunately, it seems that the toolbar is no longer shown (or, at least, a screen reader can't intercept it) after applying the patch.

jessebeach’s picture

falcon03, you couldn't find the toolbar on the text format configuration form or on a node edit form?

You're right, the patch here is definitely in a "needs work" state. It's my next task.

falcon03’s picture

@jessebeach: I couldn't find the toolbar on the node edit form. I know that the patch needs work, but I just wanted to report the issue in case it was not known. :-)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
4.59 KB
39.62 KB

Sorry, the patch in #97 contained an error, so the toolbar wasn't rendering because of it.

The patch fixes that error and cleans up the rest of the ckeditor tests. All tests are now passing.

This allows us to start updating the JavaScript component of the module to enable an accessible interactive experience.

jessebeach’s picture

Status: Needs review » Needs work

Setting back to needs work. I just wanted the bot to run to verify that we're starting from a clean test base with CKEditor toolbar groups plumbed through from configuration to HTML in both the configuration screen and on an entity edit form.

jessebeach’s picture

Incremental update. Buttons are now drag-droppable between groups only. Groups are sortable as well.

Next patch

  • Fix the parsing the ckeditor toolbar DOM structure so that it populates the configuration textfield correctly. Currently configuration changes cannot be saved correctly.
  • Make rows addable/removeable
  • Make groups addabled/removeable/editable
  • Fix placement of dividers.
jessebeach’s picture

This is still a patch under development, so it might contain some debug and console statements.

The configuration of a toolbar can now be editing through drag and drop again. Buttons can be shifted as well as toolbar groups. Save works as expected.

I'm cleaning up the HTML, CSS and ARIA attributes now.

jessebeach’s picture

I was tumbling further and further into spaghetti code as I tried to work changes into the existing structure. It finally struck me that I had to refactor the code to do the type of cleanly separated visual, aural and keyboard views we need to make this accessible. So this patch introduces Backbone and our View patterns to this configuration component.

jessebeach’s picture

I've done extensive testing on the drag-drop configuration interaction. You can drag buttons to new groups and rename the groups. I'll be working on the keyboard interactions next.

Wim Leers’s picture

First: wow, this is a huge patch. I guess that was to be expected.

Manual testing

When using CKEditor on the full node edit form

  1. Basic HTML's CKEditor configuration has two rows of buttons, should be one. I think this is only for your testing.

The configuration UI

  1. When dragging a button into a new group and then clicking "Cancel", it ends up in the "new group" placeholder, rather than being reset into the list of available buttons.
  2. Creating new rows is now less obvious than it used to be. Wouldn't it be better to have an explicit button to be pressed to add a new row? It would also help in reducing the required vertical screen estate.
  3. Previously, the representation of the active toolbar on the configuration matched the eventual toolbar that end users would get to see pretty well. No longer. I wonder if we want some sort of "preview" mechanism, or a way to toggle the group names to be hidden?
  4. The description of the "Toolbar configuration" fieldset should be updated, to explain the group names concept.
  5. The live updating of the "filtered HTML" settings is broken. In other words, #1894644: Unidirectional editor configuration -> filter settings syncing is broken! :(

Code review

(Only the first 20% or so, i.e. pretty much only PHP — will review more tomorrow.)

  1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -80,8 +91,13 @@ function theme_ckeditor_settings_toolbar($variables) {
    +      'data-drupal-ckeditor-button-name' => $button['name'],
    

    Why the more explicit prefixing? It's fine, but it's also overkill.

  2. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -118,6 +134,19 @@ function theme_ckeditor_settings_toolbar($variables) {
    +  $print_button_group = function($buttons, $group_name, $print_buttons) {
    

    This is a very odd new pattern, it's kind of like a "partial theme function". I've seen shorter ones , but because this one is so long, it feels weird. Not saying you should change it, but let's keep this in mind and if you see a way to make it feel less weird, go ahead :)

  3. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -118,6 +134,19 @@ function theme_ckeditor_settings_toolbar($variables) {
    +
    

    Extraneous newline.

  4. +++ b/core/modules/ckeditor/css/ckeditor.admin.css
    @@ -6,61 +6,81 @@
    +  border: 1px solid whitesmoke;
    

    Hahaha, "whitesmoke" :D

  5. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -4,204 +4,165 @@
    +      // Create the confiugration Views.
    

    typo

  6. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -4,204 +4,165 @@
    +    sync: function () {
    +      // Push the settings into the textarea.
    +      this.get('$textarea').val(JSON.stringify(this.get('activeEditorConfig')));
         }
    

    Nice :)

jessebeach’s picture

Basic HTML's CKEditor configuration has two rows of buttons, should be one. I think this is only for your testing.

Correct, I'll revert this back soon. I just needed to test that row manipulation works.

Creating new rows is now less obvious than it used to be. Wouldn't it be better to have an explicit button to be pressed to add a new row? It would also help in reducing the required vertical screen estate.

I'm looking ahead to the keyboard interaction. If you pick up a button with the keyboard before you create a row, you'll need to put that button back, navigate to the create row button, navigate back to the button list, grab the button and then navigate to the new group in the new row. If the groups are always available, then you're never stuck with that egg-chicken problem. Any time you pick up a button, you can place it in an existing group or create a new one just by placing the button.

We could reduce the vertical space by making the placeholder groups visible only during a drag-drop sequence.

Previously, the representation of the active toolbar on the configuration matched the eventual toolbar that end users would get to see pretty well. No longer. I wonder if we want some sort of "preview" mechanism, or a way to toggle the group names to be hidden?

I was thinking the same thing. Maybe we open the toolbar into a "preview mode" as soon as the user goes to move a button. We'd have a toggle as well, but we can trigger that toggle automatically when they express through interaction the intent to configure the bar.

The live updating of the "filtered HTML" settings is broken. In other words, #1894644: Unidirectional editor configuration -> filter settings syncing is broken! :(

Crap, I thought I'd had that working :/ It at least worked with the <hr> tag. Oh how I wish we had front end tests for these sorts of things.

This is a very odd new pattern, it's kind of like a "partial theme function". I've seen shorter ones , but because this one is so long, it feels weird. Not saying you should change it, but let's keep this in mind and if you see a way to make it feel less weird, go ahead :)

I just extended existing code here. The pattern feels JavaScripty so I went with it. It could do with a theme function instead.

jessebeach’s picture

I was only able to get to the button group dialog code today. I added validation that a name is provided and dealt with the cancel button. I also spoke to tkoleary about the design changes and got input from him. Most importantly, he believes that the drop targets are not larger or prominent enough.

jessebeach’s picture

I've added all of the keyboard controls to move buttons, groups and dividers around. The last bit of work is a @todo that I added to deal with canceling the naming of a group. In this case, the moved button needs to be put back in the position it was in before it was moved into a new group.

Dealing with setting a key press event on the Drupal dialog form took me about 2 hours to untangle. Handling an enter key press is wicked difficult because you also need to suppress the form submit. It took me a while to finagle just the right combo of bubbling suppression and bindings to get the behavior I wanted.

Up next, linking aural updates to the model/DOM updates in the Aural View.

Wim Leers’s picture

I reviewed the overall JS. I think this is heading in the right direction; it's indeed applying structure to the spaghetti that was there. It already is a big, big step forward :)

I think it could still be cleaner/more explicit/more structured, but I think that's because you're not done yet. Maybe we should down together and figure out what's worthwhile — we don't want to spend too much time on this, but then again it's also what enables better/easier tight CKEditor plugin integration with Drupal 8 over the next >5 years, so it might be worth it after all.


As per the above, I only have few remarks for now:

  1. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -305,16 +267,70 @@ Drupal.behaviors.ckeditorAdmin = {
    +    sortAvailableFeatures: function (features) {
    

    This function doesn't do any sorting; it ensures that CKEditor buttons that are enabled ("active toolbar") are also allowed by the filter settings, if not, then it disables them ("available buttons").

    Something like disableFeaturesDisallowedByFilters, or the less verbose but also less exact disableDisallowedFeatures seems more appropriate.

  2. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -334,16 +350,17 @@ Drupal.behaviors.ckeditorAdmin = {
    +        // @todo I think this is a dead code path.
    

    Not AFAIK. E.g. enable the "Styles" button. Now go make configuration changes. That will trigger this event, and hence will fire this event handler.

  3. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -355,165 +372,701 @@ Drupal.behaviors.ckeditorAdmin = {
    +      // Remove the dividing elements if any.
    +      return _.without(buttons, '-', '|');
    

    Beautiful!

  4. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -355,165 +372,701 @@ Drupal.behaviors.ckeditorAdmin = {
    +        '@row': getRowName($destinationRow),
    

    this.getRowName

  5. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -355,165 +372,701 @@ Drupal.behaviors.ckeditorAdmin = {
    +        '@position': (destinationPosition + 1),
    +        '@totalPositions': $destinationRow.children().length
    

    These variables are not defined in this scope.

  6. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -355,165 +372,701 @@ Drupal.behaviors.ckeditorAdmin = {
    +     * @param {jQuery} event
    ...
    +     * @param {jQuery} $row
    ...
    +     * @return {String}
    

    Nitpick: AFAIK Drupal doesn't use {type} argName style docs, but type argName. There is some code that does, but it's the exception.

  7. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -355,165 +372,701 @@ Drupal.behaviors.ckeditorAdmin = {
    +      var $currentRow = $button.closest('.ckeditor-buttons');
    

    Unused.

jessebeach’s picture

This update completes the work to produce an aural interface for changes to the toolbar configuration. It's good enough to test while we're at DrupalCon Prague.

I updated the Drupal.behaviors.detach method. It had not been updated to parse the new configuration correctly.

I fixed canceling placement of a button in a new button group with the keyboard. The button move was not properly reverted through the keyboard view.

Still left to be addressed.

  1. Respond to Wim's reviews in #107 and #111
  2. Many functions need comment updates. I got through about 50% tonight.
  3. The configuration preview, with the toolbar palette group names always visible, does not give the site builder an accurate representation of what the CKEditor will look like. We need a toggle that hides and show the toolbar palette group names on demand.
  4. Revert the Basic format configuration to just one row.
  5. Reduce complexity and redundancy in the code as much as possible given time constraints.
  6. Revert testing styles back to standard focus and highlighting styles.
jessebeach’s picture

Status: Needs work » Needs review
FileSize
38.48 KB
108.25 KB

Here's what I have from work completed during DrupalCon Prague. I've commented all the new code and jsHinted it.

I have not attempted to refactor all of the JS or PHP code that relates to the editor configuration. So there are still some odd patterns in the code that existed before.

What I could use some help with now is verifying that the feature checking code still works. For example, when the configuration page first loads, the Styles feature vertical tab is always present, even though the button for the styles dropdown is not part of the Basic editor configuration. If the styles button is added to the toolbar and then removed, the styles vertical tab is removed as well, as expected. So something in the initialization sequence is running correctly.

Other than verifying the ckeditor features support, we'll want to do a heuristic and usability evaluation of the interactions introduced in this path. The patch can be tested on a path like this one: /admin/config/content/formats/manage/basic_html

jessebeach’s picture

quicksketch’s picture

I tried this out in Simplytest.me and found a bunch of strange behaviors, sorry if you were already aware of these:

- In Firefox, the "focus" outline when highlighting buttons now shoots off the left side of the screen.
- I expected that I could hit the delete key to remove a button from the toolbar. Apparently the old version didn't do this either, but after leaving it alone for a while and coming back to it, I had assumed this functionality to be part of the keyboard support. Maybe it should be a separate issue. Removing a "separator" element is particularly difficult with the mouse, so accessing it with the keyboard and hitting delete would be a welcome ability.
- Likewise in the "expected but didn't work" category, the Escape key doesn't work within the modal for canceling the creation of a new group.
- I'd like to see the group names give indication that they will do something when clicked. This could be done with setting the cursor on span.ckeditor-toolbar-group-name to "hand" and setting a hover style (i.e. make it look like link), or the span could just be changed to an anchor tag directly but that would affect the tab key behavior. (I didn't realize you could just set tabindex=0 to allow spans to be focusable, but maybe using a native element that is always accessible by default is better?)
- Actually I just realized you can drag the entire group... that needs visual indication too. Maybe we could use CSS to put a grabbie handle on each group to indicate it's draggability?
- When used in the overlay, moving a button to a new group with the mouse causes the button to jump out from under the cursor. As long as you ignore the visual misdirection, dropping the button still works. When overlay is not used, this problem does not occur.

Overall, the changes look great. The ability to specify groups is unobtrusive yet part of the workflow. The only thing I'd like to avoid changing is the always-there new row. I find the explicit actions for adding/removing rows to be more clear than automatic removal and addition of rows.

Wim Leers’s picture

Feedback

  • I love the way the group naming is not omnipresent, which I feared would be the end result :)
  • The code is already much more maintainable now that we're using Backbone Views!
  • The "Show/hide group names" button: excellent :) The ability to drag entire groups around is also very nice indeed, but as quicksketch says, it could use some kind of indication that it *is* draggable. You can only find out accidentally right now.
  • When dragging in a new button, the "Show group names" button is "clicked" automatically: the group names appear. Nice :) However, wouldn't it be better if this appeared instantaneously when dragging? I felt that was better, so I implemented it that way.
  • #113:

    For example, when the configuration page first loads, the Styles feature vertical tab is always present, even though the button for the styles dropdown is not part of the Basic editor configuration. If the styles button is added to the toolbar and then removed, the styles vertical tab is removed as well, as expected. So something in the initialization sequence is running correctly.

    That is being fixed in #2089631: Showing/hiding of CKEditor plugin settings is fragile, automate it (breaks in narrow viewports and when enabling CKEditor), it's a pre-existing problem that we don't need to solve *here*.

Changes

  • FIXED: There was inconsistency WRT the naming of the "button groups" concept. Sometimes it was called a "palette" (or even "@type" when outputting strings), which is a word that never appears in either CKEditor's documentation, nor in the discussions we've had so far. So, I simply consolidated it to "group".
  • Related: The show/hide toggle for group names labeled group names "group titles", which is inconsistent with the code, and with the term used in the dialogs. Renamed to "group names". (And hence also updated the ConfigurationModel's groupTitlesVisible attribute.)
  • CKEditorAdminTest didn't pass anymore; fixed it.
  • FIXED: BUG: when enabling CKEditor for a text format that didn't have any text editor before, and that text format doesn't allow tags that correspond to buttons in CKEditor's default list of buttons (e.g. Restricted HTML doesn't allow for images), then those buttons are correctly removed from the default active toolbar, but instead of showing up in the list of available buttons, they've completely disappeared!
  • FIXED: BUG: editing the button group name did not show the current button group name. In fixing this, I also discovered that old jQuery UI dialogs were not being cleaned up correctly: they continued to exist in the DOM, causing the autofocus functionality to fail as well. I opened a new issue for this, since it's unrelated to this issue: #2107199: Allow Close(Modal)DialogCommand to remove the dialog from the DOM, document how client-side dialogs can do that too. That issue provides a universal solution, but this issue luckily does not depend on that issue :)
  • FIXED: BUG: button group name input focusing didn't work, I had to replace input.focus() with $(input).focus(), unfortunately.
  • IMPROVED: the button group names appear automatically when dragging in a new button, but only after dropping it, which means it doesn't help the user while dragging, which I think is the actual goal. So I changed that.
  • IMPROVED: there was a comment saying that an announcement must be made when the user has opened a dialog for editing the button group name, but there was no code actually doing that. I added that.
  • FIXED: BUG: from #115: "Likewise in the "expected but didn't work" category, the Escape key doesn't work within the modal for canceling the creation of a new group."
  • FIXED: BUG: the ConfigurationModel Backbone model was defined slightly incorrectly, which caused its default values to not be picked up, which then caused subtle problems later on.
  • IMPROVED: from #115: "I'd like to see the group names give indication that they will do something when clicked. […] Actually I just realized you can drag the entire group... that needs visual indication too."
  • FIXED: BUG: from #115: "In Firefox, the "focus" outline when highlighting buttons now shoots off the left side of the screen." — found the solution at http://www.webbykat.com/2013/01/fixing-too-wide-focus-indicator-firefox-..., this is a bug in Firefox AFAICT.
  • FIXED: BUG: when enabling CKEditor for a text format that didn't have any text editor before, the group names end up being display: inline'd, through an inline style attribute. It was a jQuery.toggle() side-effect (to show/hide the button group names). Now I'm using jQuery.toggleClass() plus some CSS to show/hide the button group names, and the problem is gone.
  • FIXED: BUG: moving a placeholder button group within one row is forbidden, but it is possible to move row 2's placeholder button group to row 1, so that row 2 has none of them and row 1 has two. I used jQuery.sortable({cancel: 'SELECTOR'}) to simply exclude the placeholder groups from being draggable, which also fixes the problem, and simplifies endGroupDrag(). I also set an appropriate cursor and made sure the user cannot accidentally select the group names text by adding user-select: none CSS.
  • FIXED: BUG: None of the "setting syncing" code was working (#1894644: Unidirectional editor configuration -> filter settings syncing). It was actually a server-side problem, this was the culprit:
    @@ -175,7 +192,7 @@ public function settingsForm(array $form, array &$form_state, EditorEntity $edit
           'editor' => 'ckeditor',
           'settings' => array(
             // Single toolbar row that contains all existing buttons.
    -        'toolbar' => array('buttons' => array(0 => $all_buttons)),
    +        'toolbar' => $editor->settings['toolbar'],
             'plugins' => $editor->settings['plugins'],
           ),
         ));
    

    That small change, which you probably did an early stage, caused the CKE configuration UI to not break anymore, but at the same time it now included only the *enabled* buttons, rather than *all* buttons!

  • … plus a bunch of very minor changes, including quite a few docs additions.

#115: most of your feedback has been incorporated! :)

I cannot reproduce

When used in the overlay, moving a button to a new group with the mouse causes the button to jump out from under the cursor. As long as you ignore the visual misdirection, dropping the button still works. When overlay is not used, this problem does not occur.

I didn't implement

The only thing I'd like to avoid changing is the always-there new row. I find the explicit actions for adding/removing rows to be more clear than automatic removal and addition of rows.

yet because now everything *is* working again, so we can review the rest first, then determine if there's anything else that needs work besides the way we allow users to create new rows.

mgifford’s picture

I just did a quick test and we need more input from screen reader users. However, couple things from an accessibility point of view.

I couldn't approve the title without using a mouse here:

Can't Approve the title without a mouse

I also wondered why you were using this class rather than the visually-hidden, but then saw the Show/Hide group names button on the right. That probably makes sense to do it this way.

I do think it would be better to use a heading rather than a span, as that would provide a better relationship to the list of links that follow:

<h3 class="ckeditor-toolbar-group-name">
    Linking
</h3>

We could also use the aria-labelledby attribute as described here: https://developer.mozilla.org/en-US/docs/Accessibility/ARIA/ARIA_Techniq...

There should be a semantic relationship though between the header and the list though.

Wim Leers’s picture

#117: thanks for the prompt review!

The button group name dialog
What do you mean by "I couldn't approve the title without using a mouse"? I *think* you mean that you couldn't trigger the "apply" button without using the mouse. Well, that's not true, unless it's a browser-specific issue:
  1. you can just press enter to press "apply" (or ESC to press "cancel")
  2. you can tab to it as well, but the button:focus styling is far too subtle
"I also wondered why you were using this class rather than the visually-hidden"
Which class?
"We could also use the aria-labelledby attribute"
Great find! I didn't touch any ARIA stuff at all in #116. Now I have. Changes:
  • FIXED: BUG: newly created groups are not tabbable
  • FIXED: BUG: newly created groups are not movable with the keyboard
  • IMPROVED: at least AFAICT, the data-drupal-ckeditor-toolbar-group attribute on button groups was useless.
  • IMPROVED: the per-button data-drupal-ckeditor-toolbar-group attribute (which was actually the group name, not the HTML-class-ified group name) was only being used internally in the PHP, not in the JS, so I made sure that it doesn't show up on the client-side anymore. Slightly simpler HTML on the front-end now.
  • FIXED: BUG: correct use of aria-labelledby to label button groups.
jessebeach’s picture

Wim addressed quicksketch's review in #115 (thanks for the review!) with his patch in #118. Wim, thanks for the input! The one comment that hasn't been addressed is this one:

The only thing I'd like to avoid changing is the always-there new row. I find the explicit actions for adding/removing rows to be more clear than automatic removal and addition of rows.

This UX pattern represents, to me, a compromise. Adding a specific action to add or remove a row means that a user, specifically a blind user, must make a decision to add a row before a button or group is "grabbed". With the UX pattern in this patch, a user must simply grab a button or group and move it around. There are no decisions to be made beforehand regarding where the button or group will be placed; you can create a place to drop it as part of the moving action.

I would really like to keep this pattern as a compromise between visual and non-visual folks.

So, in this update, I've done the following:

  1. Returned the basic html format editor configuration to its one-row default. I had inserted two rows for development.
  2. I refactored the CSS a little more to conform to standards.
  3. The span element that wrapped the group label has been changed to an h3 following mgifford's comment in #117. I believe the group labels are using the aria-labelledby attribute correctly already.

This patch feels ready for testing for me. I'd love to get falcon03's input since he has already put so much thought into this issue.

jessebeach’s picture

Test the patch in #119 on simplytest.me

Navigate to this path once you have installed your new site: /admin/config/content/formats/manage/basic_html

Wim Leers’s picture

Your explanation for why we shouldn't have the add/remove row buttons makes sense. I had the feeling you did it for a reason like this one, I'm glad I didn't change that yet.

And with that, I believe we are indeed in the final stretch for getting this patch committed. I personally have no further complaints, since I fixed those myself already in #116 and #118.

Just for good measure, I went through the code again, looking for flaws. I found a few nitpicks (that I missed myself earlier):

  1. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -1,207 +1,194 @@
    +      //
    

    This still needs a comment, I forgot to add that in #116.

  2. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -1,207 +1,194 @@
    +        // Process the rows
    

    Missing trailing period.

  3. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -305,21 +305,83 @@ Drupal.behaviors.ckeditorAdmin = {
    +          // Default toolbar buttons are in fact "added features".
    

    s/Default/Existing/

    (I know, I added this comment, my bad.)

  4. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -329,21 +391,22 @@ Drupal.behaviors.ckeditorAdmin = {
    +        // @todo I think this is a dead code path.
    

    Not, it is not :)

    Enable the StylesCombo button. Then start typing some styles in its setting, e.g. "p.callout|Callout" and "address.special|Special address". Then blur the textarea where you typed those things.

    Now a CKEditorPluginSettingsChanged event will fire, which will trigger this code path. This code path will then talk to the server, retrieve the updated "hidden CKEditor config" which is used to generate features metadata, and then the updated features metadata will cause the <address> tag to be added to the list of allowed HTML tags.

    As you can see, a very necessary code path :)

  5. +++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/CKEditorPlugin/DrupalImageCaption.php
    @@ -76,12 +76,21 @@ function isEnabled(Editor $editor) {
    +    // @todo, This parsing of the settings structure should be provided by a
    +    // method on the CKEditor, but $editor here is not a CKEditor, it's a
    +    // generic editor. I'm not sure how to get a reference to the CKEditor.
    

    Why? Because doing something like this requires duplicating this type of logic everywhere?

    This is not *that* common to do, and neither all that complex.

    I don't think we have to provide convenience methods for *everything*.

  6. +++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php
    @@ -94,13 +94,29 @@ public static function create(ContainerInterface $container, array $configuratio
    +          // Button groups
    

    Missing trailing period.


Leaving at "needs review", because these things are code nitpicks, they're not blocking accessibility review!

quicksketch’s picture

FileSize
183.28 KB

This is looking really good. Still experiencing the "disconnected cursor" problem. Again, it only happens *in the overlay* and *in Firefox*. :\

Icon jumping out from under cursor

Personally, I find the always-there new row to be a trade-off that puts emphasis disproportionally on accessibility over usability, but overall this patch clearly moves things forward overall on both fronts, so I can't knock it.

mgifford’s picture

Just a re-roll of 119.. Someone else needs to apply changes from 121.

Status: Needs review » Needs work

The last submitted patch, ckeditor-configuration-a11y-1872206-123.patch, failed testing.

mgifford’s picture

Not sure why I'm getting these errors here admin/config/content/formats/manage/basic_html

Notice: Undefined index: rows in template_preprocess_ckeditor_settings_toolbar() (line 35 of core/modules/ckeditor/ckeditor.admin.inc).
Warning: Invalid argument supplied for foreach() in template_preprocess_ckeditor_settings_toolbar() (line 35 of core/modules/ckeditor/ckeditor.admin.inc).
Notice: Undefined index: rows in Drupal\ckeditor\Plugin\Editor\CKEditor->settingsForm() (line 155 of core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php).

I'm happy to say that it's basically working now. The only problem I came across with creating the custom toolbar had to do with the "new group". From a usability perspective it's bit shocking for someone to bring along a button and suddenly get a dialog asking for a "Button group name", when it's not clear you're at the end of the list and are being presented with an opportunity to create a new group.

Button Group name required

What is the button row before the row of groups? That doesn't make a lot of sense to me, but you can with a mouse move items into this row above the groups.

Ungrouped Items

The row of group numbers are actually wrong though. The first row of grouped elements shouldn't be row 2 of 2 and the 2nd row definitely shouldn't be button group 3 of 2.

Button Group in Row 2 of 2
Button Group in row 3 of 2

It did work though the first time I tried it. I could position the buttons and have a good sense of where I was. This is just how @quicksketch's bug above looks like in Safari & VoiceOver.

I reloaded the page though and then it just jumped down the the vertical tabs below.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
3.34 KB

quicksketch, for the longest time I couldn't reproduce the displacement of the drag icon that you documented in #122. Then I happened to scroll the overlay a little and I tried again. Bingo. So what seems to be happening is the jQuery UI Sortable plugin is not taking the iframe's scroll offset into account. Or the plugin doesn't deal well with a fixed positioned, scrollable iframe. This is not altogether unreasonable. But it means that we can't really fix this without hacking the plugin.

Luckily, Overlay's days are probably numbered. #787896: Add a link so that administrators can return to their most recently visited non-admin page

I've addressed all of Wim's comments in #121.

I would of course really like to get someone to give another pass on an accessibility review for this patch. That being said, I do strongly feel that these changes represent a significant incremental improvement. Further improvements are of course possible. For instance, we are now using the "complex" CKEditor configuration that specifies toolbar groups in the end-user UI. For blind authors using an editor to create content, this means they will be able to navigate the toolbar using the best accessibility standards that CKEditor has in place. That change required us to update the configuration UI at the same time. I have striven to make this UI expressive both visually and aurally. I'd love to get verification of this. Below is a link to test this patch on simplytest.me.

Test on simplytest.me

Status: Needs review » Needs work

The last submitted patch, ckeditor-configuration-a11y-1872206-124.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review

mgifford, I believe the "extra row" issues you're running into are from a corrupted reroll or install. Try the patch in #126.

The only problem I came across with creating the custom toolbar had to do with the "new group". From a usability perspective it's bit shocking for someone to bring along a button and suddenly get a dialog asking for a "Button group name", when it's not clear you're at the end of the list and are being presented with an opportunity to create a new group.

I agree. I can add a string to the announcement when a user reaches the last existing group, that moving the button out of the current group will create a new group.

jessebeach’s picture

Addressing mgifford's usability concerns from #125:

For a button, the option to create a new row is announced when the active button is in the first position of the last row. Moving a button to another row always places the button in the first position, so this announcement will be heard whenever a button is moved into the active toolbar or moved between rows. The option to create a new button group is announced when the active button is in the last position of the last group of a row.

"italic button in position 1 of 1 in Formatting button group in row 2 of 2.
Press the down arrow key to create a new button group in a new row.
This is the last group. Move the button forward to create a new group." 

The option to create a new row with an active group is announced when the group is in the first position of the last row.

"Tools button group in position 1 of 2 in row 2 of 2.
Press the down arrow key to create a new row."

The extra announcements add more context. Thanks for the suggestions!

Test this patch on simplytest.me.

Do we need to do anything else to get this committed?

Status: Needs review » Needs work

The last submitted patch, ckeditor-configuration-a11y-1872206-129.patch, failed testing.

jessebeach’s picture

Rolled a new patch. I think the last one was rolled incorrectly.

Test this patch on simplytest.me.

jessebeach’s picture

Status: Needs work » Needs review

go bot, go

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs accessibility review

From #126:

That being said, I do strongly feel that these changes represent a significant incremental improvement.

Yes. Let's not let good (or in this case: "very significantly better") be the enemy of perfect.

Jesse has been pushing this forward for many, many weeks. It may not be perfect. But unequivocally, it's a huge step forward on two levels:

  1. The resulting CKEditor toolbar as presented to the end user is now fully accessible.
  2. The UI to configure the CKEditor toolbar is now usable for screenreader users.

Let's achieve perfection in further iterations to fix the remaining small problems individually when they are discovered.

So, I say, let's get it in, so we can stop rerolling a >100 KiB patch.

(I also tested the patch in #131, and it's working great.)

cosmicdreams’s picture

+1 RTBC, but this will need a follow up to fix the remainder.

webchick’s picture

Really sorry, but this no longer seems to apply for me. :(

Re-testing to see if this is just my problem.

webchick’s picture

Oh, wait, I get it. #2089631: Showing/hiding of CKEditor plugin settings is fragile, automate it (breaks in narrow viewports and when enabling CKEditor) conflicts. I will test this one and if it's good to go, I'll roll back the other one in the meantime since this is a critical. Stay tuned!

webchick’s picture

I have not read the rest of this issue yet; I'm probably repeating stuff other people have said, but I like going into UX issues blind so I can not be tainted. I mean no disrespect. I'll go back after this and read what other people said.

So the biggest thing, and I don't claim to be an accessibility expert by any means, but this is whack:

Each button in the toolbar configuration palette says its name twice and then all of the instructions for how to use the toolbar configuration palette

When I would expect to get that huge whack of instructions is when tabbing into "Toolbar Configuration," *not* on every single button I tab into within that fieldset. That feels like a commit blocker, unless I've just done something horrible to my machine.

Second thing. I love the "Show/hide group names" button there. Lovely. Is there a reason we can't default this to "hide" like we do row weights, skip links, and various other accessibility features? This way, there's no degradation as far as a visual user is concerned. It seems that when you attempt to drag in a toolbar icon, it automatically shows the group names (yay!) so that shouldn't be too terrible of a change.

There's a lot of jarringness that happens here:

Dragging a button from the palette into a new group...

A modal with a totally blacked on background takes over page!

I notice that when you click on a group name, you still get the same modal, so can we just make it so this "auto-pop" doesn't happen on first drag, and your group is just called "New group" until you deliberately name it something different?

And finally, this is happening (once again, in Firefox 24.0, OSX):

- Do various configurations. For example, move button A into Formatting, button B into Lists, whatever.
- Either drag a button into an unnamed group, or click on an existing group name to get the pop-up. Enter "Whatever" and hit ENTER.
- The screen reloads and you lose all of your changes. (*&!*(&!(@!*

:(

Hopefully these are all small, reasonable changes to fix. I would really love to get this in for alpha4 (Friday) if possible, so we can get further accessibility testing done e.g. at BADCamp on a non-moving target. GREAT work getting it this far! If any of these are horrible things that need tons of more time, please push back and we can resolve them later, as long as they don't introduce critical issues (the two critical ones feel like the "hitting enter causes data loss" bug [unless that's just an issue with modals generally] and the "available buttons are not actually accessible because they repeat the entirety of all of the instructions every time" bug).

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Sorry. :\

webchick’s picture

Ok, now read the past 50 or so comments, although it is past midnight here so I might've missed something. :)

Looks like both Mike and I ran into the "AHHHH!!" problem with the modal. ;)

quicksketch said:

"The only thing I'd like to avoid changing is the always-there new row. I find the explicit actions for adding/removing rows to be more clear than automatic removal and addition of rows."

I agree with this, too. Jesse stated it's a compromise in the design for both visual and blind users, which is fair enough for initial commit, I think. But let's make a "normal" follow-up for it to see if we can brainstorm something even better later.

Looks like we might also need a follow-up to further tweak the ARIA language, per Mike's comments.

The overlay blip also needs a follow-up, but it should be postponed on nod_'s patch. Then hopefully we can mark it "won't fix" shortly thereafter. ;)

Wim Leers’s picture

First thing
I'll leave that for Jesse to answer.
Second thing: "sweet merciful crap, what happened to my screen" + "can we just make it so this "auto-pop" doesn't happen on first drag, and your group is just called "New group" until you deliberately name it something different?"
No, we can't do that. When a user creates a new group, that's a conscious choice. Hence it must be consciously named. If we don't force the user to choose a name, then many will stick with the default of "new group" and then we end up with CKEditor instances for end users that are equally bad or even worse than before this patch.
Third thing: "Enter "Whatever" and hit ENTER."
I bet that's a Firefox-specific bug because I've never seen that happen. I'm hopeful this will be a trivial fix for Jesse.
webchick’s picture

"No, we can't do that. When a user creates a new group, that's a conscious choice. Hence it must be consciously named. If we don't force the user to choose a name, then many will stick with the default of "new group" and then we end up with CKEditor instances for end users that are equally bad or even worse than before this patch."

What? No one's going to leave something called "New group" sitting there. :) If they do, then they deserve whatever they get, and their users will yell at them. :) Or, they won't notice because they're not using a screen reader, and will never find out that these groups have names. Either way, it doesn't make sense to default to "You're too stupid to set up your own site," IMO.

jessebeach’s picture

Straight re-roll of #131. No changes made yet. Just providing this so others can continue testing.

jessebeach’s picture

Re: #137, the repetition of the fieldset description. Maybe this was a JavaScript error issue? When I tab over the available buttons, this is what I get in VoiceOver:

 strike button. Press the down arrow key to activate.

Can you retry with the patch in #142?

webchick’s picture

Nope, I can consistently re-produce this in Firefox 24.0, even on simplytest.me.

falcon03’s picture

@webchick, I've not tested this patch at all, unfortunately. However, I have to say that we should enforce site builders to make their websites as accessible as it can be in any possible way. And of course enforcing users to give a name to the CKEditor toolbar's groups is definitely something to do. Blind users could of course report the issue (about missing toolbar groups) to site builders and they could fix it... But isn't producing accessible content in the scope of a CMS?

So, the "You're too stupid to configure your website" concept is really appropriate regarding accessibility. Unfortunately, reality shows that none cares about accessibility if 1) They know the consequences of producing content that is not accessible to people with disabilities or 2) They are enforced to do that or 3) the platform they're using do the job silently for them.

jessebeach’s picture

I don't think we can fix the VoiceOver/Firefox issues. Firefox just simply doesn't integrate well with VoiceOver in Macs:

http://accessgarage.wordpress.com/2008/08/21/firefox-and-os-xs-voiceover...

webchick’s picture

falcon03: Sorry, I didn't mean that we should shove accessibility under the rug. What I did mean is that "New group" has both visual and aural affordances (it's called "New group" as opposed to "Formatting" or "Linking" or something sensible, and it's italicized/emphasized unlike the others) to denote "Hey, bozo. You should change this from the default value." So we should just trust that people will do that, and not create a very jarring visual user experience with the entire screen suddenly blacking out expect for this small modal box asking for a name as soon as you put something in the group in order to "babysit" site builders. We don't do this sort of babysitting anywhere else, so I'm not sure why we'd do it here.

Bummer about FF and VoiceOver. :( I was really hoping that would just be a quick markup change. I guess if the "hit enter on modal and lose all of your changes" issue is generalized to all modals and not just this one, we can probably do a few more small tweaks and then call this good.

jessebeach’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
113.1 KB
45.76 KB

Third thing: "Enter "Whatever" and hit ENTER."
I bet that's a Firefox-specific bug because I've never seen that happen. I'm hopeful this will be a trivial fix for Jesse.

Derp derp. This was an easy fix. I was missing a reference to event in the signature of the function that prevents the form from submitting when the enter key is pressed. One line fix, illustrated below.

// Prevent the form from submitting.
$widget.on('keydown keypress', function (event) { // event added to the function signature
  if (event.keyCode === 13) {
    return false;
  }
});
I agree with this, too. Jesse stated it's a compromise in the design for both visual and blind users, which is fair enough for initial commit, I think. But let's make a "normal" follow-up for it to see if we can brainstorm something even better later.

Putting on my UX hat for a moment, the always-there new row suggests to a user that creating a new row is possible and that to do it, you simply drag a button into the new group. We're providing an affordance. This is a configuration screen and we want to make the configuration actions salient.

But, I've developed a compromise here that I hope works. The extra row and placeholder groups are only visible once the user starts configuring the active toolbar. Hiding the group names also hides the extra row.

Screenshot of the CKEditor toolbar configuration fieldset. The active toolbar does not present placeholder toolbar groups or a new row. It looks like it would to an end user

What I did mean is that "New group" has both visual and aural affordances (it's called "New group" as opposed to "Formatting" or "Linking" or something sensible, and it's italicized/emphasized unlike the others) to denote "Hey, bozo. You should change this from the default value." So we should just trust that people will do that, and not create a very jarring visual user experience with the entire screen suddenly blacking out expect for this small modal box asking for a name as soon as you put something in the group in order to "babysit" site builders. We don't do this sort of babysitting anywhere else, so I'm not sure why we'd do it here.

I am inclined to leave the interaction alone. We don't want users to create a new group and leave the label as "New group". They can cancel out of the modal and not have any change to their configuration occur. Changing it allows for bad accessibility support and turns a one-click configuration action into a two-click configuration action.

Regarding mgiffords comment about aria-labelledby, this attribute is already in place.

We could also use the aria-labelledby attribute as described here: https://developer.mozilla.org/en-US/docs/Accessibility/ARIA/ARIA_Techniq...

***************

That's all of the commentary so far addressed in this patch.

Test this patch on simplytest.me

Status: Needs review » Needs work

The last submitted patch, ckeditor-configuration-a11y-1872206-148.patch, failed testing.

Bojhan’s picture

I wholeheartedly support Angie's position on this, although we should where possible support workflows that focus on making things more and more accessible the affordances here are enough to trigger that state, there is no need to give a "label this dude!" kind of message.

Being considerate also means, not being jarring - and its more likely people will care for a11y if the system as a whole is considerate and informative about choices, rather than when it tries to force you into certain decisions.

falcon03’s picture

@webchick: well, once again we have different point of view on users-babysitting! :-)

However, in this case I don't really "see" the side effect of the interaction. I admit that I've not tested the patch (I'm attending a CS degree course at university and I'm busy for most of the day having lessons and studying). Anyways, I think Jesse is right: if a site builder does not change the name of the group (what does tell us that the user will change it, since that name is in bold?), we could easily end up in a situation that is worst than the one we have today without this patch.

falcon03’s picture

@Bojhan: just wondering, since this is a 100+k patch, can we discuss uix/usability in a follow up (even critical, if needed)? I know that each issue should be "self-contained" at this point in the release cycle; however, the removal of the enable/disable module functionality used this pattern as well.

webchick’s picture

Jesse and I discussed this and think we landed on something that will make both UX-ish folks and a11y-ish folks happy. Simply change the "drop zone" for each "New group" area to a button that says "New group." When clicking a button, you're deliberately initiating an action, so it's not jarring at all to then get a pop-up then asking for more details. This will then require site builders to enter something meaningful, which solves the a11y issue, without introducing new UX patterns that we don't use anywhere else.

webchick’s picture

Also, big +1 for:

"But, I've developed a compromise here that I hope works. The extra row and placeholder groups are only visible once the user starts configuring the active toolbar. Hiding the group names also hides the extra row."

That seems great! :) Then we keep the discoverability, but only expose it in context.

jessebeach’s picture

I roughed out the "Add group" button. If this feels like a good solution, then I'll clean up the patch and get it in for real.

Test on simplytest.me

cosmicdreams’s picture

Also, and I'm not sure this is relevant, but CKEditor itself has some accessibility updates that are noteworthy:
http://ckeditor.com/blog/CKEditor-4.2.2-Released

Specifically:
http://dev.ckeditor.com/ticket/10753
http://dev.ckeditor.com/ticket/10812

and possibly others.

I can't figure out by looking at it what version of 4.2 we're using. Perhaps 4.2.0? If so we have some low hanging fruit that can be fixed by updating.

webchick’s picture

Cool, here's what #155 looks like.

When just loading up the configuration page:

Toolbar is shown, but with none of the groups, because it is only being viewed.

As soon as you either click on one of the buttons in "Available buttons" (or keyboard navigate to one and press an arrow key):

Now, active toolbar is expanded with group names and buttons to add a group.

"Add group" pops open the modal as before.

Yay! That looks great.

Something is wacktastical though (once again in Firefox 24.0, because I really enjoy causing Jesse pain :\) because I can't even type into the modal without it kicking me back to admin/config/content/formats with "The text format Basic HTML has been updated." Presumably that's why it was marked "do-not-test" :)

Anyway, this looks great to me and resolves all of my previous concerns, and I don't believe degrades the a11y in any way relative to the previous patch.

@cosmicdreams: We're currently effectively running a "dev release" of CKEditor 4.3. See #2039163: Update CKEditor library to 4.4 which attempts to move us to a proper release, but is blocked on upstream fixes. But in other words, if those accessibility fixes are in 4.2, we should already have them.

webchick’s picture

Oh, I lie. The wacktastical thing is happening in Chrome too. Yay. ;)

Bojhan’s picture

@webchick That sounds like a fine solution.

The whole group thing might be a little flabbergasting because you don't actually see the labels in your content creation page, can we add a small piece that it is shown to screenreaders? .

Wim Leers’s picture

#156: you can find which version of CKEditor Drupal ships with by looking at ckeditor_library_info(). The 4.2.2 brings minor end-user accessibility improvements/bugfixes, this issue is about allowing the administrator to define a more accessible CKEditor configuration in a more accessible manner. We continuously update to the latest CKEditor. There's an issue right now to update to the CKE 4.3 beta, which includes 4.2.2 changes: #2039163: Update CKEditor library to 4.4. So: no worries, we'll get those improvements, but they are not related in any way to this issue :)


#153/#157: that's a nice conceptual compromise, but the visual compromise is awful, IMHO. Before, everything in the visual/drag-and-drop CKEditor toolbar builder UI was visually consistent, nice, balanced. By adding that "Add group" button (that will be styled differently in every operating system and possibly even every browser), that is now guaranteed to be broken. However, that's a super tiny nitpicky problem, so that can be a future follow-up :)

I agree with falcon03 that in this case, we must force the user to do the right thing, even if that makes the UX slightly less nice. What's most important? To have a nice "text format/editor config UX" or a "great screen reader UX"? The former affects one person, the latter affects many. Hence I think it's worth making the admin UX slightly less nice, if that's what it takes to guarantee a better screen reader UX.

(UPDATE: That was actually solved by moving to the "Add group" button approach in #157.)

#159: Yes! We should update the "Toolbar configuration" fieldset's description to explain what these groups are for! Good call :)

jessebeach’s picture

Status: Needs work » Needs review
FileSize
115.28 KB
7.25 KB

#159: Yes! We should update the "Toolbar configuration" fieldset's description to explain what these groups are for! Good call :)

I've added a sentence to the fieldset description.

This rounds out the "Add group" button implementation. I've tested in FF, Chrome, and IE. I had some finicky issues with the key press events, but I believe I've gotten them all sorted. This patch should be ready to go now.

Wim Leers’s picture

Code review

  1. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -174,7 +174,7 @@ function theme_ckeditor_settings_toolbar($variables) {
    +  $output .= '<div class="clearfix"><label id="ckeditor-active-toolbar">' . t('Active toolbar') . '</label></div>';
    

    And why this clearfix?

  2. +++ b/core/modules/ckeditor/ckeditor.admin.inc
    @@ -174,7 +174,7 @@ function theme_ckeditor_settings_toolbar($variables) {
       $output .= '<div data-toolbar="active" role="form" class="ckeditor-toolbar ckeditor-toolbar-active clearfix">';
    
    +++ b/core/modules/ckeditor/css/ckeditor.admin.css
    @@ -23,6 +23,10 @@
    +.ckeditor-toolbar-active {
    +  clear: both;
    

    "clear: both" in CSS *and* a "clearfix" class on the element where this class exists on. Accident?

  3. +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -879,7 +881,14 @@ Drupal.ckeditor = {
    +        // Open the group renaming dialog in the next evaluation cycle so that
    +        // this event can be cancelled and the bubbling wiped out. Otherwise,
    +        // Firefox has issues because the page focus is shifted to the dialog
    +        // along with the keydown event.
    

    WOW! Crazy work-around! Stupid Firefox :(

Manual testing

I did manual testing and found two problems:

  1. the ESC key stopped working to close the modal dialog
  2. a big regression in the modal dialog button styling (they're not styled at all anymore): Screen Shot 2013-10-17 at 11.57.48.png
Wim Leers’s picture

Status: Needs review » Needs work

NW, unfortunately :(

jessebeach’s picture

the ESC key stopped working to close the modal dialog

This was broken. I've fixed it. The introduction of the add group buttons changed the way that key presses are handled slightly. In fact, it's made the key press handling a little easier, which is nice.

And why this clearfix?

We need this wrapper in order to have something to push the active configuration toolbar top margin against in IE. The buttons in IE are enormous; they span over a single line height and when floated, they overlap the content below them. womp womp.

Screenshot of IE 10 showing the Show group names button well spaced above the active CKEditor toolbar configuration

"clear: both" in CSS *and* a "clearfix" class on the element where this class exists on. Accident?

Yes, copy-paste accident from the roughing out of the change. I've removed the

clear:both</both>

<blockquote>a big regression in the modal dialog button styling (they're not styled at all anymore):</blockquote>

I had to add the <code>button

and button-primary class to the buttons so that they hook into the theme styling.

Wim Leers’s picture

Status: Needs review » Needs work
  • All problems I found in #162 are fixed.
  • Interdiff looks good … mostly.
  • I would RTBC… except that this change now results in the jQuery UI dialog elements lingering in the DOM, instead of being cleaned up:
    +++ b/core/modules/ckeditor/js/ckeditor.admin.js
    @@ -1404,11 +1396,6 @@ function openGroupNameDialog (view, $group, callback) {
    -    close: function (event) {
    -      // Automatically destroy the DOM element that was used for the dialog.
    -      $(event.target).remove();
    

    (That's why I said "mostly".)

Once that's restored, this is RTBC.

Bojhan’s picture

Can we apply the button styling used for the other buttons there? It's a bit strange there is a sudden hard corner one.

Wim Leers’s picture

#166: I hope Jesse understands what you mean, but I have no idea. Providing sufficient context in issue comments is better, and preferably too much than too little.

Bojhan’s picture

I am referring to the new style that was added for the "Show Group names" this is not matching any styling I have seen elsewhere. At the very least the roundness should match that of the CKEditor buttons that are close to it.

Wim Leers’s picture

#168: that's no style, that's default browser style. That's precisely the concern I raised in #160. Improving that can be a follow-up.

jessebeach’s picture

I tried applying the standard button styling. I'm a little worried that the contrast is insufficient. Here's a comparison of the browser styling versus the standard button styling.

Screenshot of the CKEditor configuration toolbar. One Add group button has the default browser visual styling. One has the Seven theme button visual styling.

I would really like to avoid going down a path of inventing a new button style here. As soon as we change anything about a button's styling (the corner rounding for example) then ALL of the default styling is lost. We would then need to style everything about the button. I know, it's annoying :/

Wim, I added the close function back. I mistakenly assumed that we don't want to remove the dialog from the DOM in this way because the Dialog spec specifies that a dialog should persist, yada yada yada. Anyways, the function is back in.

Bojhan’s picture

Why don't we just fix the style now? Followups like that tend to take foreeever.

jessebeach’s picture

Status: Needs work » Needs review

Go bot go!

jessebeach’s picture

Bojhan, the question is, what should the style be?

Bojhan’s picture

@Jesse I would just go with the way CKEditor styles its buttons. Given that most of the buttons in this area uses buttons with a different kind of styling than core (and/or the style guide), it makes sense to stay consistent to that.

jessebeach’s picture

I turned the "Show group names" toggle into a link.

I reused the CKEditor button styling for the "Add group" buttons. What do we think?

Style change in IE9.

Windows_7_64__Running_-2.png

Style change in Chrome.

Basic_HTML___Drupal_D8_Dev.png

mgifford’s picture

Status: Needs review » Needs work

I just applied #175 and when testing it with voice over http://sc4de5d91fa3c73f.s3.simplytest.me/node#overlay=admin/config/conte... in Safari (with VoiceOver).

Pressing the down arrow on the image didn't activate it. It just skipped down to the Image Vertical tab below the CKEditor configuration.

I tried it without overlay too and it also didn't work.

jessebeach’s picture

Pressing the down arrow on the image

What image specifically? The "Insert Image" CKEditor button.

You do bring up that Safari doesn't want to tab to a tags by default. #headmeetdesk

I'm fixing this. Hopefully it resolves your issue as well.

jessebeach’s picture

mgifford, in order to tab to link elements in Safari, you must toggle a preference checkbox in the Safari preferences:

Safari preferences Advanced pane. The Press tab to highlight each item on a webpage checkbox is now checked.

mgifford’s picture

Jesse, I do appreciate the screen capture with "Press Tab to highlight each item on a webpage" and indeed it was selected. Nice to have that reminder and confirm.

The problem I'm seeing isn't happening on Firefox, Chrome, ChromeVox, Chrome without VoiceOver or Safari without VoiceOver. It's specifically broken when Safari or Chrome is used in conjunction with VoiceOver.

Firefox & VoiceOver are broken and may not be fixed any time soon. Opera's got problems with tabbing over the buttons, but that's outside of the use case we're aiming for.

However, JAWS & VoiceOver are two of the screenreaders that we should be testing with. I just do testing with VoiceOver.

I just loaded your most recent patch up with SimplyTest.me and tested it again.

I think that Down/Up arrows with VoiceOver are reading next/previous line and overruling your commands.

That's the best I can get from:
http://accessibility.psu.edu/voiceover
https://www.apple.com/voiceover/info/guide/_1131.html

jessebeach’s picture

I think that Down/Up arrows with VoiceOver are reading next/previous line and overruling your commands.

I'm not getting this behavior with either Chrome or Safari using VoiceOver. Maybe you've got custom bindings to the up/down keys? I can use the up/down keys to move the buttons around. Here is a video.

http://www.youtube.com/watch?v=-_w-GEjNep0&feature=youtube_gdata_player

mgifford’s picture

I think I was reporting a problem with my VoiceOver. I haven't tracked down what the problem is (despite all of Jesse's support). However, I was able to verify that it worked on another Mac. So not sure how to track down that problem, but it no longer presents an issue for getting it into Core.

webchick’s picture

Is that an RTBC? :)

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Yes.. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, great. I did some more testing today in VoiceOver and I can no longer figure out how to break this. :)

Committed and pushed to 8.x. YEAAAAHH!! Awesome work, all! This is kick-ass.

Wim Leers’s picture

Issue tags: -sprint

YAY! :D

Go Jesse!

mgifford’s picture

That's really amazing.. Great that we've resolved this difficult issue!

mgifford’s picture

Issue summary: View changes

Removed note about needing to apply a patch as a prerequisite—that patch is in core, yay!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

hadi_gsf’s picture

Hello

I'm a new user of drupal and I use a ScreenReader software (JAWS) with windows to create my site with drupal.
Can i apply this patch to drupal 7, or is there any other solutions for drupal 7? or I just have to wait for years untill drupal 8 gets released.
I'm just about to work with drupal, and lots of people advise against running drupal 8 alpha as a new user.
I really want to remove some unused toolbar buttons from my editor so it'd be an easy job to navigate in my editor environment.
Thanks.

nod_’s picture

Hi,

The code is widely different for Drupal7, there is no backport for this planned (that I know of).

I don't think we have a good solution for JAWS on D7 right now.

mgifford’s picture

Hey @hadi_gsf - CKEditor isn't going to be brought back into D7. However, CKEditor is it's own project and you can get involved in the issue queue there: https://www.drupal.org/project/ckeditor

I am not sure why you'd be warned against using Drupal 8 as a new user. As someone trying to learn about Drupal, this is a great place to start. It's not a great place to start if you are looking to launch a site this year.

Would be great to have you involved as a JAWS user.

hadi_gsf’s picture

Thanks guys for your replies. I see that d8 is getting lots of new accessibility changes, so i might wait for its release time then build my desired website off it..

mgifford’s picture

Issue tags: +Drupal.announce