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)

34 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + wontfix
firefox36 + fixed
firefox37 + fixed
firefox-esr31 --- unaffected

People

(Reporter: rsingh, Assigned: roc)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files, 3 obsolete files)

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.
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9350909a3401&tochange=24a69de91baa

Regressed by Bug 1022612.
Blocks: 1022612
Status: UNCONFIRMED → NEW
Component: CSS Parsing and Computation → Layout
Ever confirmed: true
Keywords: regression
OS: Mac OS X → All
In local build
Last Good: 58d7c85b017e
First Bad: 2b0d786121d4
Triggered by
	2b0d786121d4	Robert O'Callahan — Bug 1022612. Part 20: Do the business. r=mattwoodrow
Attached file reduced html zip (obsolete) —
[Tracking Requested - why for this release]:
Attached file reduced html zip
Attachment #8536892 - Attachment is obsolete: true
Regression, tracking.
Roc, can you help here? Thanks
Assignee: nobody → roc
Flags: needinfo?(roc)
Attached patch fix (obsolete) — Splinter Review
Attachment #8539876 - Flags: review?(tnikkel)
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 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?
Don't we have the same problem when using 'filter' ?
(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)
(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)
No, the two testcases I attached still seem broken with your patch.
Flags: needinfo?(mats)
(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 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?
Flags: needinfo?(roc)
(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)
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.
ni for new patch based on comment 17.
Flags: needinfo?(roc)
Comment on attachment 8539876 [details] [diff] [review]
fix

It seems like an incomplete fix per comment 14.
Attachment #8539876 - Flags: review?(mats) → review-
Attachment #8536899 - Attachment mime type: application/x-zip → application/java-archive
Attached patch 221453.diff (obsolete) — Splinter Review
Attachment #8539876 - Attachment is obsolete: true
Attachment #8539876 - Flags: review?(tnikkel)
Attachment #8545688 - Flags: review?(tnikkel)
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 on attachment 8545688 [details] [diff] [review]
221453.diff

Thanks for putting up with my pedantry :)
Attachment #8545688 - Flags: review?(tnikkel) → review+
Attached patch fix v2Splinter Review
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)
Attachment #8546386 - Flags: review?(tnikkel) → review+
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 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+
https://hg.mozilla.org/mozilla-central/rev/1f43f1fcfd04
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Thanks all.
You need to log in before you can comment on or make changes to this bug.