Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/dev 212 errors summary #12125

Merged
merged 49 commits into from
Aug 9, 2023
Merged

Conversation

i-just
Copy link
Contributor

@i-just i-just commented Oct 14, 2022

Work for dev-212 and #11569

Description

for dev-212:

  • added errorsSummary to CpScreenResponseBehavior
  • added errorsSummary to the cp layout
  • added errorsSummary to the CpScreenSlideout

for #11569:

  • added crossSiteValidate param to saving elements; it gets triggered when applying draft and saving element for multi-sites; it then subsequently checks if the element we're saving is supposed to be enabled, live or pending and only validates against Live scenario if that's the case
  • cross site show validation errors using the new errorsSummary section with a link to that element for the site that failed validation (with prevalidate param)

TODO:

  • styling for the new panel

Related issues

#11569
#11612

@linear
Copy link

linear bot commented Oct 14, 2022

DEV-212 Summarize errors on top of entry page

Motivation: I've been testing the readout of field errors and "Couldn't save" alerts with VO and have found that the experience of identifying and navigating to fields is difficult from the experience of a screen reader user.

  • The "Couldn't save entry" text is read out, but no additional context is immediately conveyed to screen reader users about which fields have errors
  • On the panel with the title field, autofocus brings the user's focus to that field; this may cause the user to believe the error has to do with that field.
  • A user can backtrack to the tab panel to find out if there are errors on the active panel, but the output only indicates that there are errors, not which fields they are associated with. In addition, because only one tab panel is in the focus order, the user would have to navigate between each tab using left/right arrows to find out if there are errors in that panel.

My suggestion is to add a new section to the content container. If the entry has errors after submit and couldn't be saved, keyboard focus should be moved to the heading indicating how many errors there were.

A list inside will link to each of the fields with errors, activating the relevant tab panel and moving focus to the input in question.

Screen Shot 2022-01-19 at 3.29.39 PM.png

This follows the pattern described here: https://www.w3.org/WAI/tutorials/forms/notifications/#listing-errors

@brandonkelly brandonkelly changed the base branch from develop to 4.3 October 17, 2022 12:20
# Conflicts:
#	src/controllers/ElementsController.php
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
src/services/Elements.php Outdated Show resolved Hide resolved
src/services/Elements.php Outdated Show resolved Hide resolved
@brandonkelly brandonkelly changed the base branch from 4.3 to 4.4 October 26, 2022 00:42
@gcamacho079
Copy link
Contributor

gcamacho079 commented Nov 8, 2022

Awesome work on this feature @i-just

Accessibility

The focus management on this page when the page loads is a little unexpected. Focus seems to either be placed on the title field or the active tab button. However, when there’s an error summary present, focus should be placed on the error container. This will require adding tabindex=“-1” to the .errors-summary container so that focus can be programmatically placed on it. Without explicitly placing focus on the summary, a screen reader user might miss it entirely.

I also noticed on entries with a lot of fields and/or multiple tabs or Matrix blocks, it can be difficult for users to track down the field in question. It would be nice if the list item text was wrapped in an anchor tag. When clicked, focus is moved to the field in question; if it’s in a different tab than the currently-active one, the new tab is activated and then focus is moved. See an example of this in the 'Error on top' recovery pattern

For the cross-site validation, I recommend removing the "(View)" text and having the entire error message as the link. This will help provide information about the link's purpose in-context

Styles

One styling suggestion that's also a11y-related - a large chunk of red text in the error summary might be anxiety-inducing (especially when paired with the error notification and the red tabs). I think having the summary heading and list text inherit the body text color would work well
A container that reads 'Found 2 errors' along with a list of fields containing errors; the text color is black
Then maybe adding a red warning icon to the left of the heading (like what's being used in the notification container):
A notification container that has a red warning icon, followed by the message 'Couldn't create entry'

@i-just
Copy link
Contributor Author

i-just commented Nov 9, 2022

Thanks @gcamacho079! All clear, all makes sense. I've actioned the following:

  • summary heading and list text have the body text color,
  • added a red warning icon to the left of the heading,
  • programatically placing focus on errors container,
  • entire cross site validation message is a link (removed "view"),
  • list items in the errors summary are wrapped in an anchor tag and link to their corresponding errors within the form - this is still a work in progress and doesn't currently work in the slideouts;

@i-just i-just marked this pull request as ready for review November 11, 2022 14:37
@i-just i-just requested a review from a team as a code owner November 11, 2022 14:37
@MoritzLost
Copy link
Contributor

Wasn't this PR supposed to be part of the 4.4 release? We were really looking forward to this one, sad to see it didn't make it. Error messages have way too little visibility right now, multiple of our clients have run into problems because they missed the tiny error message at the bottom left that the entry didn't save correctly.

This PR would've helped, though it still doesn't address the problem regarding validation errors on related assets I mentioned in my previous comment.

@i-just @gcamacho079 Any insight on why this didn't make it? Also, I would really appreciate a response to my previous comment as well as the discussion linked there. This is the largest issue we have with Craft, and our clients constantly stumble over this. Regarding the low visibility of error messages, we had to humbly apologize to a client who lost a lot of work because they didn't notice their entries hadn't been saved. There's some user error involved here, but due to the way that errors aren't clearly presented, we couldn't in good conscience tell them it was just their fault.

I feel this should get a higher priority due to the accessibility implications and frustration potential related to this issue.

@i-just i-just changed the base branch from 4.4 to develop March 15, 2023 12:43
@brandonkelly
Copy link
Member

@MoritzLost I didn’t get a chance to review in time for 4.4, but it’s planned for 4.5 :)

@i-just
Copy link
Contributor Author

i-just commented Mar 15, 2023

@MoritzLost, regarding your previous comment, we have decided to make the validation message for related elements more user-friendly, which we believe should help.

The related element validation will read “Validation errors in - please fix them.”

@MoritzLost
Copy link
Contributor

@MoritzLost I didn’t get a chance to review in time for 4.4, but it’s planned for 4.5 :)

Thanks @brandonkelly! We're looking forward to it.

@MoritzLost, regarding your previous comment, we have decided to make the validation message for related elements more user-friendly, which we believe should help.
The related element validation will read “Validation errors in - please fix them.”

@i-just Thanks, that will be an improvement over the current error message. Though I would still like to see the actual validation errors that occurred somewhere in the context of the field. I guess we'll see if this change helps our new clients understand the problem better without us having to explain it.

@brandonkelly brandonkelly changed the base branch from develop to 4.5 May 5, 2023 22:49
# Conflicts:
#	src/services/Elements.php
#	src/web/CpScreenResponseFormatter.php
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
#	src/web/assets/cp/src/js/ElementEditor.js
[ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants