Problem/Motivation

Having verbose error logging on is good for security as we see bad things soon, but screens full of backtraces are a PITA.

Which nudges us developers to develop without warnings and notices, which is bad and kills kittens.

Proposed resolution

Wrap backtraces in a details tag in UI error messages.

Remaining tasks

Make followups

Usability review
Code review

User interface changes

Notices / Warnings are much more compact.

Before

before

After

after

API changes

None.

Data model changes

None.

Release notes snippet

If developer chooses to see verbose error messages in the UI, the backtraces are now wrapped in a details element.

CommentFileSizeAuthor
#16 focus.jpg79.06 KBrkoller
#16 voiceover.mp42.32 MBrkoller
#15 Screen Shot 2023-02-04 at 12.24.08 PM.png196.43 KBsmustgrave
#15 Screen Shot 2023-02-04 at 12.23.56 PM.png467.57 KBsmustgrave
#11 reroll_diff_3153567_2-11.txt1.62 KBankithashetty
#11 3153567-11.patch724 bytesankithashetty
#2 Bildschirmfoto vom 2020-06-20 22-38-49.png40.93 KBgeek-merlin
#2 Bildschirmfoto vom 2020-06-20 22-34-17.png190.01 KBgeek-merlin
#2 drupal-3153567-2-error-details.patch1.07 KBgeek-merlin

Issue fork drupal-3153567

Command icon Show commands

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

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geek-merlin created an issue. See original summary.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
Issue summary: View changes
Status: Active » Needs review
FileSize
drupal-3153567-2-error-details.patch1.07 KB
Bildschirmfoto vom 2020-06-20 22-34-17.png190.01 KB
Bildschirmfoto vom 2020-06-20 22-38-49.png40.93 KB

Patch flying in that implements this. And the result is a revelation:

Before

With patch

Chi’s picture

Chi CreditAttribution: Chi commented

How will this look in CLI context?

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented

> How will this look in CLI context?

This will only ever go to the HTML UI message area via the messenger service. Even the message going to the logger is a different one, see related issue.

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

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

9.3 still applies, 9.4 needs reroll

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
Issue summary: View changes
rootwork’s picture

rootwork
he/him
Primary language English
Location 🇺🇸 US-Pacific 🌎 Chinook (Multnomah, Clackamas) & Cowlitz lands 🌹 Portland, OR
CreditAttribution: rootwork commented
Issue tags: +Novice, +Portland2022

Tagging as novice for the reroll.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
CreditAttribution: ankithashetty as a volunteer and at Specbee commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3153567-11.patch724 bytes
reroll_diff_3153567_2-11.txt1.62 KB

Rerolled the patch in #2, thanks!

rootwork’s picture

rootwork
he/him
Primary language English
Location 🇺🇸 US-Pacific 🌎 Chinook (Multnomah, Clackamas) & Cowlitz lands 🌹 Portland, OR
CreditAttribution: rootwork commented

I think this can still be novice for the review, if someone wants to take this up and confirm that it applies and provide screenshots.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs Review Queue Initiative, +Needs usability review
FileSize
Screen Shot 2023-02-04 at 12.23.56 PM.png467.57 KB
Screen Shot 2023-02-04 at 12.24.08 PM.png196.43 KB

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Seems like a neat request.

Tagging for tests as we will need coverage for this change.
Tagging for usability review so they can see if there is any issue. Will admit the arrows for the collapsed section just looked like bullets. If I didn't know to looked for collapsed not sure I would of clicked on them.

Adding screenshots

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
CreditAttribution: rkoller commented
FileSize
voiceover.mp42.32 MB
focus.jpg79.06 KB

i've quickly tested the patch in drupal 9.5.3. . in general it is a great idea. i've noticed one or two details.

  1. i've tested the patch with voiceover (in macos 12.6.1 - see voiceover.mp4). i've only tabbed to the first notice and pressed command-option-space as voiceover recommended. what i have expected is that the rest of the content of the detail element is announced (the one wrapped in the pre tag). but instead first the summary of the second warning, then the summary of the third warning, and after that the rest of the content of the first expanded detail element is announced. if i wouldn't been able to follow along visually that would be super confusing and i would be unable or at least have a hard time to distinguish between the content of the different warnings. Edit: And the state change to expanded of the detail element isn't announced somehow.
  2. the color of the focus outline is dark blue on black in olivero which is probably not meeting WCAG SC 1.4.11. i was unable to switch on the focus in safaris webprofiler and get the actual hex values (instead i've only used a color picker which isnt providing the exact values :( ). but the values i got for the warnings in olivero were #24396f for the focus outline and #0e1215 for the background which leads to a contrast of 1.69:1 instead of 3:1 (and visually it looks like it has too less color contrast plus the outline isn'T shown on all four sides for none of the three detail elements). the outline would have to be checked for claro as well (but i dont know how to simulate such an error for claro as well - was just using the error test module with /error-test/generate-warnings)

    3. admin notice with three warning in olivero

  3. and from a sighted users perspective the readability might be improved. i consider it difficult to grasp the visual structure. the content in pre tags might have a slightly too large line height. and some of the lines which are too long are instead of being wrapped just being cut/hidden. i wonder would it make sense to get some design input for the notices?
Rishabh Vishwakarma’s picture

Rishabh Vishwakarma CreditAttribution: Rishabh Vishwakarma at OpenSense Labs for DrupalFit commented

Patch 11 works on 10.1.x as well.

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented

@rkoller Thx for the usability review in #16.
As of your points:
> 2. [colors]
> 3. [line height]
This is undoubtably theme dependent. I'd propose to tacke these optimizations in followup issues per theme.

> 1. [details difficult to navigate via text-to-speech assistive technology]
I'm not into a11y of details elements. I guess adding some kind of aria-foo attribute may help.
Do you have a suggestion what this might be?
EDIT: I misread the post, it's not about aria labels.

geek-merlin opened merge request !3866

geek-merlin’s picture

geek-merlin
they/omg
Primary language German
Location Freiburg, Germany
CreditAttribution: geek-merlin at Geeks4Change, MachbarMacher commented
Status: Needs work » Needs review
Issue tags: -Needs tests

As of a11y, a line of error summary with option to read aloud a backtrace is obviously better than a 50+-lines bob of backtrace.
As of theme optimization, i can open followups.
Now we have the feature and a green test.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs usability review +Pittsburgh2023

Looked at this with @lauriii at DrupalCon Pittsburgh for the usability tag and didn't notice any issues with the change.

Testing out it functions as expected
Used the error_test module
Went to /error-test/generate-warnings
Errors were in a detail tag now.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Component: browser system » base system
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Let's open a followup to deal with the accessibility issues raised in #16 through #20; I don't believe this is any worse than it was previously and so this shouldn't block commit.

Not eligible for backport as this is a UI change in 10.1.x, even if this is developer-facing only.

Committed 87afd2c and pushed to 11.x. Thanks!

  • longwave committed 87afd2c3 on 11.x 
    Issue #3153567 by geek-merlin, ankithashetty, smustgrave, rkoller: Wrap...

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Issue summary: View changes
Status: Closed (fixed) » Needs work

This needs work for the followups, see #24