Enhance Compatibility debugging workflow narrative

[amedina]

The AMP plugin functionality is centered around interactions between actions taken by the plugin and actions taken by the user. The plugin can be configured to detect validation errors and automatically remove them (automatic sanitization). Or it can be configured to only detect errors but not removing them (manual sanitization). The user/developer has the control of what to do with detected errors. He/she can accept the plugin action (remove/not remove), or he/she can reject the plugin action.

This dicothomy between plugin and user actions percolates to the compatibility tool, and it is a main source of confusion for users of the plugin. This manifests in the labeling of errors that are surfaced by the compatibility tool as New Accepted or New Rejected, and the labeling of errors after the user has taken an action: Accepted or Rejected.

The problem is that the plugin actions are Remove and Keep (referring to the fate of the offending markup causing an error), while the user action is Accept or Reject (referring to the user intention with respect to the plugin action).

To remove this confusion, the compatibility tool should label the errors according to the action of the plugin, and not according to the action of the user. This entails updating the error labels this way:

• New Accepted to New, Removed
• New Rejected to New, Kept
• Accepted to Removed
• Rejected to Kept

This is only a change in labels and internal resource labeling.

[westonruter]

See also #1741

Does this terminology change work for "Statuses"?

Since the underlying entity is the "validation error", does it make sense that a "validation error is removed" and a "validation error is kept"? Or does removed/kept make more sense if the underlying entity is the "invalid markup"?

[amedina]

I see that point. Still we are on the right track. Let's flesh out things a little more. The ultimate goal should be two fold:

1. Decouple plugin actions from user actions
2. Maintain consistency on the plugin side so that labels and statuses refer to either plugin actions or error-specific labels.

The status of an validation error as removed captures the fact that the error is not present in the generated content. That makes sense for status labels: this is an error which has been removed from the content. Similarly, the status of an error can be Kept, meaning it is still present in the generated content.

There are no other statuses for errors; that is, there is not a 'fixed' status, or anything else. And this approach satisfies conditions (1) and (2) above.

WDYT?

[westonruter]

As discussed, it may be better to speak of “Retained” rather than “Kept”.

Something else to consider is that if:

• ❌ Rejected ➡️ Retained/Kept
• ✅ Accepted ➡️ Removed

Do these icons still make sense?

[westonruter]

For master issue on the Compatibility Tool redesign, see #2316.

[amedina]

Discussing today with @pbakaus we considered using:

❌ Rejected ➡️ Markup Retained
✅ Accepted ➡️ Markup Removed

Which aligns well with the sanitization actions by the plugin.

[westonruter]

The only gotcha with “Markup” is that the validation error could be due to non-markup code, like an illegal CSS at-rule or excessive CSS.

[pbakaus]

Retained is a pretty complicated word, that many non-native speakers will not understand, I'm afraid.

How about:
-> Remove invalid content
-> Keep invalid content

We could also replace invalid with incompatible, might be clearer.

[amedina]

The label must capture the status of the markup (past tense). We can have:

❌ Rejected ➡️ Markup Kept
✅ Accepted ➡️ Markup Removed

[amedina]

Agree with using content instead of markup for generality and simplicity

…

On Thu, May 16, 2019, 5:29 PM Paul Bakaus ***@***.***> wrote: Retained is a pretty complicated word, that many non-native speakers will not understand, I'm afraid. How about: -> Remove invalid content -> Keep invalid content We could also replace invalid with incompatible, might be clearer. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#2023?email_source=notifications&email_token=AAD3R2LLPNHYPWA54MVC3UTPVX35BA5CNFSM4HBJAPYKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVTMVZY#issuecomment-493275879>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD3R2IHNVORZJ7DARWKX3TPVX35BANCNFSM4HBJAPYA> .

[westonruter]

What about “Code” instead of “Content”?

[postphotos]

YES! This is a major step in the right direction. 🚀

Calling out Weston's note above, sometimes it's an illegal CSS rule or excessive that is flagging the sanitizer, not markup.

So if it's helpful, I'd suggest a slight adjustment, but with the same line of thinking:
❌ Rejected ➡️ Content Kept (AMP Not Served)
✅ Accepted ➡️ Incompatible Content Hidden

Maybe that's too wordy, but at least it's well understood.

[westonruter]

Reject: 🗑
Accept: 💾

😉

[westonruter]

Another alternative to “accept” is “suppress”, as the acceptance is intended to be temporary. The underlying issue should ideally be fixed in the theme/plugin that caused the problem in the first place.

[westonruter]

Stepping back from this to get a fresh look.

---

The AMP plugin has an internalized representation of the AMP validator specification. The internalized validation rules are used in the plugin's sanitizers to prevent invalid AMP markup from being served on the frontend (to prevent Google Search Console from complaining). At the same time, the AMP plugin provides warnings about those caught validation errors in order to ensure the user is aware that it performing any sanitization actions.

Validation errors that have already been seen need to be marked as acknowledged in order to differentiate them from newly-encountered validation errors that require the user to review.

The user also needs to be able to opt-out of the markup causing a validation error from being sanitized. It may be that some invalid AMP markup on the page is actual critical for the page to function properly (e.g. some interactive graphic), and in this case the user needs to reject it from being sanitized. When a sanitization for a validation error's markup is rejected, there are two outcomes:

