-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core(doctype): check for limited quirks mode #14576
Conversation
@@ -41,6 +43,7 @@ class Doctype extends Audit { | |||
failureTitle: str_(UIStrings.failureTitle), | |||
description: str_(UIStrings.description), | |||
requiredArtifacts: ['Doctype'], | |||
__internalOptionalArtifacts: ['InspectorIssues'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use this outside the experimental config, maybe drop the __internal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not until FR does a better job re: scheduling. Also that's a bigger change :/
811ec89
to
5be976f
Compare
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
5be976f
to
e92e3e3
Compare
@@ -3299,10 +3299,11 @@ | |||
}, | |||
"doctype": { | |||
"id": "doctype", | |||
"title": "Page has the HTML doctype", | |||
"title": "Page lacks the HTML doctype, thus triggering quirks-mode", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the doctype artifact is out of date on the flow artifacts:
yarn update:flow-sample-json --rebaseline-artifacts Doctype
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -295067,7 +295067,8 @@ | |||
"Doctype": { | |||
"name": "html", | |||
"publicId": "", | |||
"systemId": "" | |||
"systemId": "", | |||
"documentCompatMode": "CSS1Compat" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only for one flow step, shouldn't there be more
2 navigations and a snapshot so 3 total
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose! The update script didn't work so I just updated one thing 🙈
fixes #14570
We can't determine limited-quirks-mode without doing a bunch of work ourselves; and the only protocol is via inspector issues which is not snapshot friendly but given this is a very minor edge case, it seems ok to use an optional artifact here. FR doesn't know how to schedule such artifacts, but the runner still passes to the audit when possible, so in the most common cases (not filtering to just doctype audit, or excluding all audits that require InspectorIssues) this should just work. And when it doesn't... whatever?
Without knowing for sure if something is no-quirks-mode, we will always pass the audit.