1565384 - object-fit for <embed> and <object> is unreliable when running over http

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

bug 1381855 disabled a bunch of tests in the WPT reftest harness when those tests work fine in the main reftest harness. It turns out the substantive difference here appears to be loading the tests over file: vs. http:. Loading them over http: in the browser produces intermittent results, at least when e10s is enabled. (I didn't check that bit in the browser, but I know that in the wpt harness they seem OK with e10s disabled.)

Steps to reproduce:

1. run ./mach wpt testing/web-platform/tests/css/vendor-imports/mozilla/mozilla-central-reftests/images3/object-fit-cover-svg-003o.html (don't use --no-pause, so the server stays running)
2. copy the URL out of the output and load it in the browser
3. reload a few times

Actual results: the later images in the test don't get the correct scaling; it varies how many at the end are wrong.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

When this is fixed we should undo the changes in bug 1381855.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The first thing that looks like it's wrong in the broken cases is that nsSubDocumentFrame::GetSubdocumentSize is calling nsLayoutUtils::ComputeObjectDestRect and the parameter than looks wrong is that the intrinsic ratio is 0 rather than 2. (That also matches the display, which is showing a ratio that I think matches the 300x150 default intrinsic ratio of 0.5.)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... also call to the same function from nsSubDocumentFrame::Reflow.

It seems like in the bad cases, the result of ObtainIntrinsicSizeFrame is null.

So, a question: what's supposed to cause the things in the parent document to get invalidated when the child document loads sufficiently to have its intrinsic ratio, or if the content inside changes dynamically?

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #3)

So, a question: what's supposed to cause the things in the parent document to get invalidated when the child document loads sufficiently to have its intrinsic ratio, or if the content inside changes dynamically?

I don't remember, but I'll take a look & can take this from here. Thanks for identifying this as the cause of those test failures!

Comment 5

[Daniel Holbert [:dholbert]]

So if the content changes dynamically (via scripted "setAttribute") in a way that affects the intrinsic ratio, we handle it here in nsSVGOuterSVGFrame::AttributeChanged (correctly, based on some testing [EDIT: not correctly after all, see comment 6]):
https://searchfox.org/mozilla-central/rev/15be167a5b436b57fef944b84eef061d24c1af8c/layout/svg/nsSVGOuterSVGFrame.cpp#695-707

Notably, the inner doc notifies the host frame that it needs a reflow here:

          embeddingFrame->PresShell()->FrameNeedsReflow(
              embeddingFrame, IntrinsicDirty::StyleChange, NS_FRAME_IS_DIRTY);

I would imagine that same codepath is supposed to be triggered for slowly-loading SVG content as well, but I'll need to check that.

Comment 6

[Daniel Holbert [:dholbert]]

Ah, so I bet the problem is that this FrameNeedsReflow call is guarded by:

  if (DependsOnIntrinsicSize(embeddingFrame)) {

...which probably returns "false" for this testcase, because we have a specified px-valued width and height on the embedding element.

So we skip the FrameNeedsReflow call.

Really, we probably need to add something like the following (these functions aren't real):

else if (CaresAboutIntrinsicRatio(embeddingFrame)) { embeddingFrame->RequestRepaint(...) }

...or somesuch.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: (svg for use in testcase)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (with dynamic SVG)

Here's a testcase. Expected rendering is yellow/blue, no red visible.

Firefox produces expected rendering after you resize the window (forcing a reflow/repaint).

(Don't use Chrome as a reference here -- they show red before + after the dynamic change, which is broken. I think they don't properly support object-fit on <embed> -- if I use img instead, their rendering changes to hide the red. [which is the correct no-dynamic-change rendering])

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, for what it's worth, this made https://github.com/web-platform-tests/wpt/pull/17784 difficult to merge.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1565384 part 1: When an embedded SVG document has an aspect-ratio change, trigger a reflow if 'object-fit' is set on the embedding element. r?emilio

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1565384 part 0: Remove some single-use helper-variables that don't add value. r?emilio

This patch doesn't affect behavior; it's just minor code cleanup.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 1565384 part 2: Change 'disabled' WPT annotations to expected-fails, for object-fit tests. r?boris

These tests are expected-fails due to using some obsolete positioning syntax.
They will become expected-pass once our "upstream" test-fixes in Bug 1559276
finish off their synchronization roundtrip back into our WPT import.

Depends on D37910

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b29078312200
part 0: Remove some single-use helper-variables that don't add value. r=emilio
https://hg.mozilla.org/integration/autoland/rev/0c49c78a3216
part 1: When an embedded SVG document has an aspect-ratio change, trigger a reflow if 'object-fit' is set on the embedding element. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8231ec901388
part 2: Change 'disabled' WPT annotations to expected-fails, for object-fit tests. r=boris

Comment 14

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/b29078312200
https://hg.mozilla.org/mozilla-central/rev/0c49c78a3216
https://hg.mozilla.org/mozilla-central/rev/8231ec901388

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

WPT import in https://github.com/web-platform-tests/wpt/pull/17886

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The test sync to our WPT copy and the expectations update have now landed... although there are a bunch of remaining bug lines, and also a bunch of the PNG tests that still apparently fail with non-webrender.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #16)

although there are a bunch of remaining bug lines

True -- I think many of those files (all the .ini files that just have a bug line as the only annotation) are now stubs that don't do anything, & they can be removed.

Like this one:
https://hg.mozilla.org/mozilla-central/annotate/dbb4eb5782783c4bb3fb72169617741e0e78ec53/testing/web-platform/meta/css/vendor-imports/mozilla/mozilla-central-reftests/images3/object-fit-contain-png-001e.html.ini

I can take care of that.

and also a bunch of the PNG tests that still apparently fail with non-webrender.

Yeah -- we perhaps need to revert those .ini files their pre-bug 1559276 state (when they had some specific PASS conditions):
https://searchfox.org/mozilla-central/rev/1f9cb879fe48956c4a110e3a96c3e46dc03ddaca/testing/web-platform/meta/css/vendor-imports/mozilla/mozilla-central-reftests/images3/object-fit-cover-png-001c.html.ini#3
...and/or annotate them as fuzzy instead of failing, if appropriate, depending on how the failure actually looks. (I suspect the pre-bug 1559276 failure annotation predated our support for fuzzy annotations.)

Comment 18

[Daniel Holbert [:dholbert]]

I've now fixed the "bug" dummy-files that dbaron mentioned in bug 1569263.

I've filed bug 1569345 on adjusting the annotation for the PNG test failures that dbaron mentioned, to better reflect reality. (They pass in our own copy of the test, which has image-rendering: -moz-crisp-edges, but the WPT version has image-rendering: pixelated instead, which doesn't have any effect for us right now. So these tests do fuzzy-scaling and have a few fuzzy pixels, which is why they fail.)