Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Steps to reproduce
- Apply the attached patch,
break-it--break-it-good.patch
- Open a node for in-place editing by activating the quick edit link from its contextual links.
- Change the content of a field on the node.
- Press the save button.
- The saving spinner should remain and never dismiss.
- Check the browser console to see the printed error message. The error handler is in
EntityModel::save
.
Proposed resolution
- Deal with failed AJAX requests (error handler)
- 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.
Related Issues
None.
Original report by jessebeach
Comment | File | Size | Author |
---|---|---|---|
#29 | edit_network_error_handling-2055937-29.patch | 4.88 KB | Wim Leers |
Comments
Comment #1
nod_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?
Comment #2
nod_The technical issue isn't hard to solve it's more the expected UX we need to know about.
Comment #3
jessebeach CreditAttribution: jessebeach commentedThe 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.
Comment #4
Bojhan CreditAttribution: Bojhan commentedIt'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.
Comment #5
Wim LeersNote 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.
Comment #6
Wim LeersWe're not working on this in the current sprint.
Comment #7
yoroy CreditAttribution: yoroy commentedClearing out the 'needs usability review' tag.
Comment #7.0
yoroy CreditAttribution: yoroy commentedput in a list.
Comment #8
Wim LeersIn 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.Comment #10
Wim Leers#8 was rolled on top of some other patches: reroll straight on current HEAD.
Comment #12
Wim Leers10: edit_network_error_handling-2055937-10.patch queued for re-testing.
Comment #14
jessebeach CreditAttribution: jessebeach commentedI 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!
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.
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.
Comment #15
Wim LeersFair enough — rerolled.
(Thanks for the review! :))
Comment #16
jessebeach CreditAttribution: jessebeach commentedLooks great!
Comment #17
star-szrTagging for reroll.
Comment #18
Wim LeersI wonder why? Let's do a re-test…
Comment #19
Wim Leers15: edit_network_error_handling-2055937-15.patch queued for re-testing.
Comment #21
Wim LeersStraight reroll. One line in
core/modules/edit/js/util.js
had changed, that's all.Comment #22
Wim LeersOops — back to RTBC, should come back green, just like #15 was.
Comment #24
Wim LeersEither HEAD or testbot is broken ATM.
Comment #25
Wim Leers21: edit_network_error_handling-2055937-21.patch queued for re-testing.
Comment #26
Wim LeersCame back green. Hence back to RTBC.
11 comments for what was a simple f'ing straight reroll. d.o-- :(
Comment #27
webchick21: edit_network_error_handling-2055937-21.patch queued for re-testing.
Comment #29
Wim LeersStraight reroll (one of the other Edit patches changed one of the patch hunk contexts).
Comment #31
Wim Leerstestbot is drunk; this is a JS-only patch, it cannot cause exceptions.
Comment #32
webchick29: edit_network_error_handling-2055937-29.patch queued for re-testing.
Comment #33
Wim LeersVICTORY! :D
Comment #34
webchickOk 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!
Comment #35
Wim LeersNow also backported to the Drupal 7 contrib module: http://drupalcode.org/project/edit.git/commit/5f6f3afae6da7a6d9b9c2d1661....