Implement no-quirks mode for text/plugin/media/ua-inline documents
Categories
(Core :: DOM: Core & HTML, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox94 | --- | fixed |
People
(Reporter: masonfreed, Assigned: hsivonen)
References
(Blocks 1 open bug, Regressed 1 open bug)
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/94.0.4590.0 Safari/537.36
Steps to reproduce:
The spec has recently changed for these document types, to indicate that they should be standards-mode documents instead of quirks-mode documents.
https://github.com/whatwg/html/pull/6745
The implementation should change accordingly.
These three WPTs test some of this behavior:
https://wpt.fyi/results/html/browsers/browsing-the-web/read-media/pageload-video.html
https://wpt.fyi/results/html/browsers/browsing-the-web/read-media/pageload-image.html
https://wpt.fyi/results/html/browsers/browsing-the-web/read-text/load-text-plain.html
Also, this existing bugs tracks these WPTs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1715432
Updated•3 years ago
|
Updated•3 years ago
|
Comment 1•3 years ago
|
||
MediaDocuments could just override the default, but text/plain requires changes to the parser.
(does HTML validator need updates too?)
Assignee | ||
Comment 2•3 years ago
|
||
We need to make the plain text case set nsHtml5TreeBuilder::isSrcdocDocument
to true
(and we should rename that flag).
Assignee | ||
Comment 3•3 years ago
|
||
Assignee | ||
Comment 4•3 years ago
|
||
Assignee | ||
Comment 5•3 years ago
|
||
Need to check out these:
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/image/image-in-iframe-1.html == layout/reftests/image/image-in-iframe-1-ref.html | image comparison, max difference: 196, number of differing pixels: 612
REFTEST TEST-UNEXPECTED-FAIL | dom/html/reftests/741776-1.vtt == dom/html/reftests/741776-1-ref.html | image comparison, max difference: 255, number of differing pixels: 68
Assignee | ||
Comment 6•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #5)
REFTEST TEST-UNEXPECTED-FAIL | layout/reftests/image/image-in-iframe-1.html == layout/reftests/image/image-in-iframe-1-ref.html | image comparison, max difference: 196, number of differing pixels: 612
The problem here is that the standards mode makes a scrollbar appear. The test case came from bug 1701510. Hence, needinfoing dholbert.
Assignee | ||
Comment 7•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #5)
REFTEST TEST-UNEXPECTED-FAIL | dom/html/reftests/741776-1.vtt == dom/html/reftests/741776-1-ref.html | image comparison, max difference: 255, number of differing pixels: 68
Fixed this one by adding a doctype to the reference.
Comment 8•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #6)
The problem here is that the standards mode makes a scrollbar appear. The test case came from bug 1701510. Hence, needinfoing dholbert.
This is a real test-failure, and it's indicating that the image document is more than 51px tall (which is odd/unwanted, since it only contains a 50px-tall-image). This has something to do with inline layout / baseline-alignment, since the image document is really just an img element which is display:inline by default, and the inline-level image element tries to do baseline-alignment within its line, and the line ends up being a little bit taller than the content itself.
Here's a reduced testcase demonstrating this (using a canvas instead of an image). This is basically a reduced version of the image document:
data:text/html,<!DOCTYPE html><div style="overflow:auto;height:51px;width:51px"><canvas height=50px width=50px>
(This produces a scrollbar in both Firefox and Chrome, so we're not doing anything non-interoperable here, layout-wise.)
To avoid having scrollbars here, we need the <img>
inside of the image document to have either display:block
or vertical-align:top
, I think. Either one of these will avoid the baseline-alignment that produces weird height effects.
Comment 9•3 years ago
|
||
So e.g. we might want to just remove the @media print
wrapper here:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/layout/style/ImageDocument.css#28
I'm not sure if that would break any other tests; that's worth testing.
If it does break anything, then img { vertical-align:top }
is probably a slightly more targeted fix with fewer potential side effects. (There's definitely no reason we'd need to be doing baseline-alignment on the line box inside of an image document, which is what the default vertical-align
behavior is attempting to give us.)
Comment 10•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #8)
This is a real test-failure, and it's indicating that the image document is more than 51px tall (which is odd/unwanted, since it only contains a 50px-tall-image). This has something to do with inline layout / baseline-alignment, since the image document is really just an img element which is display:inline by default, and the inline-level image element tries to do baseline-alignment within its line, and the line ends up being a little bit taller than the content itself.
(BTW, this "works" in current builds -- i.e. image iframe documents don't overflow beyond their image's height in current builds -- because the image document is in quirks-mode, and quirks-mode has different rules for line-box-height computation. I suspect https://quirks.spec.whatwg.org/#the-blocks-ignore-line-height-quirk is the relevant quirk here, though I don't have a recent enough memory of the intricacies of line layout to know for sure.)
Comment 11•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #9)
So e.g. we might want to just remove the
@media print
wrapper here:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/layout/style/ImageDocument.css#28
(i.e. giving images display:block
unconditionally)
Historical note RE the safety of doing this (styling these images as display:block
) & why it seems likely-fine:
(1) We used to have an unconditional display:block
for img
in top-level-image-documents in a .cpp file, to address a print-specific thing. This essentially dates back to bug 528046 comment 10, and it ended up in TopLevelImageDocument.css in this later commit:
https://hg.mozilla.org/mozilla-central/rev/b77ec6e8eb8c2f182adf92e25280551c86c2dc51#l2.20
(2) Later on we accidentally guarded it inside of @media not print{ }
(even though it was intended to address a print-specific issue)
https://hg.mozilla.org/mozilla-central/rev/e1d850a8ee2e14dafff96f727e85ea79a4084c43#l1.47
(3) Then someone noticed that and moved it to a @media print{ }
block:
https://hg.mozilla.org/mozilla-central/rev/4bb2bcc8c95aa6ae4b4c672931421f220983b3aa#l1.39
...and it's been guarded by that print-specific media query since then.
But there never seemed to be any issues with having it unguarded, and I think it's only guarded because we inadvertently grouped it with other styles that needed guarding in (2) here.
So: I suspect it's fine to just unconditionally have img { display:block}
in ImageDocument.css.
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #11)
So: I suspect it's fine to just unconditionally have
img { display:block}
in ImageDocument.css.
Thanks. Try run with that change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=983c8dc4c485812dd7e9ae777b03003bbb1455a6
Assignee | ||
Comment 13•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #12)
(In reply to Daniel Holbert [:dholbert] from comment #11)
So: I suspect it's fine to just unconditionally have
img { display:block}
in ImageDocument.css.Thanks. Try run with that change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=983c8dc4c485812dd7e9ae777b03003bbb1455a6
Looks OK. With more platforms and test suites:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5756d2cd830ba4c0068e240b7d49da68e9055de8
Assignee | ||
Comment 14•3 years ago
|
||
Hmm. dom/html/test/test_bug369370.html fails on Windows and Mac but not on Linux.
Assignee | ||
Comment 15•3 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #14)
Hmm. dom/html/test/test_bug369370.html fails on Windows and Mac but not on Linux.
Because it's disabled on Linux: bug 1258103.
Comment 16•3 years ago
|
||
The error there is:
TEST-UNEXPECTED-FAIL | dom/html/test/test_bug369370.html | Image is vertically centered - got 16.5, expected -616.5
That comes from this piece of the test:
is(img.height, 600, "image height");
var bodyHeight = kidDoc.body.scrollHeight;
var imgRect = img.getBoundingClientRect();
is(imgRect.top, bodyHeight - imgRect.bottom, "Image is vertically centered");
If I'm understanding correctly, the test-failure here means that:
(a) bodyHeight
is 0
(b) we are apparently placing the image at y=16.5px
(not sure why...)
(c) the test is implicitly expecting us to place the image at -300px instead so that it's vertically "centered" in our 0px-tall scrollable area.
I think the main issue here is that we are not expecting bodyHeight
to be 0, given that this code gets kicked off by a kidWin.resizeTo(400, 600 + 50 + decorationSize);
, and the header says we're resizing the window "so the image fits vertically". And given that it's the scrollable area in a document that supposedly contains a 600px-tall image. So something is odd here.
I added some logging and think I got to the bottom if it; more coming in a subsequent comment.
Comment 17•3 years ago
|
||
So it looks like the test is revealing another observable behavior-difference between quirks & standards mode, which happens to be observable in image documents -- abspos content contributes to document.body.scrollHeight
in quirks mode, but does not in standards mode.
Compare these testcases to see:
data:text/html,<canvas style="background:purple;position:absolute"></canvas><script>alert(document.body.scrollHeight)</script>
data:text/html,<!DOCTYPE html><canvas style="background:purple;position:absolute"></canvas><script>alert(document.body.scrollHeight)</script>
The former alerts something nonzero; the latter alerts 0. (I'm using position:absolute
since top-level-image-documents have that styling from https://searchfox.org/mozilla-central/rev/b022ae1fc071ad7a29f64f281bc19b7b093df538/layout/style/TopLevelImageDocument.css#14 )
I think this just pointing at some of the body-vs-html-element hand-waviness of quirks-mode documents. In standards-mode, the abspos image (or <canvas>
in my data-URI testcase) uses the root node as its containing block, which means it doesn't contribute to the scrollHeight of the body.
Comment 18•3 years ago
•
|
||
Bottom-line, I think you just need to adjust the snippet of the test that I quoted above with s/kidDoc.body/kidDoc.documentElement/
That seems to fix the test-failure locally for me, at least. And it's a more-correct way of asking the child document what its scrollable overflow area is (which is really what we're trying to get at here). kidDoc.body
is empty as of your patch (from a frame tree perspective) which is why it has scrollHeight
of 0 and is causing us trouble here; but the overall rendering is unchanged, I think.
Assignee | ||
Comment 19•3 years ago
|
||
Comment 20•3 years ago
|
||
Pushed by hsivonen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2be0d111fd0a Make text/plain and MediaDocuments use the Standards Mode. r=smaug,emilio
Comment 21•3 years ago
|
||
bugherder |
Assignee | ||
Comment 22•3 years ago
|
||
Description
•