object-fit for <embed> and <object> is unreliable when running over http
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox70 | --- | fixed |
People
(Reporter: dbaron, Assigned: dholbert)
References
Details
Attachments
(5 files)
(svg for use in testcase)
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:
- 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) - copy the URL out of the output and load it in the browser
- 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.
|
Reporter | |
Comment 1•5 years ago
|
||
When this is fixed we should undo the changes in bug 1381855.
|
|
Reporter | |
Comment 2•5 years ago
|
||
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.)
|
|
Reporter | |
Comment 3•5 years ago
|
||
... 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?
|
Assignee | |
Comment 4•5 years ago
|
||
(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 | |
Comment 5•5 years ago
•
|
||
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.
|
|
Assignee | |
Comment 6•5 years ago
•
|
||
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.
|
|
Assignee | |
Comment 7•5 years ago
|
||
|
|
Assignee | |
Comment 8•5 years ago
|
||
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])
|
|
Reporter | |
Comment 9•5 years ago
|
||
Also, for what it's worth, this made https://github.com/web-platform-tests/wpt/pull/17784 difficult to merge.
|
|
Assignee | |
Comment 10•5 years ago
|
||
|
|
Assignee | |
Comment 11•5 years ago
|
||
This patch doesn't affect behavior; it's just minor code cleanup.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 12•5 years ago
|
||
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
|
|
||
Updated•5 years ago
|
|
||
Comment 13•5 years ago
|
||
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
|
||
Comment 14•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/b29078312200
https://hg.mozilla.org/mozilla-central/rev/0c49c78a3216
https://hg.mozilla.org/mozilla-central/rev/8231ec901388
|
|
Reporter | |
Comment 15•5 years ago
|
||
WPT import in https://github.com/web-platform-tests/wpt/pull/17886
|
|
Reporter | |
Comment 16•5 years ago
|
||
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.
|
|
Assignee | |
Comment 17•5 years ago
•
|
||
(In reply to David Baron :dbaron: 🏴 ⌚UTC-7 from comment #16)
although there are a bunch of remaining
buglines
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.
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.)
|
|
Assignee | |
Comment 18•5 years ago
•
|
||
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.)
Description
•