Dynamic changes to "object-fit" on <embed> w/ SVG source file aren't reflected in rendering

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({regression})

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36+ fixed, firefox37+ fixed, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(3 attachments, 9 obsolete attachments)

1.00 KB, text/html
Details
1009 bytes, text/html
Details
8.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
Posted file testcase 1
STR:
 1. Load testcase. (which has an <embed> with a dynamic "object-fit" change, triggered at load time)
 2. (optional) Hover the colorful rect.

EXPECTED RESULTS:
Colorful rect should immediately fill with lime, at load time. No red should be visible.

ACTUAL RESULTS:
Colorful rect doesn't fill with lime until you hover it.


This only happens with a SVG image-file (not with a PNG image-file), and only with <embed> or <object> (not with <img>). This leads me to believe the bug may lie in nsSubDocumentFrame.
Assignee

Updated

5 years ago
Blocks: 1109730
Assignee

Comment 1

5 years ago
Facts:
 (1) When nsStylePosition::CalcDifference() detects a change in "object-fit"/"object-position", it only requests a repaint.
 (2) nsSubDocumentFrame doesn't check object-fit/object-position into consideration when painting -- only when reflowing.

So, the change triggers a repaint, but it isn't actually reflected in the rendering until/unless we get a reflow.
Assignee

Comment 2

5 years ago
So the important stuff in nsSubDocumentFrame::Reflow that needs to be called here is:
> 785     nsRect destRect =
> 786       nsLayoutUtils::ComputeObjectDestRect(nsRect(offset, innerSize),
> 787                                            intrinsSize, intrinsRatio,
> 788                                            StylePosition());
> 789 
> 790     nsViewManager* vm = mInnerView->GetViewManager();
> 791     vm->MoveViewTo(mInnerView, destRect.x, destRect.y);
> 792     vm->ResizeView(mInnerView, nsRect(nsPoint(0, 0), destRect.Size()), true);
> 793   }
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=a6db8f54f5aa#785

One way to achieve this would be:
 - Make CalcDifference report "nsChangeHint_SyncFrameView" for object-position/object-fit changes.
 - Extend nsContainerFrame::SyncFrameViewProperties (which gets triggered by that hint) to have that ^ code quoted above.
Assignee

Comment 3

5 years ago
Posted file testcase 2
Assignee

Comment 4

5 years ago
Posted patch wip v1 (obsolete) — Splinter Review
This implements the suggestion in comment 2, but it's not quite sufficient -- we adjust the position dynamically, but we don't seem to fix the zoom-factor inside the <embed> until after we've had a reflow.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Comment 5

5 years ago
We've got some b2g-only random-oranges that I think are versions of this bug. (Specifically: bug 1104098, bug 1105249, bug 1107869, bug 1109579, bug 1109730, bug 1111132, bug 1116452.) These are all rare random failures in "object-fit-[...]-svg-002[e|o].html" files. (e|o = embed | object).

I just landed a patch that marks these tests as "random-if(b2g)", since they're not adding any new information at this point (and I'm pretty sure they'll be fixed by this bug). I intend to remove the annotations once this bug is fixed.  I marked both the "e" and "o" variants, even when there's only been a bug filed on one of them, because they should both be affected. (but failures are rare enough that we presumably just haven't happened to hit one in the other variant yet)

I'm calling this annotation cset "part 0" of this bug, and it landed here:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a64998089d38
Assignee

Comment 6

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #5)
> These are all rare random failures in
> "object-fit-[...]-svg-002[e|o].html" files. (e|o = embed | object).

(sorry, meant to wildcard out the "002" there; this isn't specific to just a single numbered test-group)

Updated

5 years ago
Blocks: 1120334
Assignee

Comment 8

5 years ago
(BTW, the thing missing in my WIP patch is a call to mFrameLoader->UpdatePositionAndSize(this), for the nsSubDocumentFrame. When we reflow, that gets called in a post-reflow callback, and it appears to be necessary to get the correct positioning/sizing of the internal document.)
Assignee

Comment 9

5 years ago
I think we should fix this by adding a new nsChangeHint, for changes to object-fit & object-position, which triggers any additional behavior that's needed beyond simply repainting.

Generally repainting is sufficient (for e.g. <img> and <video>), since usually we only care about object-fit & object-position at paint time.  But as noted in comment 2, nsSubDocumentFrame::Reflow also has some code that uses this rect and needs to be invoked when it changes.  We could address this by simply triggering a reflow, but we can also be smarter and just directly make the adjustments without requiring a reflow (since no frames have been moved/resized in our outer document).

So, this first patch adds the new nsChangeHint. A later patch here will add the code to actually handle it.  (populating the stub that I'm adding here to RestyleManager::ProcessRestyledFrames())
Attachment #8535868 - Attachment is obsolete: true
Attachment #8548693 - Flags: review?(roc)
Assignee

Comment 10

5 years ago
This intermediate patch creates a helper-method to manage the existing code in nsSubDocumentFrame::Reflow, for dealing with object-fit & object-position. (I'll add a new caller of this helper-method in the next patch.)
Attachment #8548694 - Flags: review?(roc)
Assignee

Comment 11

5 years ago
This last patch makes us actually handle the new change hint, calling a new method on nsSubDocumentFrame which resizes/repositions our view (in the same way that we do during reflow) and invokes mFrameLoader->UpdatePositionAndSize(this) (in the same way that we do at the end of reflow, in nsSubDocumentFrame::ReflowFinished).

This includes a reftest, as well.

One minor concern -- before the existing UpdatePositionAndSize() call (in ReflowFinished()), we create a nsWeakFrame, out of worry that our frame might be destroyed during the UpdatePositionAndSize call.  I initially thought I might need to do the same here (and worry about that possibility). As discussed in bug 1121198, though (which I filed today), I don't think we actually need to worry about this, even at the existing UpdatePositionAndSize() call, and I've posted patches over there to remove the nsWeakFrame from ReflowFinished.
Assignee

Updated

5 years ago
Attachment #8548698 - Flags: review?(roc)
Assignee

Updated

5 years ago
Keywords: leave-open
Assignee

Comment 12

5 years ago
(Posting updated version of part 1 with a comment tweaked to more closely match the new change hint's name)
Attachment #8548693 - Attachment is obsolete: true
Attachment #8548693 - Flags: review?(roc)
Attachment #8548702 - Flags: review?(roc)
Assignee

Updated

5 years ago
Flags: in-testsuite?
Assignee

Comment 14

5 years ago
(In reply to Daniel Holbert [:dholbert] from comment #11)
> One minor concern -- before the existing UpdatePositionAndSize() call (in
> ReflowFinished()), we create a nsWeakFrame, out of worry that our frame
> might be destroyed during the UpdatePositionAndSize call.  I initially
> thought I might need to do the same here

Turns out I probably *do* need to worry about this here, since UpdatePositionAndSize can trigger synchronous reflow (and frame destruction) after all, as discussed in bug 1121198 comment 12.
Comment hidden (obsolete)
Comment hidden (obsolete)
Assignee

Comment 17

5 years ago
[marking speculative comment 15 & 16 as obsolete, since I'm now going a different route]

As suggested by bz in IRC, I'm just going to use a script runner here; that's what we already do for some other nsSubDocumentFrame internal stuff, so it seems appropriate.
Assignee

Comment 18

5 years ago
Posting an updated version of patch 2. (As compared to previous version, I just tweaked some comments and hoisted the mInnerView null-check back out of the helper-function, to make the patch a little more minimal and to allow us to skip the math involved with creating |innerSize| in the null case.)
Attachment #8548694 - Attachment is obsolete: true
Attachment #8548694 - Flags: review?(roc)
Attachment #8549092 - Flags: review?(roc)
Assignee

Comment 19

5 years ago
Here's an updated version of part 3, using a script runner, so that any reflow / frame destruction that we trigger in nsFrameLoader::UpdatePositionAndSize can't cause trouble.
Attachment #8548698 - Attachment is obsolete: true
Attachment #8548698 - Flags: review?(roc)
Attachment #8549101 - Flags: review?(roc)
Why are we adding a new change hint here? Why don't we just reflow? Surely we don't care about the efficiency of object-fit and object-position changes? (And such reflows wouldn't be very expensive, anyway...)
Flags: needinfo?(dholbert)
Assignee

Comment 22

5 years ago
I was thinking about e.g. a transition or an animation on "object-position".  If we just reflow, then we'd need to reflow for each intermediate animation frame, which seems wasteful.  (Though it's likely the reflows would be pretty cheap, right.)

This demo is one example of that:
 https://dev.opera.com/articles/css3-object-fit-object-position/none-transitions.html

Also: I expect object-fit/position to be set much more commonly on <img> than on <embed> (e.g. in that Opera demo), and nsImageFrame never actually needs a reflow -- just a repaint. So if we just used a reflow change-hint, we'd be making that common case slower, unnecessarily. I'd like to avoid that & just do a repaint in that common case. And AIUI, the only way to do that is to use a new changehint, which we can react to (or not) at the point where we know the frame-type. (ProcessRestyledFrames))

I'd definitely be up for interpreting the new changehint as "trigger a reflow" IFF we have a ProcessRestyledFrames, though.  (I wanted to optimize further & just adjust the view / internal-document-size, but perhaps I'm over-optimizing)
Flags: needinfo?(dholbert)
Assignee

Comment 23

5 years ago
(Let me know if you'd like me to update part 3 to just trigger a reflow in the have-a-nsSubDocumentFrame case, or if you can think of another way to do this without requiring that we reflow for "object-position" changes to <img> like in the opera demo, or if you think I just should use a reflow change-hint & be done with it)
Flags: needinfo?(roc)
I don't think that Opera demo is realistic. It wastes blank space for no good reason. A realistic version of that demo would reflow the text to fill up the blank space.

It seems likely to me that the cost of the repaint will dwarf the cost of reflowing a single <img> element. I say let's just use a reflow hint here.
Flags: needinfo?(roc)
Assignee

Comment 25

5 years ago
Fair enough. Here's a patch that just adds nsChangeHint_NeedReflow, and includes the reftest from my former "part 3".

A few notes:
 - We don't need AllReflowHints (or any other reflow-related flags), because we're not changing any sizing or positioning (from the perspective of the host document). We literally just need ::Reflow to be invoked on our nsSubDocument frame, so that it can update the subdocument.
 - We do need to keep nsChangeHint_RepaintFrame, because NeedReflow by itself won't trigger any repaint (IIRC) if the frametree doesn't change (and it won't change, in this case)
Attachment #8548702 - Attachment is obsolete: true
Attachment #8549092 - Attachment is obsolete: true
Attachment #8549101 - Attachment is obsolete: true
Attachment #8548702 - Flags: review?(roc)
Attachment #8549092 - Flags: review?(roc)
Attachment #8549101 - Flags: review?(roc)
Attachment #8549362 - Flags: review?(roc)
Assignee

Comment 26

5 years ago
(my current reftest tweaks "object-fit" -- I should include a testcase that tweaks "object-position", to make sure we handle that correctly as well. I'll post an updated patch with that second testcase included.)
Assignee

Comment 27

5 years ago
OK, this version includes an "object-position" test as well.

I've verified locally that both reftests fail before the fix & pass after the fix.
Attachment #8549362 - Attachment is obsolete: true
Attachment #8549362 - Flags: review?(roc)
Attachment #8549365 - Flags: review?(roc)
Assignee

Comment 28

5 years ago
(sorry, reposting to fix a typo in a comment within the new reftest)
Attachment #8549365 - Attachment is obsolete: true
Attachment #8549365 - Flags: review?(roc)
Attachment #8549366 - Flags: review?(roc)
Assignee

Comment 30

5 years ago
Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d1864ee9f2b2
Flags: in-testsuite? → in-testsuite+
Hi Daniel, sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5493793&repo=mozilla-inbound
Flags: needinfo?(dholbert)
Assignee

Comment 32

5 years ago
Interesting -- the failure there just shows a single column of reddish-green pixels at the right edge of the <object>.  We must have a rounding error on that platform or something.

I expect I can work around that by making the reference case use the same replaced elements as the testcase (<img>, <object>, etc) instead of just lime-backgrounded divs.
Flags: needinfo?(dholbert)
Assignee

Comment 33

5 years ago
This failure (fuzziness) seems to be specific to the "Android 4.0 API10+" platform. Here's the mozilla-inbound cycle where this landed, showing the failures as R5 oranges:
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d1864ee9f2b2
Assignee

Comment 34

5 years ago
(Oh, looks like it affects "Android 2.3 API9 opt", too -- I initially missed that due to the different R# numbering there. This runs as part of R9 on that platform.)
Assignee

Comment 35

5 years ago
Try run with the reference cases tweaked to use the same replaced elements as the testcases, per comment 32: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b450a40b7bac
Assignee

Comment 36

5 years ago
Try run looks good on Android & the other completed platforms (most of them). Re-pushed with that reference case tweak described above:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b21f4d438c
Assignee

Comment 37

5 years ago
Comment on attachment 8549366 [details] [diff] [review]
alternate patch, v3: just add nsChangeHint_NeedReflow (now with another test)

Requesting Aurora approval.

Approval Request Comment
[Feature/regressing bug #]: bug 624647 (object-fit / object-position support)

[User impact if declined]: Broken rendering on pages that use these CSS properties on <embed> / <object> elements, with SVG content.

[Describe test coverage new/current, TBPL]: Good. We've got lots of tests for object-fit & object-position rendering, for static content, and this patch includes tests for testing dynamic changes.

[Risks and why]: Low risk. Very targeted change, to make us now require a reflow when these properties change. (as we do for many other properties)

[String/UUID change made/needed]: None.
Attachment #8549366 - Flags: approval-mozilla-aurora?
Assignee

Comment 38

5 years ago
I also landed a patch to revert the annotations from comment 5 here, since those random-orange tests should be passing reliably now that this bug is fixed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/96b9d4781508
https://hg.mozilla.org/mozilla-central/rev/b6b21f4d438c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8549366 [details] [diff] [review]
alternate patch, v3: just add nsChangeHint_NeedReflow (now with another test)

Aurora+

Bug 624647 landed in 36. This change is relatively small and we are near the beginning of the Beta cycle. What do you think about uplifting this fix to Beta?
Flags: needinfo?(dholbert)
Attachment #8549366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee

Comment 42

5 years ago
(To be clear, my aurora approval was for 37, for the aurora-that's-about-to-become-beta.)

This is a small/safe enough change that I would be comfortable with it landing during Beta (though I suspect it will land on 37 *just before* it becomes beta).

I'd be a somewhat hesitant landing this fix for 36, the beta-that's-about-to-become-release, though it's honestly small/safe enough that it'd probably be fine there too.  But I'm not going to lobby for that at this point (or for preffing off bug 624647 in Firefox 36 as a result of this bug), because I think this bug is unlikely to get hit frequently in the real world during the 6 weeks where Firefox 36 is our release version.  As I understand it, the primary web-developer use-cases for this feature ('object-fit'/'object-position') are with <img> and <video> -- and this bug is only a problem with <embed>/<object>, and it only happens with dynamic changes to these properties, *and* it only happens when the underlying image-data is SVG.  (And even then, the problem is just a rendering glitch -- nothing security-sensitive or anything.) (And SVG has its own long-supported ways of doing this sort of thing -- using viewBox & preserveAspectRatio -- so anyone who might hit this would likely already be using those instead of object-fit & object-position.)
Flags: needinfo?(dholbert)
The next uplift is over a month away?
Assignee

Comment 44

5 years ago
Oops, sorry, I was mistakenly thinking that merge day was this coming week. Looks like it was actually this past week. :)

Given that -- yes, I think this is safe enough to be uplifted to Beta, paricularly given that we're early in the Beta cycle.  I'll go ahead & request that.
Assignee

Comment 45

5 years ago
Comment on attachment 8549366 [details] [diff] [review]
alternate patch, v3: just add nsChangeHint_NeedReflow (now with another test)

Approval Request Comment
[see comment 37 for approval-request questions/answers]
Attachment #8549366 - Flags: approval-mozilla-beta?
Attachment #8549366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.