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.

Sign up for GitHub

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

Jump to bottom

Fix validation error reporting for invalid attributes on the HTML element #4190

Closed
wants to merge 1 commit into from
Closed

Fix validation error reporting for invalid attributes on the HTML element #4190

wants to merge 1 commit into from

Conversation

westonruter
Copy link
Member

Summary

In #2500 the admin bar was suppressed from being shown when doing validation requests. Then in #3187 this was reverted because the addition of dev mode meant that the presence of the admin bar would no longer make pages invalid. Nevertheless, this had an unexpected consequence. When the <html> element has invalid attributes, these are never detected during validation because a validation request is made as an authenticated user, and thus the admin bar is present, and the admin bar entails dev mode being enabled. Since the data-ampdevmode attribute is on the html element, any invalid attributes on the element are ignored for validation purposes. This is not expected.

For example, activate Twenty Twenty and add this plugin code:

add_filter(
	'language_attributes',
	function ( $attr ) {
		if ( ! is_admin() ) {
			$attr .= ' amp-version="1902151859190" onclick="alert(\'hello\')" ';
		}
		return $attr;
	}
);

This adds two invalid AMP attributes, and when I look at an AMP page while logged-in, I see them:

<html class="no-js" lang="en-US" amp-version="1902151859190" onclick="alert('hello')" amp="" data-ampdevmode="">

Nevertheless, the admin bar shows ✅ for validity.

However, if I access the same AMP page while being logged-out, I see:

<html class="no-js" lang="en-US" amp="">

Similarly, if I go to validate the URL I see no validation errors:

image

This doesn't seem right.

To prevent this from happening, we can prevent showing the admin bar on the frontend as we were doing before. When that is done, the result when validating is:

image

This reverts commit 3380451 from #3187.

Todo

  • What if someone is forcing the admin bar to be shown on the frontend even for unauthenticated users? Should the admin bar not be excluded in this case?
  • Will suppressing the admin bar make it more difficult to reconcile validation errors that occur on the frontend while logged-in but validation is is done with the admin bar suppressed?
  • Should dev mode be filtered off in addition to the admin bar being excluded?

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.5 milestone Jan 29, 2020
@googlebot googlebot added the cla: yes Signed the Google CLA label Jan 29, 2020
@westonruter westonruter mentioned this pull request Jan 30, 2020
Merged
3 tasks
@westonruter
Copy link
Member Author

A better solution here may be to capture whether the html element originally had data-ampdevmode and if so, ignore validation errors. Otherwise if it was added by the plugin to enable dev mode, then go ahead and report validation errors on the element even though it has the attribute. This would be a special case unique for the html element.

@westonruter westonruter mentioned this pull request Mar 14, 2020
Closed
@westonruter westonruter removed this from the v1.5 milestone Mar 14, 2020
@westonruter
Copy link
Member Author

Closing in favor of #4387.

@westonruter westonruter deleted the fix/document-element-validation branch March 14, 2020 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes Signed the Google CLA Validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@westonruter @googlebot