Problem/Motivation

Steps to reproduce

  1. Apply the attached patch, break-it--break-it-good.patch
  2. Open a node for in-place editing by activating the quick edit link from its contextual links.
  3. Change the content of a field on the node.
  4. Press the save button.
  5. The saving spinner should remain and never dismiss.
  6. Check the browser console to see the printed error message. The error handler is in EntityModel::save.

Proposed resolution

  1. Deal with failed AJAX requests (error handler)
  2. Add a timeout for requests that take too long. Allow the user to continue editing, but tell them that their content may not have been saved.

Remaining tasks

Fix it.

User interface changes

No.

API changes

None.

None.

Original report by jessebeach

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Looks like #77245-43: Provide a common API for displaying JavaScript messages will finally get some consideration :D

How about showing an error message telling which fields failed and show a link to refresh the page?

nod_’s picture

Issue tags: +Needs usability review

The technical issue isn't hard to solve it's more the expected UX we need to know about.

jessebeach’s picture

The status message is necessary to inform the user that an error occurred.

In order to prevent data loss, I'd like to put the in place editing back into a "dirty" state so that the user can attempt to save again. At worst, they can copy-paste the content out of the fields and not lose it.

Bojhan’s picture

It'd be nice to immediately offer ways to resolve the issue. Like "try and press save again" or copy your content refresh and paste it.

Wim Leers’s picture

Note that this is a problem in *all* core JS AFAIK: none of it deals with network failure or 500 responses. We just assume all goes well :)

That being said, here the user is far more directly affected.

Wim Leers’s picture

Issue tags: -sprint

We're not working on this in the current sprint.

yoroy’s picture

Clearing out the 'needs usability review' tag.

yoroy’s picture

Issue summary: View changes

put in a list.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes
Status: Active » Needs review
Issue tags: +sprint
FileSize
60.53 KB
61.93 KB
5.02 KB
1.09 KB

In this patch, if you follow the steps to reproduce in #0, you will get a modal dialog indicating something went wrong, and instead of being unable to try to save again, you can now choose to try again: note how the "Save" button is no longer stuck in the "Saving…" state, but you can click it again:

Analogously, the scenario of in-place editing failing to start because the form could not be loaded shows a similar dialog, and now you can just click on another field (or the same field) — instead of a broken page you can just choose to continue or stop in-place editing:

We could of course go much further than this, but that would require Drupal.ajax to be less crappy, and would require a lot more code. This smooths the roughest edges and removes the biggest frustrations.

The last submitted patch, 8: edit_network_error_handling-2055937-8.patch, failed testing.

Wim Leers’s picture

#8 was rolled on top of some other patches: reroll straight on current HEAD.

Status: Needs review » Needs work

The last submitted patch, 10: edit_network_error_handling-2055937-10.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

The last submitted patch, break-it--break-it-good.patch, failed testing.

jessebeach’s picture

I love seeing a flexible system flex! The state model is great for dealing with errors in a coherent way and the changes blend right into the existing flows. Nice work Wim!

  1. +++ b/core/modules/edit/js/models/EntityModel.js
    @@ -288,6 +288,16 @@ Drupal.edit.EntityModel = Backbone.Model.extend({
    +              var message = Drupal.t('Your changes to <q>@entity-title</q> could not be saved, either due to a website problem or a network connection problem.<br>Please try again.<br>If this problem persists, please contact us.', { '@entity-title' : entityModel.get('label') })
    

    Let's drop the "If this problem persists, please contact us." bit of the string. It implies some kind of support and is a little to specific for core.

  2. +++ b/core/modules/edit/js/util.js
    @@ -65,7 +96,20 @@ Drupal.edit.util.form = {
    +        var message = Drupal.t('Could not load the form for <q>@field-label</q>, either due to a website problem or a network connection problem.<br>Please try again.<br>If this problem persists, please contact us.', { '@field-label' : fieldLabel });
    

    Again, let's remove the "please contact us" bit of the string.

I've updated the test case patch with errors for both entity save and field form load. To trigger a field form error, create an article and add an image. Then in-place edit the article and attempt to change the image. The field form alert will be raised.

My only critiques are the text changes above. The code looks good, passes jsHint and addresses the two failure states that might arise because of network issues. The user is no longer left in a state where they can't continue because Edit is locked in a processing state.

Test on Simplytest.me with both the breaking patch and the fix patch.

Wim Leers’s picture

Fair enough — rerolled.

(Thanks for the review! :))

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

star-szr’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

Wim Leers’s picture

I wonder why? Let's do a re-test…

Wim Leers’s picture

The last submitted patch, 15: edit_network_error_handling-2055937-15.patch, failed testing.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Oops — back to RTBC, should come back green, just like #15 was.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: edit_network_error_handling-2055937-21.patch, failed testing.

Wim Leers’s picture

Either HEAD or testbot is broken ATM.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Came back green. Hence back to RTBC.

11 comments for what was a simple f'ing straight reroll. d.o-- :(

webchick’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: edit_network_error_handling-2055937-21.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.88 KB

Straight reroll (one of the other Edit patches changed one of the patch hunk contexts).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: edit_network_error_handling-2055937-29.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

testbot is drunk; this is a JS-only patch, it cannot cause exceptions.

webchick’s picture

Wim Leers’s picture

VICTORY! :D

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok cool. One nitpick is I know @yoroy would be-cry the use of the word "please" in UI text, but in trying to look up why that is I found http://ux.stackexchange.com/questions/25500/why-avoid-the-word-please-in... and it sounds like it's appropriate in cases where you're inconveniencing a user. Telling someone their edits were just lost seems like such a case. :P

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

Now also backported to the Drupal 7 contrib module: http://drupalcode.org/project/edit.git/commit/5f6f3afae6da7a6d9b9c2d1661....

Status: Fixed » Closed (fixed)

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