Closed Bug 1565384 Opened 4 months ago Closed 4 months ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: dbaron, Assigned: dholbert)

References

Details

Attachments

(5 files)

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.

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

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

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

Flags: needinfo?(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!

Assignee: nobody → 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.

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.

Flags: needinfo?(dholbert)

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

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

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

Attachment #9077761 - Attachment description: Bug 1565384: For embedded SVG documents with non-default 'object-fit', be sure to update the view when the aspect ratio changes. → Bug 1565384 part 1: When an embedded SVG documents has an aspect-ratio change, trigger a reflow if 'object-fit' is set on the embedding element. r?emilio

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

Attachment #9077761 - Attachment description: Bug 1565384 part 1: When an embedded SVG documents has an aspect-ratio change, trigger a reflow if 'object-fit' is set on the embedding element. r?emilio → 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
Pushed by dholbert@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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.

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

Flags: needinfo?(dholbert)
Blocks: 1569263
Blocks: 1569345

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

Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.