Currently ajax errors are only visible in the browser console. This is confusing for non-technical users because there's no indication in the UI that an error happened. Providing easy to understand error messages is often listed as one of the most important UX considerations.
Display the following text:,
Oops, something went wrong. Check your browser's developer console for more details.
-
In case of an ajax error, the error will be visible in the UI.
-
-
| Comment | File | Size | Author |
|---|---|---|---|
| #53 | 2987444-53.patch | 947 bytes | tаo |
| #34 | Screen Shot 2023-03-17 at 12.01.25 PM.png | 39.79 KB | hooroomoo |
| #4 | 2987444-4.patch | 3.78 KB | GrandmaGlassesRopeMan |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare changes, plain diff MR !3609
Comments
Comment #2
edysmp+1
Comment #3
tim.plunkett+1
Comment #4
GrandmaGlassesRopeMan- enable `ajax_test` module
- visit `/ajax-test/exception-link`
- see error
Comment #6
lauriiiThe test failure is caused by the fact that big pipe has now dependency to
drupal.announcewhich appends some markup to the body.Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testMultipleClosingBodies_2678662is testing whether the big pipe stop signal was presented right before the closing body and HTML tags. I'm not sure if that is an implementation detail or requirement for the big pipe. This is not a new problem since if something on the page already depended ondrupal.announce, this same situation would occur.Comment #7
lauriiiI don't think we have to worry about this as part of this issue. This is something we should think about as a separate feature in the future.I misunderstood the @todo. I thought this was about storing JavaScript errors in the logs but @drpal explained that this is about the "Error messages to display" setting in the logging and errors settings page. It seems like it is possible to disable showing error messages altogether. I agree that we probably should honor that setting.
As a next step, we should expose the setting in drupalSettings and adjust the error handling according to that.
Comment #8
lauriiiWe discussed this with @drpal and @tedbow and agreed that we should add a new configuration to the UI that would allow configuring whether there should be ajax error visible for users. This will be configured so that existing sites would have errors turned off, but any new sites would have them turned on by default. Also, the texts in the UI should encourage users to have this configuration turned on opposed to the PHP error messages since this is UX enhancement, and won't include any potentially private information.
Comment #10
ckrina+1 to this
Since we already have the form Configuration / Site settings it could go there initially, mainly because we already have the "Default 403 (access denied) page" and "Default 404 (not found) page", but I'm not 100% sure. This maybe could go under Configuration / User interface too if we add other UI changes that would make sense together with this.
Comment #12
lauriiiComment #19
lauriiiComment #22
tim.plunkettRunning Drupal\Tests\system\FunctionalJavascript\OffCanvasTest::testNarrowWidth locally I also see a failure.
core/themes/olivero/js/messages.js is trying to write to
Drupal.oliverocore/themes/olivero/js/navigation-utils.js defines
Drupal.oliveroand they are running in that order!
I'm not sure the best way to get them in the correct order.
Comment #23
hooroomooComment #25
tim.plunkettI love that this surfaced and fixed a bug!
I'd mark it RTBC, but #10 is not addressed as this is not yet configurable. Is that still needed first?
Comment #26
tim.plunkettOh lol, Nightwatch tests are still failing. Glad I didn't RTBC :D
Comment #27
hooroomooComment #28
smustgrave commentedReviewing MR 3609
Left a comment but why wouldn't olivero/global-styling not be there?
Also I see the new ajax_test routes but don't see the test that calls them? Unless there's something that loops through all the routes? But couldn't find that.
Comment #29
smustgrave commentedAlso to echo what tim.plunkett said. This sounds like a great feature to get included!
Comment #30
tim.plunkettThe test routes were added in #4 but only for manual testing. You're right that we need an automated test to use them. Leaving the other response on the MR.
Comment #31
hooroomooComment #32
lauriiiDiscussed with @ckrina who is one of the Usability topic maintainers. We agreed that this is a net-win as in all situations where user is triggering an action, and it fails to execute. Adding new error messages may make us discover some bugs in other systems but that's not the fault of the error, in fact it's the opposite since it will help us expose those use cases and work on the root cause.
Quoting an article from Nielsen Norman:
Based on this, it would be great if we could still iterate on the specific wording of the message. I think we should at least try to remove mention of ajax since it's not language that users are accustomed to.
Comment #33
smustgrave commentedMoving to NW per #32.
Leaving the needs tests tag incase any additional coverage is needed.
Comment #34
hooroomooAn AJAX system error has occurred. -> Oops, something went wrong. Check the console for more details.
Updated message string to remove the mention of AJAX and point to the console if the user wants more information.
Comment #35
tim.plunkettI got this failure once in every 10 local test runs.
I think this is described here as well #3346122-4: ckeditor test failures causing pain for unrelated patches
Tests were added in
b1f1df3b - WIP: add test that currently failsComment #36
smustgrave commentedGet the same results as #34 using the new routes provided.
Points from #32 appear to be addressed.
Think
could be a follow up?
Comment #37
tim.plunkett+1 for RTBC, the wording changed in the last commit and I think it's shippable as-is
Comment #38
borisson_I agree with #37, the wording is now less technical. Looks great.
Comment #39
gábor hojtsyHm, the problem statement is this:
Looking at the error message proposed, I think "the console" would not be evident to a non-technical user. Is it a Drupal backend concept or in their command line terminal? I think "your browser's developer console" or something along those lines would be easier to google for. It would be nice if there would be a product agnostic page that we could link to, like https://balsamiq.com/support/faqs/browserconsole/ but not under Balsamiq (nothing against Balsamiq but you get the idea). Otherwise I think "your browser's developer console" makes it much more specific and is good basis to google for. I am not attached to these specific words, but IMHO "the console" is not enough for a non-technical user.
Comment #41
hooroomooComment #42
smustgrave commentedConfirmed the message is now
Oops, something went wrong. Check your browser's developer console for more details.
Comment #43
bnjmnmIt looks like there was a decision to make this configurable, which I don't see in the MR. Either that should be added, or someone can point out where that requirement changed and switch this back to RTBC.
Comment #44
lauriiiReading it back, I realize that #32 may have been unclear. The reason I made that comment was to try to make a point that we shouldn't make it configurable because having these error messages are essential for UX. I discussed that with @ckrina as well to make sure that she didn't have concerns on that.
Comment #45
quietone commentedI agree that this is a step in the right direction.
I am concerned that the message is not helpful to a non technical user because it suggests that they learn how to use a technical tool. And they will need to search for how to do that for their browser. Instead can we change this to a message that they should contact the site admin?
Comment #46
bnjmnmRefining that message content could potentially delay what is a very helpful improvement. I created a followup to focus on adjusting that language after this lands, which lets this feature get in while still providing us with a place to potentially improve that messaging.
#3349901: Determine if there is a better message for AJAX errors
Comment #48
lauriiiCreated and published a CR for this.
Committed e492ae3 and pushed to 10.1.x. Thanks!
Comment #51
tedbowSo I think this issue broke Javascript messages on batch forms. But there were no tests for that so it wasn't going to be caught.
Created #3392196: Exceptions in batch no longer are shown on the page when Javascript is disabled we have a test there and are working on a fix. Any help would be appreciated.
Comment #52
aporieHi,
I might be in the wrong here, but why assuming that every Ajax response should return a 200 is the expected behavior for all websites.
Sometimes you might want your user to be able to click an ajax link which will return a 403 for example. Then you purposely display a message to your user that they are not allowed to perform the action.
With this addition, websites have now a non-needed, non-user-friendly message displayed to users on expected 403.
Maybe a good addition would be to make this feature optional (as per improving the message in #3349901: Determine if there is a better message for AJAX errors)?
Comment #53
tаo commentedIt makes AJAX error behaviors more configurable.