Add explainer to warning notice when validation errors are displayed

[westonruter]

Feature description

When Developer Tools are enabled, when a validation error occurs on the permalink for a post, a warning will be displayed in the editor:

Prior to 2.0, Developer Tools were not available for Reader mode. Since most sites are in Reader mode, many users are going to be seeing this warning notice for the first time. We need to explain to them why they are seeing this warning all of a sudden. They need to know that the validation errors were occurring before, but they just weren't being told about them.

I wrote a support topic about this to preempt support topics being opened, but we could do something even more preemptive inside of WordPress. We can link to the relevant documentation page on amp-wp.org. If one isn't yet created, we should do so.

Additionally, we may want to provide a link to turn off Developer Tools in the user profile. Something like, “Don't want to see these warnings? Turn off AMP Developer Tools in your user profile” (where the bolded text links to the checkbox).

This is closely related to #4668, where we can mention here the specific themes and plugins that are causing validation errors. If there are validation errors from the theme, the warning should probably advise switching to Reader mode. Validation errors from plugins could advise to evaluate whether the removed invalid markup is needed, whether the plugin should be suppressed (with a link to the Plugin Suppression section). There can be prompts to reach out to the respective support forums for the themes and plugins.

Also related to #3821, as this information should be displayed in the sidebar.

---

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

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

[amedina]

Related: add content to amp-wp.org with explainer, to be linked from a "Why I am seeing this" link.

[johnwatkins0]

I saw this notice on a client project just yesterday and experienced a small amount of confusion and nervousness.

1. Confusion: "There are 2 issues from AMP validation which need review. The issues are not directly due to content here." The where of this is confusing. We should clarify to something like, "This post has two AMP validation issues, but they come from outside the post content. To review these issues, [go here]. To turn off these notices, [go here]."
2. Nervousness: Any time there is a link in a notice in the editor, I worried about what's going to happen when I click the link. I don't want the current tab to navigate to another page, potentially losing unsaved changes. The core UX around recovering autosaves is not great, so I avoid it in general. In other words, I think these links should have an icon indicating they open in a new tab.

Should we go forward with minor copy tweaks along these lines for 2.0.1 and save the broader updates for 2.1?

[amedina]

These notices need to change. However, as they are right now they might be confusing to some users, but the changes could also result in confusion. I don't want to change them hastily. I would like to address this in the context of the overall revamp of improving the "communication" aspects of the validation tools, add the Document and block panels with a nice design and precise messaging, and so on. We will discuss soon (i.e. Roadmap planning).

…

On Wed, Sep 2, 2020 at 8:06 AM John Watkins ***@***.***> wrote: I saw this notice on a client project just yesterday and experienced a small amount of confusion and nervousness. 1. **Confusion: ** "There are 2 issues from AMP validation which need review. The issues are not directly due to content here." The *where* of this is confusing. We should clarify to something like, "Two post has two AMP validation issues, but they come from outside the post content. To review these issues, [go here]. To turn off these notices, [go here]." 2. **Nervousness: ** Any time there is a link in a notice in the editor, I worried about what's going to happen when I click the link. I don't the current tab to navigate to another page, potentially losing unsaved changes. The core UX around recovering autosaves is not great, so I avoid it in general. In other words, I think these links should have an icon indicating they open in a new tab. Should we go forward with minor copy tweaks along these lines for 2.0.1 and save the broader updates for 2.1? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3R2OWZJW4W2FHSUG6GXTSDZNQHANCNFSM4QRCEMVQ> .

[westonruter]

We can open the link in a new window, however.

[westonruter]

Please open a quick PR for opening the link on a new window. We'll include that in 2.0.1.

[amedina]

+1

…

On Wed, Sep 2, 2020 at 8:33 AM Weston Ruter ***@***.***> wrote: Please open a quick PR for opening the link on a new window. We'll include that in 2.0.1. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5304 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD3R2PEHGLJVLEOC77DQT3SDZQUXANCNFSM4QRCEMVQ> .

[johnwatkins0]

In #5319 we made the change for 2.0.2 to open the link in a new tab. Should this be set to a later milestone now?

[westonruter]

You're right. Done.

[jwold]

Been playing with some concepts to address this, and leaning toward something like this:

Some kept and unreviewed state:

[westonruter]

@jwold I think we can just put the "Invalid Markup Kept" info inside of the "AMP Disabled" panel. So in general I think we can iterate on this element here:

This would be the “AMP Status” box and it would have 3 possible states which correspond to the 3 states in the icon:

Icon	State
	AMP Enabled, with either no validation issues or all markup being removed and reviewed.
	AMP Enabled, but there are one or more instances of invalid markup that is unreviewed.
	AMP Disabled due to some invalid markup being kept on the page.

We can tailor the message for each state for non-technical end-users.

