Problem/Motivation
We had a huuuuuuuuuuuuuuge discussion on accessibility of the inline editing capability from Spark at the Toronto Accessibility sprint. Here's the outcome of that:
In general, when approaching accessibility, there are two personas you're concerned with:
1) Keyboard-only users, who can see what's going on but don't have a pointing device.
2) Screenreader users, who are keyboard-only users, but without the benefit of being able to see what's going on.
So we need to think of this in terms of what both of those audiences would want to do, without stabbing themselves in the face 500 times. :) So let's step through the flow. I've *bolded the things that need to change, and/or need special accessibility attention.
- Browsing through the site, arriving at a page, spotting a typo (nothing special accessibility-wise).
- Tab through to the "Edit" toolbar item at the top (we're assuming here that we're moving to Jesse's toolbar design rather than how Edit module works right now) and hit Enter.
- This expands the tray, which should show a *Quick edit mode toggle (which should become a "button" type with Aria style of "pressed" when it is toggled on ... we should remove "View" here because then the difference between Edit module's "View" and Node module's "View" is unclear and confusing). Hit Enter to turn it on.
- This overlays a white sheen as it does now, and outlines the things on the page that are editable. *We need to style these differently than they are currently, e.g. a dashed outline. Right now they look like all elements have focus and they do not. This is confusing.
- *Tabbing from this point only tabs between editables (and then eventually back to the toolbar + quick edit toggle). So you should not be able to tab around to links inside the node body, for example, or on non-editable sidebars, when "Quick edit mode" is on.
- *As I tab through in a screenreader, I'll hear something like: "Title field. Editable. Press Enter to edit." (TODO: How to implement this?)
- If I hit Enter, the window opens up with the contents of whatever in it. Here there are three different behaviours, depending on the type of field:
- For a "show me a widget" field, *put the cursor on the first focusable element. Currently, this is the Save button (well, currently, it's the Label but *the Label should not be focusable, but probably eventually this will be the Cancel button when we move Save to the top (?). This is important so that screen reader users can discover that Save/Cancel exists.
- For a "content editable" field (plain textfield), or a WYSIWYG field, the cursor should be positioned in just before the first character of the text. However, if you click into the field with a pointing device, it should continue to position the cursor directly where you clicked, as it does now.
- If I shift-tab from the place I'm at now, *the dialog closes, and I'm back to my pre-existing editable having focus. A screen reader user should hear something like *Title field dialog closed. as this happens. This is equivalent to clicking somewhere off the window for a pointing device.
- Similarly, if I tab through all possible fields, without changing any of the field values, *the dialog closes, and the next editable (e.g. Body) now has focus. Once again, a screen reader user hears something like *Title field dialog closed. as this happens. This is equivalent to clicking somewhere off the window for a pointing device.
- If I tab through into one of the fields on the window and edit its contents, and then tab out (either by shift+tabbing several times or tab-tabbing several times) I will get a modal dialog that warns me about unsaved changes, and either a "Discard changes" or "Save" option (same as it works now — assuming the modal is accessible, this is fine accessibility-wise). *We'll have to revisit this if/when we move to a single save button + draft revisions option.
- For just editing, you tab through, change values, tab through, change values. To save, you either tab once more and invoke the modal dialog that warns you about dropping your changes, OR you shift+tab to go back up to the save button and click it. I noted that this seems a bit odd to rely on the "exception" behaviour for inline editing, and questioned whether we should repeat the save/cancel buttons at the bottom again, but Everett said this is the kind of detail we should test later on once it's committed and tweak from there.
A few technical challenges:
- ARIA has roles like "has pop-up" but not like "is editable." We'll need to figure out a way to explain to people what's happening.
- Need to be able to make things that are normally focusable non-focusable so you can tab between editables.We'll have to leave those elements there with the presentation role, but strip out all of their children dynamically.
- Everett also said that since Drupal 7, where we had to be careful about using more advanced accessibility features such as ARIA roles, but people will have had 3-4 years to upgrade their technology. So in the same way we don't support IE 6 and 7 in D8, we can say we don't support screen readers that don't support ARIA roles, as long as we're dealing with "advanced" editing like this, and not the basic "Edit tab on the Node page" editing.
Proposed resolution
We'll address this issue in three ways:
- Make the activation and deactivation of edit mode a one-click operation.
- Provide appropriate feedback through auditory means to the user so that the state of the page is expressed sufficiently.
- Create a logical tabbing structure in edit mode so that non-sited and keyboard-only users aren't traversing through unrelated parts of the page.
Remaining tasks
#1851046: Edit mode should be activated when clicking the Edit toolbar tab
#1851092: When entering edit mode, inform users through the aria-live aural update interface about the state of page
#1851074: Isolate tabbable fields/links to editable fields and Edit module control components when edit mode is enabled
#1851144: Use <button> for elements that are buttons
User interface changes
TBD.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#86 | make_in_place_editing-1844220-86.patch | 13.8 KB | nlisgo |
#80 | interdiff-1844220-71-79.txt | 2.37 KB | nlisgo |
#80 | make_in_place_editing-1844220-79.patch | 13.82 KB | nlisgo |
#71 | make_in_place_editing-1844220-71.patch | 14.15 KB | nlisgo |
#71 | interdiff-1844220-68-71.txt | 8.82 KB | nlisgo |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commented+1
Comment #1.0
Everett Zufelt CreditAttribution: Everett Zufelt commentedx
Comment #2
Wim LeersWow, this is awesome and extremely helpful! Thank you! :)
It'll definitely be an interesting challenge to make all of this work. At the same time, we'll gain a lot of very useful accessibility experience, so that we can make it more accessible from the start in the future :)
Comment #3
Wim LeersI'd *love* to jump in and start making the first dent in making Edit fully accessible (the very first tiny dent was made back at DrupalCon Munich, by setting
role="button"
where necessary). I do fully understand how it should work from a screenreader user point of view. I do not understand at all yet how it should work from an implementer's point of view.I'd very much appreciate links to useful references to get started on this. If you say "you'll have to read the entire ARIA spec to be able to productively implement this", that's fine, but I'd like to avoid spending days reading things that won't help me after all.
Below is a point-by-point list to explain my main questions.
And finally, a general question: if I make sure all of this works well in VoiceOver, will this also work in JAWS etc?
Comment #3.0
Wim Leersx
Comment #3.1
jessebeach CreditAttribution: jessebeach commentedadded the issue template
Comment #3.2
jessebeach CreditAttribution: jessebeach commentedadded 1851046, 1851092, 1851074
Comment #4
jessebeach CreditAttribution: jessebeach commentedadded tag a11y
Comment #5
jessebeach CreditAttribution: jessebeach commentedAll remaining tasks in this first pass at creating an aural, keyboard-focused interface for the Edit module have been completed and checked into the 8.x-1.x branch of the Edit module.
You can test the results of this work at the following demo site:
user: tester
password: tester
URL: http://edit.qemistry.us/
Further work on the aural UI will require some refactoring of the Edit module JavaScript. What we would to do now is collect feedback on the current attempt to make this UI accessible. This feedback will form the plan for the second effort after refactoring is completed.
I personally am on a steep learning curve for implementing aural UIs. I've been using ChromeVox for testing and also as a user agent for non-development web browsing. I'm coming to understand the level of effort required to build a solid aural UI; that it's really building another interface to the features of this module. We all want to make this module usable by everyone. Feedback and implementation suggestions or code are truly encouraged.
Comment #6
Bojhan CreditAttribution: Bojhan commentedDrupal standard profile should be accessible, it looks like we are making good progress. Moving it around - so we can get some eyes on this, now that Edit is in core.
Comment #7
Bojhan CreditAttribution: Bojhan commentedComment #8
webchickUnless proven otherwise, I don't believe there are any outstanding a11y problems with inline editing that would qualify as "major". (EDIT: or critical, for that matter. :P) But if I'm incorrect on that count, happy to have this moved back.
Comment #9
mgiffordDoes that link still work? I got "Server not found"
Comment #10
webchickNo, but the good news is you can just download 8.x and install it now. :D
Comment #11
Wim LeersMarking as NR, to hopefully start getting some much-needed a11y reviews.
@mgifford: see what webchick said; if you want to try it quickly, then go to http://simplytest.me/project/drupal/8.x.
Comment #12
mgiffordMuch easier to do now.
So in my local test I couldn't access the WYSIWYG without a mouse. Tabbing just jumps right over the CKEditor elements.
Like discussions with BU Editor earlier if there is a way to access it, it needs to be discoverable.
http://drupal.org/node/1306760#comment-5107904
Comment #13
Wim Leers#12: That would be an a11y bug in Edit, not in the WYSIWYG editor (i.e. CKEditor) :) Assigning to jessebeach because she's the best person to answer that. I'll make sure we coordinate with the CKEditor developers because this might lead us to tricky ARIA territory.
Also tagging accordingly.
Comment #14
Wim Leers.
Comment #15
Wim LeersPoints 1–4 have been implemented already.
Points 5–9 are now possible to implement thanks to #1913086: Generalize the overlay tabbing management into a utility library. This should only be implemented as soon as #1979784: Factor Create.js and VIE.js out of the Edit module is completed.
Points 10 & 11 are being addressed at #1678002: Edit should provide a usable entity-level toolbar for saving fields.
Comment #16
Liam MorlandTags
Comment #17
jessebeach CreditAttribution: jessebeach commentedOk, This is back in active development. #1678002: Edit should provide a usable entity-level toolbar for saving fields is now matured enough to build on.
Comment #18
Wim LeersFor CKEditor a11y issues, see #1879430: [meta] Find and report any accessibility issue of CKEditor and its Drupal integration now.
Comment #19
mgiffordadding atag tag.
Comment #20
webchickThis seems at least major.
Comment #21
jessebeach CreditAttribution: jessebeach commentedI tried to cover all the major UX components in this patch: keyboard navigation, aural announcements for state changes, correct aria role and state markup, and tabbing management for the various stages of in place editing.
Rather than take time to explain in words, I made a little video that illustrates the interaction: http://www.youtube.com/watch?v=0lcQj1ht6cA&feature=youtube_gdata_player
I still have to mark up and announce validation errors. That will land in an upcoming patch tomorrow.
I would love for folks who use screen readers to test this patch and let me know what works and what doesn't work for you. I'll post a link to a simplytest.me build that you can use to quickly test the feature.
Comment #22
jessebeach CreditAttribution: jessebeach commentedTest the patch in #21 on simplytest.me.
Comment #23
mgiffordSTM seems to be down at the moment. Will try to test this later.
Comment #24
rgchi CreditAttribution: rgchi commentedHas anyone been able to test out with Jaws? If not, I can jump in.
Comment #25
jessebeach CreditAttribution: jessebeach commented@richardgoodrow no one has tested with Jaws. I would very much appreciate your input.
Comment #26
mgiffordDid a short test in VoiceOver on STM and it seemed to work fine.
Comment #27
Wim LeersAccessibility signed off: great!
Now let's look at the code. Code review below; a whole bunch of nitpicky things I've fixed in the attached reroll.
New parameter, but no new docs.
This means that some of the elements that we're passing in are not in fact tabbable (because they are filtered away by
:tabbable
), yet we want those to be tabbable anyway, right?Which elements are these?
This means that some of the elements that we're passing in are not in fact tabbable (because they are filtered away by
:tabbable
), yet we want those to be tabbable anyway, right?Which elements are these?
This means that some of the elements that we're passing in are not in fact tabbable (because they are filtered away by
:tabbable
), yet we want those to be tabbable anyway, right?Which elements are these?
Good catch :) That was indeed unnecessary!
This can go away I think?
Why do we have these if we have the tabbing manager?
I think this accidentally snuck in?
That's a strange string to translate, I made this a little bit nicer.
Still to be implemented before this can be RTBC.
Comment #28
jessebeach CreditAttribution: jessebeach commentedAwesome fixes Wim! mgifford and I are going to do a live co-review of the patch this Friday.
Comment #29
Wim Leers#28: Just for clarity, most of the things I mentioned in the code review in #27 I did not yet fix in my reroll! The reroll only changed things I was certain of.
Comment #30
jessebeach CreditAttribution: jessebeach commentedRerolled and addressed the switch to the new Drupal modal. Working on the items from Wim's feedback that were not incorporated into the issue yet.
Comment #31
jessebeach CreditAttribution: jessebeach commentedHere is a link to launch a simplytest.me instance.
We're looking to determine if the in-place editing support for keyboard and aural access is sufficient. Evaluation questions include:
Comment #32
mgiffordShould also add here that to test this a user needs to create a new article or page and after saving the page, click on the "Quick edit" button to test the in-place editing functionality.
Comment #33
mgiffordJust trying to give a bit more context
Comment #34
jessebeach CreditAttribution: jessebeach commentedMike and I found during a co-evaluation that tabbing in Firefox is broken. We weren't able to tab from a field to the entity toolbar to save changes to the field. I'm looking into this.
Also it's difficult to discover the toolbar non-visually. I'm going to have the entity toolbar be the thing that is tabbed to after exiting a recently changed field, so that next action will be to save after editing a field.
Comment #35
Wim LeersStraight reroll of #30, because the patch didn't apply anymore.
Comment #35.0
Wim Leersadded 1851144
Comment #36
jessebeach CreditAttribution: jessebeach commentedReroll to keep up with HEAD.
Also, tabbing between field elements no longer worked, so I fixed that.
Comment #37
Crell CreditAttribution: Crell commentedI cannot yet speak to the Javascript code itself. However, I just installed this patch and was able to tab through all of visible fields on a node successfully with a mix of tab and enter. This is in Chrome.
In Firefox, I was able to tab between fields and select the save button to save the form. The only issue I had is that the cursor keys in the WYSIWYG didn't seem to work. That seems unrelated, though.
I'm not RTBCing because I don't know how to test the aural parts and I'm not qualified to judge the Javascript code itself. If someone can advise I can look into it.
Comment #38
Crell CreditAttribution: Crell commentedFurther testing has found other issues:
When using a multi-value longtext field, when that field is in "edit" mode, the "add another item" button does not work. Tabbing to it and selecting it or clicking the button with the mouse produces a momentary "working" icon, but no new field is added.
Additionally, it's unclear if the desired behavior is for the tabbing to cycle through only the editable fields. It is still tabbing through all links on the page, the browser location bar, etc. I don't know if that's by design or not. (In the use case we have for D7 it's probably not desirable, but I don't know from an accessibility standpoint what is "correct" here.)
Comment #39
Wim Leers#38: if there's a "working" icon, then there's an AJAX request. What is the response you're getting? That definitely used to work.
Comment #40
Crell CreditAttribution: Crell commentedajax.js line 551 is throwing a 404 error on path /system/ajax
Interesting.
Comment #41
jessebeach CreditAttribution: jessebeach commentedI reproduced the error. 400, bad request. Looking into it.
Comment #42
jessebeach CreditAttribution: jessebeach commentedThis error is happening on HEAD (8b0cb059), it's unrelated to this issue.
Comment #43
jessebeach CreditAttribution: jessebeach commentedFrom WatchDog:
Comment #44
jessebeach CreditAttribution: jessebeach commentedI can get my debugger to catch right inside
\Drupal\system\FormAjaxController::content()
when requesting another instance of the long text field, but it only catches for a moment, then the request crumbles and returns an error. I'm still trying to determine what the actual issue is, but this type of failure is right on the edge of my debugging skills. I don't have a way to dive deeper into what's going on. Maybe we need to look at the stack trace?Comment #45
Wim LeersI think this may be related to #2177289: In-place editing of image fields is broken, which itself is caused by AJAX forms being somehow broken.
Comment #46
mgiffordThe patch in #36 no longer applies and seems to need a serious re-roll.
Comment #47
xjmComment #48
mgiffordComment #49
mgiffordComment #50
kpv CreditAttribution: kpv commentedComment #51
kpv CreditAttribution: kpv commentedmoved dependencies into quickedit.libraries.yml and made some renames from "edit" to "quickedit"
Comment #52
mgiffordengaging the bot...
Comment #54
mgiffordI tested this patch (and Core without it) with just my keyboard and it didn't work in Firefox or Chrome on my mac.
I can navigate to the Quick edit link easily enough, but I can't seem to navigate into the title to make or edit any changes. Same with the body.
Comment #55
kpv CreditAttribution: kpv commentedComment #56
star-szrComment #57
mgiffordIt needs help to be keyboard accessible still.
Comment #59
mgiffordComment #61
pfrenssenRerolled the patch against HEAD as part of a D8 contribution training.
Comment #62
mgiffordPatch applies. I can navigate into the title & body. I can also save just using TAB & Shift-TAB.
It's a good improvement over HEAD for keyboard only users.
Comment #64
nod_Eslint errors:
I would call it "comprehensive" or something similar (just reversing the meaning of "expand"). And it makes the code simpler, the if can just be
if (!comprehensive) {}
and it's clearer.Change the name inside the if, better to not override "owns".
I would use a regex to split, in case multiple space end up in the attribute:
I'd rather use jquery (for consistency with the rest of core)
$formContainer.trigger('focus');
Does this work? from what I see the output of the whole split/filter is never used.
Same thing as before, split on regex, assign a new variable and for consistency sake do the join() in the attr().
Don't need jquery here, can just send the selector string, tabbingmanager takes care of jQuerying it.
Put the selector string in a variable too, this line is too long.
Lots going on, either create intermediate variables to make that saner of add new lines all over the place. (I'd prefer new variables).
Need empty new line before the closure.
Indentation is wrong inside the closure.
in this case, the
fn
variable is not needed.Comment #65
mgiffordComment #66
nlisgo CreditAttribution: nlisgo commentedGoing to address feedback in #64.
Comment #67
nlisgo CreditAttribution: nlisgo commentedWill need a re-roll first.
Comment #68
nlisgo CreditAttribution: nlisgo commentedHere is the re-roll. I will prepare the patch which incorporates the feedback from #64 shortly.
Comment #69
nlisgo CreditAttribution: nlisgo commentedJust marking as needs review to trigger testbot. I will switch back to needs work regardless of the outcome of the test run.
Comment #70
nlisgo CreditAttribution: nlisgo commentedComment #71
nlisgo CreditAttribution: nlisgo commentedI've addressed all feedback from #64 except:
the errors variable is unused but the todo underneath indicates that it should be used:
Comment #72
nlisgo CreditAttribution: nlisgo commentedGoing to unassign for now and I will await output of testbot and any feedback on the patch.
Comment #73
nlisgo CreditAttribution: nlisgo commentedWe need to announce the validation errors.
Comment #74
mgiffordNice. Took me a bit to get used to, but it's so much better than what is in core right now.
So you're suggesting adding Drupal.announce to
announceValidationErrors: function ()
along with some code to setaria-invalid=true
Comment #75
nlisgo CreditAttribution: nlisgo commentedWhen @jessebeach supplied the first patch a couple of years ago in #21 she made the following comment:
This has yet to materialise. We just have a @todo in the patch for someone to pick this up. I may be able to help with this but would need to understand what the expectation is. I could dig through the WAI docs but can someone with the knowledge and experience weigh in to describe the expected behaviour when there are validation errors.
Comment #76
webchickJesse (she) is no longer really involved with Drupal, so I'd suggest not waiting on her for a patch. :)
Comment #77
nlisgo CreditAttribution: nlisgo commentedI'm not suggesting that we wait for Jesse (her) specifically to weigh in but I do wonder whether there are those following this issue that have a clear idea of the way forward and I would be happy to help out with the implementation. I only referred to @jessebeach's message in #21 as it is the source of the @todo that needs to be addressed before the patch can be considered for review.
Unless we do not need to announce the validation errors to screen readers?! :)
This is a genuine question. Please can someone confirm that this is a todo that needs to be addressed.
Comment #78
jessebeach CreditAttribution: jessebeach commentedNever block an immediate improvement on the whispered hope of a future improvement :)
We can improve the validation in a future task. Onward and upward!
Comment #79
nlisgo CreditAttribution: nlisgo commentedIgnore the patch above. I uploaded the wrong one for this issue. See #80 instead.
Comment #80
nlisgo CreditAttribution: nlisgo commentedI uploaded entirely the wrong patch. #hangsheadinshame.
Thanks for weighing in @jessebeach. I have a patch that removes the announceValidationErrors method and does some other tidy up to make it more consistent with other similar js files.
Comment #81
nlisgo CreditAttribution: nlisgo commentedComment #85
nlisgo CreditAttribution: nlisgo commentedComment #86
nlisgo CreditAttribution: nlisgo commentedComment #87
mgiffordI had a lot of trouble editing with this patch. I couldn't use quick edit to modify the body field, although I was able to use it to modify the title.
I can edit the title with just using the keyboard, but I can't save it.
Comment #98
SpokjeDue to Quickedit being moved out of Drupal Core and into a Contrib Module, moving this issue to the Contrib Module queue.