1.00 KB, text/html
1009 bytes, text/html
8.96 KB, patch
|Details | Diff | Splinter Review|
Created attachment 8535760 [details] 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.
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.
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.
Created attachment 8535868 [details] [diff] [review] wip v1 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
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
(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)
(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.)
Created attachment 8548693 [details] [diff] [review] part 1: add new nsChangeHint for 'object-fit' & 'object-position' changes 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())
Created attachment 8548694 [details] [diff] [review] part 2: Refactor nsSubDocumentFrame object-fit/object-position handling logic into a helper-method 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.)
Created attachment 8548698 [details] [diff] [review] part 3: actually handle the new change hint, for nsSubDocumentFrame 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.
Created attachment 8548702 [details] [diff] [review] part 1 v2: add new nsChangeHint for 'object-fit' & 'object-position' changes (Posting updated version of part 1 with a comment tweaked to more closely match the new change hint's name)
Try run with this and bug 1121198: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5fa428b36fd
(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.
[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.
Created attachment 8549092 [details] [diff] [review] part 2 v2: Refactor nsSubDocumentFrame object-fit/object-position handling logic into a helper-method 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.)
Created attachment 8549101 [details] [diff] [review] part 3 v2: actually handle the new change hint, for nsSubDocumentFrame 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.
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...)
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)
(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)
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.
Created attachment 8549362 [details] [diff] [review] alternate patch: just add nsChangeHint_NeedReflow 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)
(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.)
Created attachment 8549365 [details] [diff] [review] alternate patch, v2: just add nsChangeHint_NeedReflow (now with another test) 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.
Created attachment 8549366 [details] [diff] [review] alternate patch, v3: just add nsChangeHint_NeedReflow (now with another test) (sorry, reposting to fix a typo in a comment within the new reftest)
Attachment #8549366 - Flags: review?(roc) → review+
Thanks! Try run before landing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=34125505e01f
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
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.
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
(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.)
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
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
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?
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
tracking-firefox36: --- → ?
tracking-firefox37: --- → +
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?
Attachment #8549366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.)
The next uplift is over a month away?
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.
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+
status-firefox36: affected → fixed
status-firefox37: affected → fixed
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
You need to log in before you can comment on or make changes to this bug.