Improve placement of AMP validation errors in the block editor

[jauyong]

Feature description

Consider moving the warning notice to the sidebar since inline it will cause problems based on where the block is located in the editor. The block could have some error outline so that when clicked it will show the block sidebar with the validation error information in an expanded panel.

This was taken out of #3664

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Make sure that the aspects in #5304 are accounted for.

Implementation brief

QA testing instructions

Demo

Changelog entry

[westonruter]

The icon on the left is the JS/CSS/HTML icon. The icon on the right is the block icon taken from the block settings. This is a departure from the design (where the block icon has a gray background with white SVG paths) because these block icons can be almost anything, so we shouldn't try to manipulate them beyond adding that border and the border radius.

I see. When I first saw the icons I wasn't sure what they were referring to, until I read your subsequent explanation. What if there is an “HTML” error in a “Custom HTML” block? Wouldn't that mean “HTML” is just repeated repeated twice? That being the case, I think the type should be more concise like the VSCode file icons. The icon can be made circular, but then it can be combined with the block icon which can be square (since it is a block after all). This will avoid scenarios where we try to fit a round peg into a square hole, as the block icons are almost all square in shape.

3. I have broken Reviewed and Unreviewed errors into two sections. And the number on the plugin icon corresponds to the number of unreviewed errors. I think this is the right approach because there may be errors that have been reviewed and which the user might not want to be bothered with, even if they remain for now. Getting that badge number down to 0 should be easy.

This seems reasonable. My only concern is that the errors would then be separated into two groups, so if two errors are both occurring in the same block, with one reviewed and another not reviewed, they would not appear together. What if instead of having two separate sections, there is instead a toggle or button to show/hide the already-reviewed errors? The reviewed issues would be hidden by default, which makes sense to me since the user already reviewed them and doesn't need to take any more action. The ones that are not reviewed should have the same red/orange styling as the unreviewed errors on the validated URL screen or unmoderated comments on the Comments screen. This would differentiate them from reviewed errors when all are shown together.

We should probably include more than this but less than what's on the URL validation screen. Any suggestions?

That's a good question. There's a lot of information available:

One thing is there should be a link to view that error on the validated URL screen (opening the screen, expanding the error, and scrolling into view).

In this context, I think the most important information to display in the panel is the theme/plugin responsible for the error. We won't always have that information available, especially for static blocks, in which case it would be up to the user to determine which plugin introduced the block. But the full name of the block needs to be stated (which we should always have available in the JS context), along with the theme/plugin which caused the error (if we have that). These along with a link to get the full details (perhaps even a "copy error to clipboard" in this context) will allow for the user to take report the error directly to the plugin author. In the editor context, users shouldn't really care about the details (the what) but about who is responsible.

I'm not sure I'm linking this orange line around the block containing an error, because as a new user I might not know what it is:

Yeah, the orange line may be confusing. But yeah, they should be able to connect the dots once they select a block. I think this is OK for now.

I'm not sure that this is a good idea. Should we perhaps only show the icon on blocks that have unreviewed errors?

Yes, let's only have the icon show up if there are any errors associated with the block. We only want to have the button if there is going to be one or more errors to show in the sidebar. If there are unreviewed errors, it should get the red number. If reviewed, then the number could be black?

If there are no errors associated with the block, then no toolbar button would be present.

I would like to explore creating a small REST endpoint to allow these errors to be marked as reviewed from the editor. As a user, this would allow me to get that badge number down to 0 without leaving.

The REST endpoint can also allow these errors to be marked as Kept/Reviewed.

This can be a small REST endpoint, likely to be adapted into a more full-featured endpoint in the future.

I think this should be deferred because we should make sure that there is a workflow where the users can see what impact removing or keeping invalid markup has on the page. To do that, we should have a preview ability and perhaps some paired browsing UI.

[johnwatkins0]

Thanks for the feedback @westonruter. I will revisit in detail later, but I wanted to mention I love this idea:

What if instead of having two separate sections, there is instead a toggle or button to show/hide the already-reviewed errors? The reviewed issues would be hidden by default, which makes sense to me since the user already reviewed them and doesn't need to take any more action.

Much better than what I'm doing now with two sections.

Also wanted to call on @jwold to take a look at Weston's first paragraph above. I agree that the block icons should be square, but when placed on a white background with no border, the core icons just float there with no apparent shape. This little piece has been challenging for me, so I would appreciate any visual ideas you can share.

[jwold]

@westonruter happy to play with the icons more.

Note that the VS studio logo uses the <> brackets icon: https://d.pr/i/fu7elu. That may make more sense for us to just replicate?

[westonruter]

Yes, that looks good. Using those VSCode icons in a circle, along with the block icon in a square.

[johnwatkins0]

@jwold Based on Weston's feedback above I'm now combining reviewed and unreviewed errors into a single list, with a checkbox to include reviewed errors.

Do you think we should visually distinguish new and reviewed errors in some way? Following this model, potentially (if I remember correctly, you proposed this at an early stage):

What do you think? While we're at it, I think we should visually distinguish the items that have been marked as Kept because they break AMP-compatibility.

[johnwatkins0]

On my question above, I'm going to try Weston's suggestion:

The ones that are not reviewed should have the same red/orange styling as the unreviewed errors on the validated URL screen or unmoderated comments on the Comments screen. This would differentiate them from reviewed errors when all are shown together.

But I still think we should also have a way to differentiate the errors that are causing AMP invalidity on the page due to their being kept.

[westonruter]

Currently your screenshot shows that the validation error text would be like:

But this is not the title for this error type. I mean, “kept” is not part of the string. The string returned would be:

Invalid script: jquery.js

So then each error will need to have something appended to it to indicate the removed/kept status. So this could be appending (removed) or (kept) to the title. Nevertheless, since the vast majority of errors will have the invalid markup removed, perhaps it is best to just leave the "removed" text absent. Then in the case of invalid markup being kept, which is the exception case, then an icon could be shown after the title, like ❗ or ⚠️ . Expanding the error will should indicate in prose that the invalid markup is being removed or kept, along with the identified source of the error.

[jwold]

1. Showing previously reviewed errors: We may want to move the Include previously reviewed errors to the bottom of the list. Since it's something that is inherently not as needed, moving it out of the way allows users to focus first on the un-reviewed items. Screenshot: https://d.pr/i/nAJDb6

2. Highlighting previously reviewed errors with a different color: Makes sense to try adding the red/orange styling for now. Will we need a way to mark something un-reviewed? We explored some ideas for that earlier, I can bring that back around if we need.

3. Adding an icon to indicate kept/removed status: The quickest idea would be appending the icon to the end of the label. I can mock this up if you'd like!

4. Regarding the icons: Let's use this one for HTML for now: https://d.pr/f/t1nebd. Agreed with changing the Gutenberg blocks to be white.

cc @johnwatkins0

[westonruter]

• Highlighting previously reviewed errors with a different color: Makes sense to try adding the red/orange styling for now. Will we need a way to mark something un-reviewed? We explored some ideas for that earlier, I can bring that back around if we need.

Reviewed errors have no highlight. If something is un-reviewed, then it gets the same red/orange styling as non-reviewed.

• Showing previously reviewed errors: We may want to move the Include previously reviewed errors to the bottom of the list. Since it's something that is inherently not as needed, moving it out of the way allows users to focus first on the un-reviewed items. Screenshot: https://d.pr/i/nAJDb6

Perhaps so. But if the reviewed errors are moved to the bottom then the errors won't appear in the order that they occur in the document.

Also, if the checkbox is moved down, then when checked would the reviewed errors appear after the checkbox, with the unreviewed errors before the checkbox?

[jwold]

Reviewed errors have no highlight. If something is un-reviewed, then it gets the same red/orange styling as non-reviewed.

Agreed. So we'd want to add the styling to the un-reviewed.

Perhaps so. But if the reviewed errors are moved to the bottom then the errors won't appear in the order that they occur in the document.

Also, if the checkbox is moved down, then when checked would the reviewed errors appear after the checkbox, with the unreviewed errors before the checkbox?

What I had in mind was a bit different.

1. Move the checkbox to the bottom, but don't change anything else.
2. Reviewed errors appear/disappear in the order in which they appear on the page, the checkbox is just always below that.

[westonruter]

• Move the checkbox to the bottom, but don't change anything else.

A problem with this is content shifting. Toggling the checkbox would cause the vertical position of the checkbox to change.

[westonruter]

@johnwatkins0 @jwold I have a suggestion related to #4668 where the important thing for non-technical users is to highlight incompatible theme/plugins. For users with DevTools turned off, we should probably still show the AMP plugin sidebar to alert them of issues. However, listing all of the validation errors is probably too much information.

So here's an idea. When DevTools are turned off, validation errors could be hidden by default in the plugin sidebar. Instead there could be a list of the theme/plugins identified as the sources for those errors in the first section of the sidebar. This could even be relevant when DevTools are enabled: the list of theme/plugins above could serve as a way to filter the list of errors that appear below.

[johnwatkins0]

@jwold Do you think you could mock something up for Weston's suggestion above? There is:

1) For non-technical users, the sidebar containing a list of themes and plugins causing AMP validation errors. Nothing else would be in the sidebar. 2) For technical users, a way to filter the errors list by theme/plugin.

I think this can potentially be done as a follow-up to this issue.

[jwold]

When DevTools are turned off, validation errors could be hidden by default in the plugin sidebar.

Will we want to also allow non-technical an option to see these errors if they wish? I realize that's not the intent with this work, but wondering if we want to enable that for the brave.

[westonruter]

I think there are three categories of users:

1. Admins with DevTools turned on: They would see the list of theme/plugins and the validation errors shown by default.
2. Admins with DevTools turned off: They would see the theme/plugins and then instead of the list of validation errors they'd see a button that could be clicked to reveal the errors.
3. Non-admins (or any user who is not eligible to enable DevTools): No list of theme/plugins and no validation errors available to display. In fact, we haven't even intended to show the AMP plugin sidebar at all. However, there may be some value to show something to indicate that there is some issue that they should reach out to an administrator to resolve. This will require more thought, as for normal authors we don't want even to show anything related to AMP for non-admins. Perhaps some note in the pre-publish panel?

In cases 1 & 2 for admins, the key is that these are administrators who have the ability to take action on the errors. They have the ability to deactivate/switch themes and plugins. In the case of admins who have DevTools turned off, there should perhaps be the same message as indicated in 3, where there is a way to notify an administrator who does have DevTools turned on. This is referenced in #2316 (nevertheless, there may not be any other administrators on the site).

The 3rd point is out of scope for this issue. For non-admins, no indication of any validation error would be shown.

The 2nd point may be still controversial. I can imagine some sites wanting everything related to AMP compatibility to be hidden, even for admins with DevTools turned off. There's a balance here of trying to conceal complexity, but at the same time not going to the degree of violating user trust by serving broken pages.

[westonruter]

QA Passed

When Developer Tools are turned on, validation errors are shown in the new AMP Validation sidebar:

Errors Collapsed	Errors Expanded

When an error is originating from a block, it is indicated in a new AMP toolbar button:

So there are no longer any warning notices that get in the way of blocks.