As for the "Show reviewed errors", I think it can be made less prominent. I know I raised a concern about a toggle appearing at the bottom of the list being problematic because of content shifting, but I think the reality is that this UI element makes better sense at the bottom since it should be giving minimal prominence. Clicking the link can cause the link to change to “Hide reviewed issues (x)”. This link would only be present if there any reviewed issues. If not, the link would be absent.

[jwold]

@westonruter I believe this captures all the comments above, and will be worth discussing on our next call.

My next step is to take this to a prototype stage, but I'd love any feedback first on it. Thanks!

cc @amedina and @delawski

[westonruter]

I believe this captures all the comments above, and will be worth discussing on our next call.

Let's include your feedback from #5741 (comment).

[jwold]

@westonruter, @amedina, @delawski the latest revision of the work is ready for review. We chatted about this in our call today, and I'd love feedback. Thanks!

[westonruter]

@jwold sorry for the delay in reviewing these.

What again is the meaning behind the styling of the three errors listed here? The first one is kept but the last one is not, but they both have the orange border. I recall @schlessera having a good suggestion for the orange border, that it would only indicate the kept state so we wouldn't need the “kept” label necessarily. The “kept” label will be problematic because it will mash together with the error title, e.g. “Invalid inline script Kept”.

The four states, in order of frequency, are:

1. Unreviewed and removed (the default state)
2. Reviewed and removed (the normal resolution)
3. Unreviewed and kept (only happens by default for CSS)
4. Reviewed and kept (uncommon, as it causes AMP to be blocked for the page)

A final one in this context would be the partially-transparent error indicating the error was in a block that was removed, but that can be applied to any existing styling above.

I remember we had a discussion a couple weeks ago and deciding to diverge from how we've been trying to maintain the design language of unmoderated vs moderated comments, but now I forget. There's also the styling of suppressed plugins:

[delawski]

@jwold @westonruter We may need to adjust the copy around the "Re-validate" message because of the way custom meta boxes are handled in the block editor. See #5741 (comment).

As discussed, we may want to drop the message about post content change and leave the button alone.

[jwold]

Great feedback @delawski and @westonruter. A few things:

1. What if we combine the re-validate button with the AMP status message? (see design)
2. I was working on two things in parallel. What the design language should be for this update, and then what we'd want to for a future release. More in this direction: https://d.pr/i/KnngTd. However, I think we can pull parts of that in now.
3. Not sure how we'd want to handle suppressed plugins. Any thoughts are welcome.

[delawski]

1. What if we combine the re-validate button with the AMP status message? (see design)

So if there are no custom meta boxes active, then the UI (the notifications area) would still look like this, correct?

Then, for the just discovered case when there are custom meta boxes active, we would use the new UI as you proposed:

I think it would work. Still, having an analogical UI but with different microcopy for both use cases would be better I guess.

2. I was working on two things in parallel. What the design language should be for this update, and then what we'd want to for a future release. More in this direction: https://d.pr/i/KnngTd. However, I think we can pull parts of that in now.

In general – yes, but I'd also say that it depends. Doing too invasive updates to the UI can easily become a rabbit hole. At the same time, we're still targeting the 2.1 release. So I guess it depends on which parts of the UI have you been thinking of.

[jwold]

@delawski good point. Let's separate the messages and keep them in different notifications. The wording will need to be massaged of course, thinking something like:

If no custom meta boxes:

Page content has changed. Re-validate.

If yes custom meta boxes:

Page content may have changed. Re-validate.

cc @westonruter

[westonruter]

If yes custom meta boxes:

Page content may have changed. Re-validate.

How about just “Re-validate” in this case? Is anything else needed? Or “Made changes? Re-validate”.

[westonruter]

@delawski @jwold Also, can this be designed in a way that content shifting is minimized? The loading spinner and validate button (as of #5741) both currently contribute to the UI shifting up and down, which is not ideal:

content-shifting-in-amp-sidebar.mov

[delawski]

Also, can this be designed in a way that content shifting is minimized?

I was thinking about this issue, too.

I spoke about the loading state with @jwold lately and he suggested a pretty nice approach:

[For the loading state, think of a situation] where a notification still shows, so it's more clear what's happening, and everything else disappears as it needs to. Also might require shifting the notification up to the top so it doesn't jump around.

@jwold Please chime in as I think we're going in a really good direction with that.

[delawski]

Another issue I've noticed while working on #5741 is that we don't currently have any error handling.

If the REST endpoint responds with 4xx or 5xx, we're showing the loading spinner forever.

We should probably handle such errors with a proper message (sometimes a message is returned in the response – we could use it) and offer some support like a "Try again" button.

[jwold]

What if we did something like this? I think it might avoid content shifting altogether.

[westonruter]

QA Passed

The scope of this issue evolved. The warning notice has been eliminated in favor of showing the error in the sidebar:

And in the Post sidebar:

And similarly in the pre-publish panel:

When Developer Tools are turned off, the AMP Validation sidebar is not present: