Closed
Bug 1111753
Opened 9 years ago
Closed 9 years ago
CSS transitions on elements with fixed position children that are larger than their parent don't render their entire area
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: rsingh, Assigned: roc)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(4 files, 3 obsolete files)
1.01 MB,
application/java-archive
|
Details | |
1.20 KB,
text/html
|
Details | |
1.27 KB,
text/html
|
Details | |
5.90 KB,
patch
|
tnikkel
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36 Steps to reproduce: 1. Go to: http://www.berklee.edu in Firefox 34.0.5 2. Watch as the slider transitions, or click one of the slider buttons. Actual results: The transition animation will only render behind the H and P tags, or the calculated area of the parent container. The large fixed position image behind it will only render after the transition has finished. Expected results: The slider transitions should be smooth and beautiful, as it was on previous versions of FF.
Comment 1•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9350909a3401&tochange=24a69de91baa Regressed by Bug 1022612.
Blocks: 1022612
Status: UNCONFIRMED → NEW
status-firefox34:
--- → affected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox37:
--- → affected
status-firefox-esr31:
--- → unaffected
Component: CSS Parsing and Computation → Layout
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
Comment 2•9 years ago
|
||
In local build Last Good: 58d7c85b017e First Bad: 2b0d786121d4 Triggered by 2b0d786121d4 Robert O'Callahan — Bug 1022612. Part 20: Do the business. r=mattwoodrow
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
[Tracking Requested - why for this release]:
Comment 5•9 years ago
|
||
Attachment #8536892 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → roc
Flags: needinfo?(roc)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8539876 -
Flags: review?(tnikkel)
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8539876 [details] [diff] [review] fix Review of attachment 8539876 [details] [diff] [review]: ----------------------------------------------------------------- This is a pretty bad regression :-(.
Attachment #8539876 -
Flags: review?(mats)
Comment 9•9 years ago
|
||
Comment on attachment 8539876 [details] [diff] [review] fix >An out-of-flow frame F may contain placeholder descendents whose out-of-flow >content G is not contained by F's visual overflow area. Thus, restricting the >dirty rect used to build G's display items to F's visual overflow area means >G's display items' mVisibleRect may not contain the true visible area. Wouldn't MarkAbsoluteFrames be called on G's containing block and it would stash the proper dirty rect (because G's containing block's overflow area would contain all of G)? If that's the case then I think the problem is the nsDisplayOpacity item created in BuildDisplayListForStackingContext: it's visible rect will get set to the current dirty rect for F, but it will contain the display items for G as well. If we want to retain this optimization can we use the intersected dirty rect in BuildDisplayListForStackingContext for the BuildDisplayList call and the AutoBuildingDisplayList, but then set the dirty rect to the un-intersected dirty rect after that?
Comment 10•9 years ago
|
||
Don't we have the same problem when using 'filter' ?
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #9) > >An out-of-flow frame F may contain placeholder descendents whose out-of-flow > >content G is not contained by F's visual overflow area. Thus, restricting the > >dirty rect used to build G's display items to F's visual overflow area means > >G's display items' mVisibleRect may not contain the true visible area. > > Wouldn't MarkAbsoluteFrames be called on G's containing block and it would > stash the proper dirty rect (because G's containing block's overflow area > would contain all of G)? Yes. > If that's the case then I think the problem is the nsDisplayOpacity item > created in BuildDisplayListForStackingContext: it's visible rect will get > set to the current dirty rect for F, but it will contain the display items > for G as well. Correct. > If we want to retain this optimization can we use the intersected dirty rect > in BuildDisplayListForStackingContext for the BuildDisplayList call and the > AutoBuildingDisplayList, but then set the dirty rect to the un-intersected > dirty rect after that? I'm not sure how that would work. If the "intersected dirty rect" is set during the BuildDisplayList call from BuildDisplayListForStackingContext, then the visible rects set on display items while building that sublist will be wrong.
Flags: needinfo?(tnikkel)
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #10) > Created attachment 8541596 [details] > filter: opacity() testcase > > Don't we have the same problem when using 'filter' ? I guess so, but this patch would fix that too, right?
Flags: needinfo?(mats)
Comment 14•9 years ago
|
||
No, the two testcases I attached still seem broken with your patch.
Flags: needinfo?(mats)
Comment 15•9 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > > If that's the case then I think the problem is the nsDisplayOpacity item > > created in BuildDisplayListForStackingContext: it's visible rect will get > > set to the current dirty rect for F, but it will contain the display items > > for G as well. > > Correct. Can you make the commit message more clear about this then? > > If we want to retain this optimization can we use the intersected dirty rect > > in BuildDisplayListForStackingContext for the BuildDisplayList call and the > > AutoBuildingDisplayList, but then set the dirty rect to the un-intersected > > dirty rect after that? > > I'm not sure how that would work. If the "intersected dirty rect" is set > during the BuildDisplayList call from BuildDisplayListForStackingContext, > then the visible rects set on display items while building that sublist will > be wrong. Good point.
Flags: needinfo?(tnikkel)
Comment 16•9 years ago
|
||
Comment on attachment 8539876 [details] [diff] [review] fix >@@ -631,17 +631,17 @@ void nsDisplayListBuilder::MarkOutOfFlow >- if (!dirty.IntersectRect(dirty, overflowRect)) >+ if (!dirty.Intersects(overflowRect)) > return; If we return here without setting the out of flow dirty rect property but the frame contains a visible out of flow (for which it is not the containing block) we will use an empty rect in nsFrame::BuildDisplayListForChild for the dirty rect. And so we would use an empty rect as the dirty rect when created (for example) the opacity item in BuildDisplayListForStackingContext and we'd get the same bug?
Updated•9 years ago
|
Flags: needinfo?(roc)
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Timothy Nikkel (:tn) from comment #16) > If we return here without setting the out of flow dirty rect property but > the frame contains a visible out of flow (for which it is not the containing > block) we will use an empty rect in nsFrame::BuildDisplayListForChild for > the dirty rect. > And so we would use an empty rect as the dirty rect when > created (for example) the opacity item in BuildDisplayListForStackingContext > and we'd get the same bug? Hmm, yes, I think that's true...
Flags: needinfo?(roc)
Comment 18•9 years ago
|
||
We're out of time for 35 here and we've already shipped this once in FF34, so this will have to ride to release from 36.
Comment 20•9 years ago
|
||
Comment on attachment 8539876 [details] [diff] [review] fix It seems like an incomplete fix per comment 14.
Attachment #8539876 -
Flags: review?(mats) → review-
Assignee | ||
Comment 21•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9872bbb6b14c
Flags: needinfo?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8536899 -
Attachment mime type: application/x-zip → application/java-archive
Assignee | ||
Comment 22•9 years ago
|
||
Attachment #8539876 -
Attachment is obsolete: true
Attachment #8539876 -
Flags: review?(tnikkel)
Attachment #8545688 -
Flags: review?(tnikkel)
Assignee | ||
Comment 23•9 years ago
|
||
I think the filter issue is separate. In that case, the nsDisplaySVGEffects item has the correct visible rect, but the filter region is wrong, because it uses the visual overflow rect of the filter frame, which doesn't include fixed-pos content. I suspect that is not a regression, but a very old bug --- although it may have gotten a bit worse due to the regression that I'm fixing here. Another problem exists when we have an overflow:hidden element containing an 'opacity' element containing a fixed-pos element. A clip is set on the opacity element which clips it (and its contents!) to the overflow:hidden element. Thinking about how to solve that, I realized that in the analogous situation in the filters case, the rendering isn't well-defined, so I asked for spec feedback here: http://lists.w3.org/Archives/Public/public-fx/2015JanMar/0002.html How that gets resolved will determine how we fix Mats's filter bug. Meanwhile, we'll need to fix opacity, but I'll file another bug for that.
Comment 24•9 years ago
|
||
Comment on attachment 8545688 [details] [diff] [review] 221453.diff Thanks for putting up with my pedantry :)
Attachment #8545688 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Filed bug 1119117 on the second paragraph of comment #23.
Assignee | ||
Comment 26•9 years ago
|
||
So that didn't work, because in some cases (e.g. filters) we need to include the original visible area as well. This seems to pass tests.
Attachment #8545688 -
Attachment is obsolete: true
Attachment #8546386 -
Flags: review?(tnikkel)
Assignee | ||
Comment 27•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d97e41231aa
Updated•9 years ago
|
Attachment #8546386 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f43f1fcfd04
Assignee | ||
Comment 29•9 years ago
|
||
Comment on attachment 8546386 [details] [diff] [review] fix v2 Approval Request Comment [Feature/regressing bug #]: 1022612 [User impact if declined]: Bad regression in rendering certain pages [Describe test coverage new/current, TBPL]: Pretty well covered by reftests [Risks and why]: low risk [String/UUID change made/needed]: none
Attachment #8546386 -
Flags: approval-mozilla-beta?
Attachment #8546386 -
Flags: approval-mozilla-aurora?
Comment 30•9 years ago
|
||
Comment on attachment 8546386 [details] [diff] [review] fix v2 For now, aurora is 36 and 35 is release, so, no need of beta uplift.
Attachment #8546386 -
Flags: approval-mozilla-beta?
Attachment #8546386 -
Flags: approval-mozilla-beta-
Attachment #8546386 -
Flags: approval-mozilla-aurora?
Attachment #8546386 -
Flags: approval-mozilla-aurora+
Comment 31•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1f43f1fcfd04
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Reporter | ||
Comment 33•9 years ago
|
||
Thanks all.
You need to log in
before you can comment on or make changes to this bug.
Description
•