• On a Transitional mode site (which is paired AMP), the user is redirected from the AMP version to the non-AMP version, and the rel=amphtml link is not even presented for crawling.
• On a Standard mode site (which is AMP-first), there are no non-AMP URLs and thus redirection is not an option. In this case the amp attribute is removed from the root HTML element, preventing the page from being validated as an AMP page (and thus preventing Google Search Console from complaining). Such pages still use the AMP framework, and eventually these would be utilizing Bento AMP.

As such, a validation error which is sanitized is shown with ✅ whereas a validation error which is not sanitized is shown with ❌, since whether something is sanitized determines whether AMP is available.

There are 4 possible states for a validation error:

• ✅ New Accepted
• ✅ (Ack) Accepted
• ❌ New Rejected
• ❌ (Ack) Rejected

Again, the “new” states are important because they highlight validation errors which haven't been dealt with yet. So the “new” here is synonymous with “unacknowledged”, “unread” or “unapproved” (which is used on the Comments screen):

On the Validated URL screen, here is what it currently looks like when a URL has validation errors in all 4 possible states:

Here is what it could look like if we replace the “new” with the concept of read/unread state, and rebrand “accepted” as “removed” and “rejected” as “kept” (while also rebranding “Status” to “Action”):

Mock 1

The semantics change here as instead of listing validation errors it is rather listing invalid markup which is causing validation errors. So instead of manipulating the status of whether or not a validation error should (have its underlying markup) be sanitized, it changes to what action should be performed with the invalid AMP markup.

Notice that there is now no longer any “New” in the dropdowns. This is moved to a “Mark as read” row action, which would behave similar to the “Approve” link for comments. It doesn't change the action, but just clears out the unmoderated state. There is also here a new capability, to mark a previously read error as unread (via the “Mark as unread“ row action link. This would provide a way for a user to essentially flag a validation error to revisit later, similar to what people do with marking emails as unread.

Clicking “mark as read” and “mark as unread” would not cause the page to reload (same as comments) and the user wouldn't have to click the Update to persist the changes. The only concern I have with this is the call to action for clearing out the “unread” is not as easy to find as perhaps should be. Maybe instead of “read”/“unread” there should be instead (following the email paradigm) be the concept of “starring”. There could be a new column with a star which the user could click to toggle the state, which would make this more discoverable, for example:

Mock 2

Or using unseen/seen instead of unstarred/starred:

Mock 3

Or going with a checkbox:

Mock 4

But I don't like this so much because there is already a checkbox in the first column for bulk actions, and checkboxes don't usually change state when clicked (which we'd be doing here), and it confuses with the other ✅/❌ iconography (which perhaps should also be revamped).

[westonruter]

Another alternative to seen/unseen is reviewed/unreviewed.

[westonruter]

Current Validated URL Screens

As for the list of Validated URLs, here is what it looks like presently:

The “new” state for the URL shown here is present because it has one or more validation errors that are in this new/unseen/unacknowledged/unapproved state.

Mocks for Updated Validated URL Screens

Something that is missing from this view is a column indicating whether AMP is enabled/disabled for the URL. This is implied by there being a validation error in the “Kept” state, but it could be more clear.

It's also not clear what “Actions” means here, or what specifically is being “Removed” or “Kept”.

[westonruter]

Current Validation Error Index Screen

Mocks for Updated Validated URL Screen

See discussion above about whether stars, eyeballs, or something else should be used to represent the acknowledged state.

[westonruter]

As an alternative to unseen/starred/unapproved, another workable concept would be flagged.

A validation error that not been reviewed yet, could get flagged by default, indicating that it is an issue that needs to be reviewed. This concept would also work for issues which actually have been seen before but which the user wants to put back into an “unread” state. There is a flag Dashicon:

But there is no corresponding “unflagged” dashicon similar to there is for filled/unfilled star or visible/hidden:

[westonruter]

Here's where @amedina and I have settled:

• Remove the “Seen” column entirely, in favor of the “Mark as seen” and “Mark as unseen” row actions.
• Rename the “Action” column to “Markup Status”.
• Changing the seen state of a validation error is persisted immediately upon clicking (as is done with Comments), whereas the removed/kept status is only persisted when clicking Update (when on the Validated URL screen).
• Use “Remove markup” and “Keep markup” for the action buttons and bulk action options.

[westonruter]

I think we need better icons to represent Removed and Kept:

The ✅ is currently used for removal because it has a positive result for AMP, in that it allows AMP to be served. The ❌ is used for keeping because it has a negative result for AMP, that it blocks AMP from being served for the given URL.

Possibilities:

The icon representing removal can be green, whereas keep can be red.

[westonruter]

Maybe it would make the most sense to just use the AMP logo? A red logo could be used for keep, whereas a green one for removal:

When a URL has any kept validation errors, then the URL as a whole would get the “AMP Disabled” gray badge in Transitional mode, whereas the orange badge in Standard mode. If a URL has no kept validation errors, then it should get the “AMP Enabled” blue badge.

[amedina]

I like the ides of using AMP logos with color differentiation.

The only aspect I am not fully convinced is the cases for AMP disabled Standard vs. Transitional modes. It might be better to have both of those cases represented with the gray symbol.

Although, I admit I like the orangee color used for the Standard mode case.

[westonruter]

@amedina Yeah, going with gray will make it simpler; we won't have to explain it isn't fully disabled.

Here's what it is looking like now:

[westonruter]

See all before/after screenshots in PR: #3630

[csossi]

Verified in QA