Closed Bug 1147673 Opened 10 years ago Closed 9 years ago

nsDisplayWrapList bounds unnecessarily large when it contains a scrollbox with a display port

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

Attachments

(5 files, 4 obsolete files)

MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc
40 bytes, text/x-review-board-request
roc
: review+
tnikkel
: review+
Details
MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond
40 bytes, text/x-review-board-request
botond
: review+
Details
MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats
40 bytes, text/x-review-board-request
botond
: review+
kats
: review+
Details
MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn
40 bytes, text/x-review-board-request
tnikkel
: review+
Details
MozReview Request: Bug 1147673 - Relax the ancestor transform assertion a little. r?kats
58 bytes, text/x-review-board-request
kats
: review+
Details
If you have something like this: <div style="opacity: 0.9"> <div class="width:400px; height: 200px; overflow: auto;" reftest-displayport-x="0" reftest-displayport-y="0" reftest-displayport-w="2000" reftest-displayport-h="200"> <div style="background: red; width: 1000px; height: 100px;"> </div> </div> </div> Then the bounds for the nsDisplayOpacity include the whole 1000px of width of the scrollbox contents, even though those contents are clipped to a width of 400px.
The reftest pull-background-displayport-5.html fails because of this.
Fixing this will fix bug 1122916.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats
Comment on attachment 8669195 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Markus, let me know if this patch resolves your issue. It makes fixed-pos-scrollable1.html pass for me locally (and all the async-scroll reftests, until I run into an assertion in split-opacity-layers-1.html, which looks unrelated (to this patch, possibly related to your patches)).
Attachment #8669195 - Flags: feedback?(mstange)
Comment on attachment 8669195 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Works perfectly, thanks! I'll look into the remaining test failures and assertions.
Attachment #8669195 - Flags: feedback?(mstange) → feedback+
Attachment #8669195 - Flags: feedback+ → review?(bugmail.mozilla)
Comment on attachment 8669195 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats
Comment on attachment 8669195 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats https://reviewboard.mozilla.org/r/21143/#review19127
Attachment #8669195 - Flags: review?(bugmail.mozilla) → review+
Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r?botond
Attachment #8671857 - Flags: review?(botond)
Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats
Attachment #8671858 - Flags: review?(bugmail.mozilla)
Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r?tn
Attachment #8671859 - Flags: review?(tnikkel)
Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Attachment #8671860 - Flags: review?(tnikkel)
Attachment #8671860 - Flags: review?(roc)
Comment on attachment 8671860 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc https://reviewboard.mozilla.org/r/21649/#review19501 ::: layout/base/DisplayListClipState.h:19 (Diff revision 1) > +class ScrollClipStack { Please add a comment explaining what this class is for/what it means. ::: layout/base/DisplayListClipState.h:32 (Diff revision 1) > + { Add a comment explaining what this function is for.
Attachment #8671860 - Flags: review?(roc)
Attachment #8671857 - Attachment description: MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r?botond → MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Attachment #8671857 - Flags: review?(tnikkel)
Attachment #8671857 - Flags: review?(roc)
Attachment #8671857 - Flags: review?(botond)
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Attachment #8671858 - Attachment is obsolete: true
Attachment #8671858 - Flags: review?(bugmail.mozilla)
Attachment #8671859 - Attachment is obsolete: true
Attachment #8671859 - Flags: review?(tnikkel)
Attachment #8671860 - Attachment is obsolete: true
Attachment #8671860 - Flags: review?(tnikkel)
Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r?botond
Attachment #8672635 - Flags: review?(botond)
Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats
Attachment #8672636 - Flags: review?(bugmail.mozilla)
Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r?tn
Attachment #8672637 - Flags: review?(tnikkel)
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc https://reviewboard.mozilla.org/r/21643/#review19571 ::: layout/base/DisplayItemScrollClip.h:32 (Diff revision 4) > + * scroll frame. Excellent! ::: layout/base/DisplayItemScrollClip.h:53 (Diff revision 4) > + * Both aPreferred and aOther are allowed to be null. This isn't quite clear enough. You say it's "analogous" to the intersection operation, but it doesn't actually perform an intersection operation, so you need to say exactly what you mean by "the most restrictive one"? It seems there's an assumption here that one of aPreferred and aOther is contained by the other, so document that and assert it.
Attachment #8671857 - Flags: review?(roc) → review+
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats https://reviewboard.mozilla.org/r/21743/#review19751 Kats already r+'d this patch before he went on PTO. There's no need to review it again.
Attachment #8672636 - Flags: review+
Attachment #8672636 - Flags: review?(bugmail.mozilla)
(In reply to Botond Ballo [:botond] from comment #25) > https://reviewboard.mozilla.org/r/21743/#review19751 > > Kats already r+'d this patch before he went on PTO. (In comment 8.)
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond https://reviewboard.mozilla.org/r/21741/#review19775 This is fine for the case where AlignFixedAndStickyLayers unadjusts the layer's own clip, but what about a case where AlignFixedAndStickyLayers adjusts the clip of a descendant layer? The ApplyAsyncContentTransformToTree call for that descendant layer will have already happened, and potentially intersected its scroll clip with its not-yet-unadjusted clip.
Attachment #8672635 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #27) > This is fine for the case where AlignFixedAndStickyLayers unadjusts the > layer's own clip, but what about a case where AlignFixedAndStickyLayers > adjusts the clip of a descendant layer? The ApplyAsyncContentTransformToTree > call for that descendant layer will have already happened, and potentially > intersected its scroll clip with its not-yet-unadjusted clip. Let's call the container layer L and the descendant D. L has a frame metrics for scroll id SL. D is a fixed layer that has a fixedness annotation with fixed-to-scrollid SL. But D does not have a framemetrics for scroll id SL, because a layer never shares a framemetrics with any of its ancestors. So we won't have applied a scroll clip from an SL framemetrics on D. More generally, I think we can say that a layer never gets a scroll clip from a scroll id for which its ancestor will unadjust, because that would require having a frame metrics of the same scroll id on a layer and its ancestor. So the case you brought up can never occur. Does that sound right?
Flags: needinfo?(botond)
Attachment #8672635 - Flags: review?(botond)
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21741/diff/1-2/
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21743/diff/1-2/
Attachment #8672636 - Flags: review?(bugmail.mozilla)
Comment on attachment 8672637 [details] MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21745/diff/1-2/
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21643/diff/5-6/
Attachment #8672636 - Flags: review?(bugmail.mozilla)
(In reply to Markus Stange [:mstange] from comment #28) > (In reply to Botond Ballo [:botond] from comment #27) > > This is fine for the case where AlignFixedAndStickyLayers unadjusts the > > layer's own clip, but what about a case where AlignFixedAndStickyLayers > > adjusts the clip of a descendant layer? The ApplyAsyncContentTransformToTree > > call for that descendant layer will have already happened, and potentially > > intersected its scroll clip with its not-yet-unadjusted clip. > > Let's call the container layer L and the descendant D. L has a frame metrics > for scroll id SL. D is a fixed layer that has a fixedness annotation with > fixed-to-scrollid SL. But D does not have a framemetrics for scroll id SL, > because a layer never shares a framemetrics with any of its ancestors. So we > won't have applied a scroll clip from an SL framemetrics on D. > More generally, I think we can say that a layer never gets a scroll clip > from a scroll id for which its ancestor will unadjust, because that would > require having a frame metrics of the same scroll id on a layer and its > ancestor. So the case you brought up can never occur. > Does that sound right? I agree that D will never have a scroll clip for SL. But what if D is itself scrollable, and has its own scroll clip as a result?
Flags: needinfo?(botond) → needinfo?(mstange)
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond Dropping review flag until we clear up comment 34. (It's very possible that I'm the one missing something - if so, my apologies.)
Attachment #8672635 - Flags: review?(botond)
Let's say D (the fixed layer) is also scrollable, with scroll id SD. When ApplyAsyncContentTransformToTree is called for D, it will set a clip on D - the scroll clip for SD. At this point it doesn't know anything about any future adjustments for SL, so with its current model of the world, the layer position and its clip, and in particular the offset between the layer and the clip, are correct. Now ApplyAsyncContentTransformToTree is called for L. An async scroll offset is applied to L, which causes D and its clip to move with it on the screen. Then AlignFixedAndStickyLayers comes along and unadjusts both D's position and its clip, and the offset between the clip and the layer stays constant (and we've already determined that this offset was correct). So I think the result will be correct, too. I'll try to answer your original question from a different perspective again, mostly for myself. (In reply to Botond Ballo [:botond] from comment #27) > This is fine for the case where AlignFixedAndStickyLayers unadjusts the > layer's own clip, but what about a case where AlignFixedAndStickyLayers > adjusts the clip of a descendant layer? The ApplyAsyncContentTransformToTree > call for that descendant layer will have already happened, and potentially > intersected its scroll clip with its not-yet-unadjusted clip. Both that scroll clip and the regular layer clip will have been not-yet-unadjusted at that point. Any future unadjusting by the container will be for an ancestor frame metrics, and thus needs to apply to both the regular layer clip and the already-applied scroll clip. Since it needs to happen to both, we can just unadjust the intersection.
Flags: needinfo?(mstange) → needinfo?(botond)
Thanks, Markus, for the explanation. After thinking about this some more, I agree. Your patch fixes a problem that only comes up in the first place in cases where a fixed/sticky layer is its own subtree root.
Flags: needinfo?(botond)
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond https://reviewboard.mozilla.org/r/21741/#review22505
Attachment #8672635 - Flags: review+
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond https://reviewboard.mozilla.org/r/21741/#review22507 Actually, I'm now confused again. In the case where a fixed/sticky layer is its own subtree root, AlignFixedAndStickyLayers doesn't adjust the layer's own clip (because 'adjustClipRect' is conditioned on 'aLayer != aTransformedSubtreeRoot'). Given that, it seems like this patch should be a no-op. If it's not, then I'm still missing something.
Attachment #8672635 - Flags: review+
Flags: needinfo?(mstange)
(In reply to Botond Ballo [:botond] from comment #41) > Actually, I'm now confused again. > > In the case where a fixed/sticky layer is its own subtree root, > AlignFixedAndStickyLayers doesn't adjust the layer's own clip (because > 'adjustClipRect' is conditioned on 'aLayer != aTransformedSubtreeRoot'). > Given that, it seems like this patch should be a no-op. If it's not, then > I'm still missing something. Ok, confusion resolved after talking over IRC! The patch I posted to this bug, which is going to land along with Markus' patches, changes the condition from 'aLayer != aTransformedSubtreeRoot' to 'aTransformAffectsLayerClip'. This can be true even in cases where the fixed/sticky layer is its own subtree root, if the layer has a Layer::mClipRect, or if the layer has multiple FrameMetrics and a metrics below the one being transformed has a FrameMetrics::mClipRect (i.e. if asyncClip.isSome() is true at the point of the AlignFixedAndStickyLayers call). Therefore, the patch is not a no-op, and by your reasoning in comment 38, is correct. Sorry it took so long for me to arrive at this conclusion!
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond https://reviewboard.mozilla.org/r/21741/#review22697
Attachment #8672635 - Flags: review+
(Clearing old needinfo.)
Flags: needinfo?(mstange)
(In reply to Markus Stange [:mstange] from comment #50) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01132d2ec72 This one is green! I'll attach the patches for final review now. They could probably be simplified a little more but I'm afraid to break anything.
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21741/diff/2-3/
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21743/diff/2-3/
Attachment #8672636 - Flags: review?(bugmail.mozilla)
Comment on attachment 8672637 [details] MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21745/diff/2-3/
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21643/diff/6-7/
Attachment #8672636 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21643/diff/7-8/
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21741/diff/3-4/
Attachment #8672636 - Flags: review+ → review?(bugmail.mozilla)
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21743/diff/3-4/
Comment on attachment 8672637 [details] MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21745/diff/3-4/
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21643/diff/8-9/
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Doesn't seem like anything changed from the interdiff.
Attachment #8672636 - Flags: review?(bugmail.mozilla) → review+
Attachment #8669195 - Attachment is obsolete: true
(Inheriting P1 from bug 1122916)
Priority: -- → P1
Attachment #8672637 - Flags: review?(tnikkel) → review+
Comment on attachment 8672637 [details] MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn https://reviewboard.mozilla.org/r/21745/#review24921
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc https://reviewboard.mozilla.org/r/21643/#review24923
Attachment #8671857 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbfa21cf2b52cb5f01360bed12aeab53fd26d76 Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1f2694536c182e853f3acff4675ad5c07d7669 Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/04b102bec36340d84ef40658fae4a0856d762540 Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn https://hg.mozilla.org/integration/mozilla-inbound/rev/68b33692bed3eb35dee9184b7b64daaf93c2ca9a Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18633322&repo=mozilla-inbound
Flags: needinfo?(mstange)
Depends on: 1233206
The mulet reftest failure was because I wasn't resetting the current scroll clip when entering an nsDisplayZoom.
Flags: needinfo?(mstange)
Comment on attachment 8672635 [details] MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21741/diff/4-5/
Attachment #8672635 - Attachment description: MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r?botond → MozReview Request: Bug 1147673 - Unadjust clip before intersecting it with the scroll clip. r=botond
Attachment #8672636 - Flags: review+ → review?(bugmail.mozilla)
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21743/diff/4-5/
Comment on attachment 8672637 [details] MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21745/diff/4-5/
Attachment #8672637 - Attachment description: MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r?tn → MozReview Request: Bug 1147673 - Use ancestor clip for root scrollable framemetrics clips. r=tn
Attachment #8671857 - Attachment description: MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r?tn, r?roc → MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc
Comment on attachment 8671857 [details] MozReview Request: Bug 1147673 - Make display items know about their scroll clips. r=tn, r=roc Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21643/diff/9-10/
Comment on attachment 8672636 [details] MozReview Request: Bug 1147673 - Determine more accurately whether an async transform affects a layer's clip rect. r=kats https://reviewboard.mozilla.org/r/21743/#review25649
Attachment #8672636 - Flags: review?(bugmail.mozilla) → review+
Attachment #8701076 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8701076 [details] MozReview Request: Bug 1147673 - Relax the ancestor transform assertion a little. r?kats https://reviewboard.mozilla.org/r/28885/#review25651
Depends on: 1234800
Blocks: 1145263
Depends on: 1238564
Depends on: 1238730
Depends on: 1242993
Depends on: 1243000
Depends on: 1246469
Depends on: 1246848
Depends on: 1247854
Depends on: 1244873
Depends on: 1250024
Depends on: 1265453
Depends on: 1271714
Depends on: 1273463
No longer depends on: 1273463
Blocks: 1247949
Depends on: 1327329
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: