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.

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

Make text/plugin/media/ua-inline no-quirks documents #6745

Merged
merged 8 commits into from
Jun 24, 2021

Conversation

mfreed7
Copy link
Contributor

@mfreed7 mfreed7 commented Jun 7, 2021

This change moves these four document types over to be "standards mode" documents.

To implement this, a new Document flag called "parser cannot change the mode" is added to the Document. This flag is checked before the parser tries to change the mode. If the flag is true, no change will be made.

Closes #5939, Closes #3113


/browsing-the-web.html ( diff )
/parsing.html ( diff )

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nits.

I don't know what category about:blank falls under (either HTML or ua-inline) but it'd be good to test that as well.

For plugin documents it's worth seeing what people do for same-origin PDFs and whether they actually generate documents with <embed>. We might end up deleting that section or converting it into something else as part of the general plugins-are-only-PDFs-these-days deprecation...

source Show resolved Hide resolved
@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 7, 2021

Thanks for the quick review!

The about:blank document isn't included here, as I would be afraid of compat issues with that one, given its usage. The spec there is already explicitly quirks-mode: https://html.spec.whatwg.org/#creating-a-new-browsing-context. Step 13 says to mark the document quirks-mode.

I'll see what I can do about pdf/embed tests. You're right that a same-origin PDF should be testable, but that does assume all browsers support PDFs. While true, I don't think it's specified, so writing a WPT feels funny.

I've got the easy ones (text/media) up in a patch now.

@domenic
Copy link
Member

domenic commented Jun 7, 2021

I did a quick test at https://selective-heliotrope-dumpling.glitch.me/pdf-iframe.html and it looks like for PDFs, Chrome uses the spec's "Page load processing model for content that uses plugins" (with some minor embellishments), while Firefox uses "Page load processing model for inline content that doesn't have a DOM".

Notably, the latter case has an opaque origin and thus cannot be tested. But the former case is same-origin.

So I think a test for this would embed a PDF and pass either if accessing frames[0].document throws (in which case it's using ua-inline for PDFs) or if frames[0].document.compatMode is CSS1Compat.

Also it'd be ideal to test the difference between no-quirks and limited-quirks but I'm not sure exactly how to do that....

@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 7, 2021

Thanks for the quick test! I made it into a WPT using your suggestion to just pass in the Firefox case. WebKit currently fails this new test.

source Show resolved Hide resolved
@josepharhar
Copy link
Contributor

The spec there is already explicitly quirks-mode: https://html.spec.whatwg.org/#creating-a-new-browsing-context

Oh boy, maybe I should stop using about:blank to fiddle with javascript...

jatin837 pushed a commit to jatin837/chromium that referenced this pull request Jun 7, 2021
The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2021
The [1] PR will change the standard so that plugin documents
are in standard (no quirks) mode. This CL adds a test, as
much as this is testable. Some UAs display PDF documents
using "content that uses plugins" [2] while others use
"inline content that doesn't have a DOM [3]. This test will
pass in either case.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745
[2] https://html.spec.whatwg.org/#read-plugin
[3] https://html.spec.whatwg.org/#read-ua-inline

Bug: 1131185
Change-Id: Iccd1668199ac8b3b1c90a69b97a0adbaf8d02655
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 7, 2021
The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2021
The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Jun 8, 2021
The [1] PR will change the standard so that plugin documents
are in standard (no quirks) mode. This CL adds a test, as
much as this is testable. Some UAs display PDF documents
using "content that uses plugins" [2] while others use
"inline content that doesn't have a DOM [3]. This test will
pass in either case.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745
[2] https://html.spec.whatwg.org/#read-plugin
[3] https://html.spec.whatwg.org/#read-ua-inline

Bug: 1131185
Change-Id: Iccd1668199ac8b3b1c90a69b97a0adbaf8d02655
@domenic domenic self-requested a review June 9, 2021 21:53
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I'll note an alternate version here which might not require as many changes to DOM would be:

source Outdated
on <var>document</var>, with <var>new mode</var> set to <code data-x="">no-quirks</code>.</p></li>

<li><p>Set <var>document</var>'s <span data-x="concept-document-mode-locked">mode locked</span>
flag to <code data-x="">true</code>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessary for the non-text cases, I don't think...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ok - we currently lock the mode for all of these documents, so I just implemented the spec the same way. Any harm in keeping it locked for all document types?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's pretty confusing for readers to add something to the spec that in actuality has no impact.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 9, 2021

I'll note an alternate version here which might not require as many changes to DOM would be:

Hmm, so you'd add this in the HTML spec, not in DOM? I saw this comment after I ended up filing the DOM PR. The biggest changes are here in HTML, and the DOM change is pretty minimal. So let me know what you think as-is.

@domenic
Copy link
Member

domenic commented Jun 10, 2021

I'll let @annevk make the call as to whether layering this throughout both HTML & DOM is worthwhile, versus keeping this specific to the HTML parser.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 15, 2021
…documents, a=testonly

Automatic update from web-platform-tests
Modify WPTs to check for standards mode documents

The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}

--

wpt-commits: ce674ef72677377bc04c08c314a753c9274e3982
wpt-pr: 29269
@annevk
Copy link
Member

annevk commented Jun 17, 2021

I hadn't read this discussion when I read the DOM PR. From this discussion it seems that over time HTML might not use the HTML parser for these type of documents in which case this problem would not come up, right? From that perspective a solution local to HTML seems preferable, as it can be more easily refactored over time.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 22, 2021

I'll note an alternate version here which might not require as many changes to DOM would be:

Alright, I updated the PR to use this suggestion instead of making changes to DOM. I'm not sure I added the flag to Document in the right spot, it feels kind of tossed in. Let me know if there's a better spot for it. I also updated the non-normative descriptions for parse errors to be correct, but I'm not sure we need to do that? And finally, I removed the "mode locks" from all other document types except text mode.

@annevk and @domenic, PTAL, thanks!

@mfreed7 mfreed7 changed the title Make text/plugin/media/ua-inline no-quirks documents Make text documents use no-quirks mode Jun 22, 2021
@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 22, 2021

Hmm, upon a re-read, I think I misunderstood this comment:

This is not necessary for the non-text cases, I don't think...

I believe we still do need to change the mode, you were just saying we didn't need to "lock" it, since the parser doesn't try to change it. If that's correct, I can add back setting the mode to no-quirks for plugin/media/ua-inline documents.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I believe we still do need to change the mode, you were just saying we didn't need to "lock" it, since the parser doesn't try to change it. If that's correct, I can add back setting the mode to no-quirks for plugin/media/ua-inline documents.

Yeah, I think per your reasoning at #6745 (comment), we indeed still need to change the mode.

source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@mfreed7
Copy link
Contributor Author

mfreed7 commented Jun 22, 2021

Yeah, I think per your reasoning at #6745 (comment), we indeed still need to change the mode.

Ok, I put those three back. And address the rest of your comments. Let me know if I missed anything, thanks!

@mfreed7 mfreed7 changed the title Make text documents use no-quirks mode Make text/plugin/media/ua-inline no-quirks documents Jun 22, 2021
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM!

Can you update the OP with a link to the tests that have been updated for text and media? It's fine to not do plugin and ua-inline.

Similarly go ahead and file bugs on WebKit and Gecko.

@annevk want to give things a final look? If not I'll merge tomorrow; everything seems nice to me.

jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jun 23, 2021
…documents, a=testonly

Automatic update from web-platform-tests
Modify WPTs to check for standards mode documents

The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}

--

wpt-commits: ce674ef72677377bc04c08c314a753c9274e3982
wpt-pr: 29269
@domenic domenic self-assigned this Jun 23, 2021
@domenic domenic merged commit 0564169 into whatwg:main Jun 24, 2021
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
The [1] PR will change the standard so that media and
text documents are in standard (no quirks) mode. This
CL updates the tests accordingly.

Chromium already made this change in code in Sept 2020
(see CLs on crbug.com/1131185) and found no compat
issues.

[1] whatwg/html#6745

Bug: 1131185
Change-Id: Ie5f5ae5eb802008bf27794c152620bdcc9948ba2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2945338
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#889933}
NOKEYCHECK=True
GitOrigin-RevId: 30f7aa86969987278f7e7ff98ed0a855a789fbeb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants