Closed Bug 1724243 Opened 3 years ago Closed 3 years ago

Implement no-quirks mode for text/plugin/media/ua-inline documents

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
94 Branch
Tracking Flags:
Tracking Status
firefox94 --- fixed

People

(Reporter: masonfreed, Assigned: hsivonen)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

Attachments

(1 file)

Bug 1724243 - Make text/plain and MediaDocuments use the Standards Mode.
48 bytes, text/x-phabricator-request
Details | Review

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

Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Blocks: html
Status: UNCONFIRMED → NEW
Ever confirmed: true

MediaDocuments could just override the default, but text/plain requires changes to the parser.
(does HTML validator need updates too?)

Severity: -- → S3
Flags: needinfo?(hsivonen)
Priority: -- → P3

We need to make the plain text case set nsHtml5TreeBuilder::isSrcdocDocument to true (and we should rename that flag).

Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)

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

(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.

Flags: needinfo?(dholbert)

(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.

(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.

Flags: needinfo?(dholbert)

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.)

(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.)

(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.

(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

(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

Hmm. dom/html/test/test_bug369370.html fails on Windows and Mac but not on Linux.

(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.

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");

https://searchfox.org/mozilla-central/rev/b022ae1fc071ad7a29f64f281bc19b7b093df538/dom/html/test/test_bug369370.html#110-113

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.

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.

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.

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

https://hg.mozilla.org/mozilla-central/rev/2be0d111fd0a

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox94: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
Regressions: 1740066
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: