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.

  1. Browsing through the site, arriving at a page, spotting a typo (nothing special accessibility-wise).
  2. 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.
  3. 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.
  4. 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.
  5. *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.
  6. *As I tab through in a screenreader, I'll hear something like: "Title field. Editable. Press Enter to edit." (TODO: How to implement this?)
  7. 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.
  8. 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.
  9. 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.
  10. 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.
  11. 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:

  1. Make the activation and deactivation of edit mode a one-click operation.
  2. Provide appropriate feedback through auditory means to the user so that the state of the page is expressed sufficiently.
  3. 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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

+1

Everett Zufelt’s picture

Project: Edit » Drupal core
Issue summary: View changes

x

Wim Leers’s picture

Project: Drupal core » Edit

Wow, 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 :)

Wim Leers’s picture

I'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.

  • Point 2: will be provided by #1137920: Fix toolbar on small screen sizes and redesign toolbar for desktop, and is blocked on that being committed.
  • Points 3 & 4: @tkoleary is designing this.
  • Points 5 & 6: I only have an extremely vague idea of how to implement this. Tabindexes come to mind, but more useful references would be most welcome.
  • Point 7: for the "form" editor, Edit should give focus to the "Save" button, but for the "direct" editors (contentEditable or WYSIWYG), Edit should give focus to the text, specifically it should put the cursor before the first word. This distinction makes little sense to me, so please help me understand why this is necessary. Why shouldn't we either always give focus to the "Save" button or always give focus to first editable thingie, whether that's a form field or a piece of text?
  • Points 8 & 9: this ties back to my remark for points 5 & 6. Except that this sounds even more complex: we must make sure that the screen reader speaks a message that is invisible to non-screenreader users. How would that work?
  • Points 10 & 11: no questions for now.

And finally, a general question: if I make sure all of this works well in VoiceOver, will this also work in JAWS etc?

Wim Leers’s picture

Project: Edit » Drupal core
Issue summary: View changes

x

jessebeach’s picture

Issue summary: View changes

added the issue template

jessebeach’s picture

Issue summary: View changes

added 1851046, 1851092, 1851074

jessebeach’s picture

Project: Drupal core » Edit
Issue tags: +a11y

added tag a11y

jessebeach’s picture

All 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.

Bojhan’s picture

Project: Edit » Drupal core
Version: 8.x-1.x-dev » 8.x-dev
Component: Accessibility » edit.module
Priority: Critical » Major
Issue tags: +Accessibility

Drupal 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.

Bojhan’s picture

Priority: Major » Critical
webchick’s picture

Priority: Critical » Normal

Unless 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.

mgifford’s picture

Does that link still work? I got "Server not found"

webchick’s picture

No, but the good news is you can just download 8.x and install it now. :D

Wim Leers’s picture

Status: Active » Needs review

Marking 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.

mgifford’s picture

Much 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

Wim Leers’s picture

Assigned: Unassigned » jessebeach
Issue tags: +CKEditor in core

#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.

Wim Leers’s picture

Issue tags: +Spark

.

Wim Leers’s picture

Status: Needs review » Postponed
Issue tags: +sprint

Points 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.

Liam Morland’s picture

Issue tags: -a11y

Tags

jessebeach’s picture

Title: Accessibility plan for Edit module » Make in-place editing keyboard or aurally accessible
Status: Postponed » Active
Issue tags: +a11y

Ok, 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.

Wim Leers’s picture

mgifford’s picture

Issue tags: +atag

adding atag tag.

webchick’s picture

Priority: Normal » Major

This seems at least major.

jessebeach’s picture

Title: Make in-place editing keyboard or aurally accessible » Make in-place editing keyboard and aurally accessible
Status: Active » Needs review
FileSize
25.59 KB

I 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.

jessebeach’s picture

Test the patch in #21 on simplytest.me.

mgifford’s picture

STM seems to be down at the moment. Will try to test this later.

rgchi’s picture

Has anyone been able to test out with Jaws? If not, I can jump in.

jessebeach’s picture

@richardgoodrow no one has tested with Jaws. I would very much appreciate your input.

mgifford’s picture

Did a short test in VoiceOver on STM and it seemed to work fine.

Wim Leers’s picture

FileSize
7.29 KB
20.95 KB

Accessibility 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.

  1. +++ b/core/misc/tabbingmanager.js
    @@ -41,7 +41,10 @@ $.extend(TabbingManager.prototype, {
    -  constrain: function (elements) {
    +  constrain: function (elements, expand) {
    

    New parameter, but no new docs.

  2. +++ b/core/misc/tabbingmanager.js
    @@ -51,7 +54,16 @@ $.extend(TabbingManager.prototype, {
    +    var $elements;
    +    // Find tabbable elements within the set of supplied elements if expansion
    +    // is requested.
    +    if (expand) {
    +      $elements = $(elements).find(':tabbable').addBack(':tabbable');
    +    }
    +    // Assume that a list of tabbable elements is provided.
    +    else {
    +      $elements = $(elements);
    +    }
    

    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?

  3. +++ b/core/modules/contextual/contextual.toolbar.js
    @@ -222,10 +222,13 @@ Drupal.contextualToolbar = {
    -        tabbingContext = Drupal.tabbingManager.constrain($('.contextual-toolbar-tab, .contextual'));
    -        this.model.set('tabbingContext', tabbingContext);
    -        this.announceTabbingConstraint();
    -        this.announcedOnce = true;
    +        var $tabbables = $('.contextual');
    +        if ($tabbables.length) {
    +          tabbingContext = Drupal.tabbingManager.constrain($('.contextual-toolbar-tab').add($tabbables));
    +          this.model.set('tabbingContext', tabbingContext);
    +          this.announceTabbingConstraint();
    +          this.announcedOnce = true;
    +        }
    

    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?

  4. +++ b/core/modules/edit/js/editors/formEditor.js
    @@ -63,12 +63,17 @@ Drupal.edit.editors.form = Drupal.edit.EditorView.extend({
    +    var owns = $(this.fieldModel.get('entity').get('el')).attr('aria-owns').split(' ');
    +    owns.push(id);
    +    $(this.fieldModel.get('entity').get('el')).attr('aria-owns', owns.join(' '));
    

    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?

  5. +++ b/core/modules/edit/js/views/AppView.js
    @@ -252,11 +261,16 @@ Drupal.edit.AppView = Backbone.View.extend({
    -      el: $(fieldModel.get('el')),
    +      el: fieldModel.get('el'),
    

    Good catch :) That was indeed unnecessary!

  6. +++ b/core/modules/edit/js/views/EditorDecorationView.js
    @@ -30,6 +31,9 @@ Drupal.edit.EditorDecorationView = Backbone.View.extend({
    +
    +    // Manage keyboard clicks.
    +    //this.$el.on('keypress.edit', this.onKeyPress.bind(this));
    

    This can go away I think?

  7. +++ b/core/modules/edit/js/views/EditorDecorationView.js
    @@ -141,14 +145,28 @@ Drupal.edit.EditorDecorationView = Backbone.View.extend({
    +      .prop({'tabIndex': '0'})
    
    +++ b/core/modules/edit/js/views/EntityToolbarView.js
    @@ -316,12 +316,17 @@ Drupal.edit.EntityToolbarView = Backbone.View.extend({
    +              'tabindex': '0'
    ...
    +              'tabindex': '0',
    

    Why do we have these if we have the tabbing manager?

  8. +++ b/core/modules/edit/js/views/EntityView.js
    @@ -16,15 +16,62 @@ Drupal.edit.EntityView = Backbone.View.extend({
    +    this.$el.off('.edit');
    

    I think this accidentally snuck in?

  9. +++ b/core/modules/edit/js/views/FieldAuralView.js
    @@ -0,0 +1,70 @@
    +    Drupal.announce(Drupal.t('Editing @aria', {'@aria': this.model.get('metadata').aria}), 'assertive');
    

    That's a strange string to translate, I made this a little bit nicer.

  10. +++ b/core/modules/edit/js/views/FieldAuralView.js
    @@ -0,0 +1,70 @@
    +    // @todo, announce the validation errors. And mark them correctly with
    +    // aria-invalid=true
    

    Still to be implemented before this can be RTBC.

jessebeach’s picture

Awesome fixes Wim! mgifford and I are going to do a live co-review of the patch this Friday.

Wim Leers’s picture

#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.

jessebeach’s picture

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

Rerolled 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.

jessebeach’s picture

Here 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:

  1. Are the controls discoverable?
  2. Is the path through the tabbables acceptable?
  3. Can you edit all the fields in an entity with quick-edit and save them or cancel out successfully?
  4. What obstacles do you encounter?
  5. Do you have suggestions for improvement?
mgifford’s picture

Should 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.

mgifford’s picture

Just trying to give a bit more context
What the screen looks like when you're starting a Quick edit of a document.

jessebeach’s picture

Mike 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.

Wim Leers’s picture

Straight reroll of #30, because the patch didn't apply anymore.

Wim Leers’s picture

Issue summary: View changes

added 1851144

jessebeach’s picture

Issue summary: View changes
Issue tags: -sprint
FileSize
3.95 KB
14.31 KB

Reroll to keep up with HEAD.

Also, tabbing between field elements no longer worked, so I fixed that.

Crell’s picture

I 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.

Crell’s picture

Further 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.)

Wim Leers’s picture

#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.

Crell’s picture

ajax.js line 551 is throwing a 404 error on path /system/ajax

Interesting.

jessebeach’s picture

Failed to load resource: the server responded with a status of 400 (Bad Request) http://d8.drupal.dev/system/ajax
Uncaught AjaxError: 
An AJAX HTTP error occurred.
HTTP Result Code: 400
Debugging information follows.
Path: /system/ajax
StatusText: Bad Request
ResponseText: A fatal error occurred:

I reproduced the error. 400, bad request. Looking into it.

jessebeach’s picture

This error is happening on HEAD (8b0cb059), it's unrelated to this issue.

jessebeach’s picture

From WatchDog:

56  21/Jan 23:47  warning  ajax  Invalid form POST data.                                                                             
 57  21/Jan 23:50  notice   php   Notice: unserialize(): Error at offset 223880 of 334725 bytes in                                    
                                  Drupal\Core\KeyValueStore\DatabaseStorageExpirable->getMultiple() (line 70 of                       
                                  /Users/jbeach/code/drupal/core/d8/core/lib/Drupal/Core/KeyValueStore/DatabaseStorageExpirable.php).
jessebeach’s picture

I 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?

Wim Leers’s picture

I 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.

mgifford’s picture

Issue tags: +Needs reroll

The patch in #36 no longer applies and seems to need a serious re-roll.

xjm’s picture

Component: edit.module » quickedit.module
mgifford’s picture

Assigned: jessebeach » Unassigned
Issue tags: +TwinCities
mgifford’s picture

Issue tags: -TwinCities +TCDrupal 2014
kpv’s picture

Assigned: Unassigned » kpv
kpv’s picture

moved dependencies into quickedit.libraries.yml and made some renames from "edit" to "quickedit"

mgifford’s picture

Status: Needs work » Needs review

engaging the bot...

The last submitted patch, 36: edit-a11y-1844220-36.patch, failed testing.

mgifford’s picture

Issue tags: +dcamsa11y

I 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.

kpv’s picture

Assigned: kpv » Unassigned
star-szr’s picture

Issue tags: -Needs reroll
mgifford’s picture

Status: Needs review » Needs work

It needs help to be keyboard accessible still.

Status: Needs work » Needs review
mgifford’s picture

Issue tags: -dcamsa11y +Needs reroll

Status: Needs review » Needs work

The last submitted patch, 51: edit-a11y-1844220-36-rerolled.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.97 KB

Rerolled the patch against HEAD as part of a D8 contribution training.

mgifford’s picture

Patch 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.

nod_’s picture

Status: Needs review » Needs work

Eslint errors:

core/modules/quickedit/js/views/FieldAuralView.js
   7:2  error  Expected indentation of 2 characters  indent
  13:2  error  Expected indentation of 2 characters  indent
  65:8  error  errors is defined but never used      no-unused-vars

✖ 3 problems (3 errors, 0 warnings)
  1. +++ b/core/misc/tabbingmanager.js
    @@ -35,7 +35,10 @@
    +    constrain: function (elements, expand) {
    +      if (typeof expand === 'undefined') {
    +        expand = true;
    +      }
    
    @@ -45,7 +48,16 @@
    +      if (expand) {
    

    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.

  2. +++ b/core/modules/quickedit/js/editors/formEditor.js
    @@ -71,13 +71,22 @@
    +      if (owns) {
    +        owns = owns.split(' ');
    +        owns.push(id);
    +        $entity.attr('aria-owns', owns.join(' '));
    +      }
    

    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:

    var ownsValues = owns.split(/\s+/);
    
  3. +++ b/core/modules/quickedit/js/editors/formEditor.js
    @@ -131,6 +140,9 @@
    +        $formContainer[0].focus();
    

    I'd rather use jquery (for consistency with the rest of core)

    $formContainer.trigger('focus');

  4. +++ b/core/modules/quickedit/js/editors/formEditor.js
    @@ -152,6 +164,19 @@
    +      var owns = $entity.attr('aria-owns');
    +      if (owns) {
    +        owns
    +          .split(' ')
    +          .filter(function (id) {
    +            // Remove id values that start with 'quickedit-form-for-'.
    +            return (id.indexOf('quickedit-form-for-') !== 0);
    +          })
    +          .join(' ');
    +        $entity.attr('aria-owns', owns);
    +      }
    

    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().

  5. +++ b/core/modules/quickedit/js/views/AppView.js
    @@ -86,6 +86,12 @@
    +            app.tabbingContext = Drupal.tabbingManager.constrain($('.quickedit-editable, #quickedit-entity-toolbar .quickedit-toolbar button'), false);
    

    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.

  6. +++ b/core/modules/quickedit/js/views/EntityToolbarView.js
    @@ -426,9 +431,17 @@
    +      this.$el
    +        .find('.quickedit-toolbar-entity [type="submit"]')
    +        .attr('aria-label', Drupal.t('Save changes to @fields', {'@fields': Drupal.formatPlural(changeFieldsCount, '@count field', '@count fields')}));
    

    Lots going on, either create intermediate variables to make that saner of add new lines all over the place. (I'd prefer new variables).

  7. +++ b/core/modules/quickedit/js/views/FieldAuralView.js
    @@ -0,0 +1,72 @@
    + */
    +(function (Backbone, Drupal) {
    +
    +"use strict";
    +
    +/**
    + * Reacts to field model changes by announcing the changes in a way that screen
    + * reading user agents will convey.
    + */
    +Drupal.quickedit.FieldAuralView = Backbone.View.extend({
    

    Need empty new line before the closure.

    Indentation is wrong inside the closure.

  8. +++ b/core/modules/quickedit/js/views/FieldDecorationView.js
    @@ -31,6 +31,9 @@
    +      var fn = this.onKeypress.bind(this);
    +      $(document).on('keypress.edit', fn);
    

    in this case, the fn variable is not needed.

mgifford’s picture

Issue tags: +Drupal.announce
nlisgo’s picture

Assigned: Unassigned » nlisgo

Going to address feedback in #64.

nlisgo’s picture

Issue tags: +Needs reroll

Will need a re-roll first.

nlisgo’s picture

Here is the re-roll. I will prepare the patch which incorporates the feedback from #64 shortly.

nlisgo’s picture

Status: Needs work » Needs review

Just marking as needs review to trigger testbot. I will switch back to needs work regardless of the outcome of the test run.

nlisgo’s picture

Status: Needs review » Needs work
nlisgo’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
14.15 KB

I've addressed all feedback from #64 except:

core/modules/quickedit/js/views/FieldAuralView.js
65:8 error errors is defined but never used no-unused-vars

the errors variable is unused but the todo underneath indicates that it should be used:

// @todo, announce the validation errors. And mark them correctly with
// aria-invalid=true

nlisgo’s picture

Assigned: nlisgo » Unassigned

Going to unassign for now and I will await output of testbot and any feedback on the patch.

nlisgo’s picture

Status: Needs review » Needs work
+++ b/core/modules/quickedit/js/views/FieldAuralView.js
@@ -0,0 +1,73 @@
+    /**
+     * Announces validation error messages to a screen reading user agent.
+     */
+    announceValidationErrors: function () {
+      var errors = this.model.get('validationErrors');
+      // @todo, announce the validation errors. And mark them correctly with
+      // aria-invalid=true
+    }

We need to announce the validation errors.

mgifford’s picture

Nice. 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 set aria-invalid=true

nlisgo’s picture

When @jessebeach supplied the first patch a couple of years ago in #21 she made the following comment:

I still have to mark up and announce validation errors. That will land in an upcoming patch tomorrow.

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.

webchick’s picture

Jesse (she) is no longer really involved with Drupal, so I'd suggest not waiting on her for a patch. :)

nlisgo’s picture

I'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.

jessebeach’s picture

Never block an immediate improvement on the whispered hope of a future improvement :)

We can improve the validation in a future task. Onward and upward!

nlisgo’s picture

Status: Needs work » Needs review
FileSize
13.72 KB

Ignore the patch above. I uploaded the wrong one for this issue. See #80 instead.

nlisgo’s picture

I 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.

nlisgo’s picture

The last submitted patch, 79: 2113323-rename_all_permalink-28.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 80: make_in_place_editing-1844220-79.patch, failed testing.

nlisgo’s picture

Issue tags: +Needs reroll
nlisgo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.8 KB
mgifford’s picture

Status: Needs review » Needs work

I 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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev
Spokje’s picture

Project: Drupal core » Quick Edit
Version: 9.3.x-dev » 1.0.x-dev
Component: quickedit.module » Code

Due to Quickedit being moved out of Drupal Core and into a Contrib Module, moving this issue to the Contrib Module queue.