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

core(doctype): check document.compatMode for quirks mode #12978

Merged
merged 14 commits into from
Apr 26, 2022
Merged

core(doctype): check document.compatMode for quirks mode #12978

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a043953
`quirks-mode` check is added in doctype audit
k99sharma Aug 24, 2021
43cbe87
doctype test is altered for quirks-mode check
k99sharma Aug 24, 2021
033258a
Delete artifacts.d.ts
k99sharma Aug 24, 2021
364a5ca
compatMode is added in doctype interface
k99sharma Aug 24, 2021
11f10b9
Merge branch 'quirk-mode-check' of https://github.com/k99sharma/light…
k99sharma Aug 24, 2021
b1ffee6
Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…
k99sharma Aug 26, 2021
4ee3671
Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…
k99sharma Aug 28, 2021
7bdbf93
Merge remote-tracking branch 'origin/master' into quirk-mode-check
paulirish Apr 25, 2022
7f7e3aa
rename compat mode prop, since its not a field of doc.doctype like th…
paulirish Apr 25, 2022
bf8ef42
more precise test matching simon's report
paulirish Apr 25, 2022
7f5b3c0
retitle string
paulirish Apr 25, 2022
4ea416a
string
paulirish Apr 25, 2022
43cea0d
more realistic test mocks
paulirish Apr 25, 2022
b82edc4
reorde the doctype checks to be what i intended
paulirish Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 16 additions & 4 deletions lighthouse-core/audits/dobetterweb/doctype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const UIStrings = {
'[Learn more](https://web.dev/doctype/).',
/** Explanatory message stating that the document has no doctype. */
explanationNoDoctype: 'Document must contain a doctype',
/** Explanatory message stating that the document has wrong doctype */
explanationWrongDoctype: 'Document contains a doctype that triggers quirks-mode',
/** Explanatory message stating that the publicId field is not empty. */
explanationPublicId: 'Expected publicId to be an empty string',
/** Explanatory message stating that the systemId field is not empty. */
Expand Down Expand Up @@ -59,6 +61,7 @@ class Doctype extends Audit {
const doctypeName = artifacts.Doctype.name;
const doctypePublicId = artifacts.Doctype.publicId;
const doctypeSystemId = artifacts.Doctype.systemId;
const compatMode = artifacts.Doctype.documentCompatMode;

if (doctypePublicId !== '') {
return {
Expand All @@ -76,14 +79,23 @@ class Doctype extends Audit {

/* Note that the casing of this property is normalized to be lowercase.
see: https://html.spec.whatwg.org/#doctype-name-state */
if (doctypeName === 'html') {
if (doctypeName !== 'html') {
return {
score: 1,
score: 0,
explanation: str_(UIStrings.explanationBadDoctype),
};
} else {
}

// Catch-all for any quirks-mode situations the above checks didn't get.
// https://github.com/GoogleChrome/lighthouse/issues/10030
if (compatMode === 'BackCompat') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we should just only check this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... I guess the specific messages are better for diagnosing. nevermind.

return {
score: 0,
explanation: str_(UIStrings.explanationBadDoctype),
explanation: str_(UIStrings.explanationWrongDoctype),
};
} else {
return {
score: 1,
};
}
}
Expand Down
6 changes: 4 additions & 2 deletions lighthouse-core/gather/gatherers/dobetterweb/doctype.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,18 @@ const FRGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
/**
* Get and return `name`, `publicId`, `systemId` from
* `document.doctype`
* @return {{name: string, publicId: string, systemId: string} | null}
* and `compatMode` from `document` to check `quirks-mode`
* @return {{name: string, publicId: string, systemId: string, documentCompatMode: string} | null}
*/
function getDoctype() {
// An example of this is warnerbros.com/archive/spacejam/movie/jam.htm
if (!document.doctype) {
return null;
}

const documentCompatMode = document.compatMode;
const {name, publicId, systemId} = document.doctype;
return {name, publicId, systemId};
return {name, publicId, systemId, documentCompatMode};
}

class Doctype extends FRGatherer {
Expand Down
19 changes: 19 additions & 0 deletions lighthouse-core/test/audits/dobetterweb/doctype-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,28 @@ describe('DOBETTERWEB: doctype audit', () => {
expect(auditResult.explanation).toBeDisplayString('Document must contain a doctype');
});

it('fails when document is in quirks-mode (but passes other checks)', () => {
const auditResult = Audit.audit({
// eg `<!DOCTYPE html foo>`. https://github.com/GoogleChrome/lighthouse/issues/10030
Doctype: {
name: 'html',
publicId: '',
systemId: '',
documentCompatMode: 'BackCompat',
},
});
assert.equal(auditResult.score, 0);
expect(auditResult.explanation)
.toBeDisplayString('Document contains a doctype that triggers quirks-mode');
});

it('fails when the value of the name attribute is a value other than "html"', () => {
const auditResult = Audit.audit({
Doctype: {
name: 'xml',
publicId: '',
systemId: '',
documentCompatMode: 'BackCompat',
},
});
assert.equal(auditResult.score, 0);
Expand All @@ -38,6 +54,7 @@ describe('DOBETTERWEB: doctype audit', () => {
name: 'html',
publicId: '189655',
systemId: '',
documentCompatMode: 'BackCompat',
},
});
assert.equal(auditResult.score, 0);
Expand All @@ -50,6 +67,7 @@ describe('DOBETTERWEB: doctype audit', () => {
name: 'html',
publicId: '',
systemId: '189655',
documentCompatMode: 'BackCompat',
},
});
assert.equal(auditResult.score, 0);
Expand All @@ -62,6 +80,7 @@ describe('DOBETTERWEB: doctype audit', () => {
name: 'html',
publicId: '',
systemId: '',
documentCompatMode: 'CSS1Compat',
},
});
assert.equal(auditResult.score, 1);
Expand Down
3 changes: 3 additions & 0 deletions shared/localization/locales/en-US.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions shared/localization/locales/en-XL.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions types/artifacts.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ declare module Artifacts {
name: string;
publicId: string;
systemId: string;
documentCompatMode: string;
}

interface DOMStats {
Expand Down