Closed Bug 1273250 Opened 3 years ago Closed 3 years ago

More proper fix for clipping of fixed background layers in BasicLayerManager

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

Attachments

(4 files)

In bug 1272525, we fixed a bug related to the clipping of fixed background layers in BasicLayerManager.

That fix was more of a workaround; Markus outline a more proper fix, which I plan to implement here.
Comment on attachment 8753053 [details]
MozReview Request: Bug 1273250 - Expand the visible rect of fixed background display items instead of special-casing them in FrameLayerBuilder. r=mstange

https://reviewboard.mozilla.org/r/52964/#review49816

I think this is fine, but Matt needs to approve it, too.
Attachment #8753053 - Flags: review?(mstange) → review+
Well, not just fine, I think it's better.
Attachment #8753052 - Flags: review?(mstange) → review+
Comment on attachment 8753052 [details]
MozReview Request: Bug 1273250 - Factor out a helper function to calculate the viewport rect. r=mstange

https://reviewboard.mozilla.org/r/52962/#review49818
Attachment #8753053 - Flags: review?(matt.woodrow)
Comment on attachment 8753054 [details]
MozReview Request: Bug 1273250 - Set a layer clip rather than a scroll clip for fixed background layers if the clip moves with the layer. r=mstange

https://reviewboard.mozilla.org/r/52966/#review49820

I have one more request, which can be done in a follow-up patch:

I think you should be able to replace "shouldFixToViewport" with "!clipMovesWithLayer" everywhere - I don't think the other two conditions are necessary any more at this point.

Oh, and for consistency, we need to rename mSingleItemFixedToViewport to !mClipMovesWithLayer in some more places.
Attachment #8753054 - Flags: review?(mstange) → review+
Attachment #8753053 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8753053 [details]
MozReview Request: Bug 1273250 - Expand the visible rect of fixed background display items instead of special-casing them in FrameLayerBuilder. r=mstange

https://reviewboard.mozilla.org/r/52964/#review49824
Comment on attachment 8753052 [details]
MozReview Request: Bug 1273250 - Factor out a helper function to calculate the viewport rect. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52962/diff/1-2/
Comment on attachment 8753053 [details]
MozReview Request: Bug 1273250 - Expand the visible rect of fixed background display items instead of special-casing them in FrameLayerBuilder. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52964/diff/1-2/
Comment on attachment 8753054 [details]
MozReview Request: Bug 1273250 - Set a layer clip rather than a scroll clip for fixed background layers if the clip moves with the layer. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52966/diff/1-2/
Comment on attachment 8753115 [details]
MozReview Request: Bug 1273250 - Simplify logic for determining whether a display item should be fixed to thew viewport. r=mstange

https://reviewboard.mozilla.org/r/53014/#review50078

This actually reads very well now, I think!

::: layout/base/FrameLayerBuilder.cpp:3882
(Diff revision 1)
>      }
>  
>      bool clipMovesWithLayer = (animatedGeometryRoot == animatedGeometryRootForClip);
>  
> -    bool shouldFixToViewport = !clipMovesWithLayer &&
> -        !(*animatedGeometryRoot)->GetParent() &&
> +    // For items whose clip scrolls relative to the layer rather than moving
> +    // with t he layer, remove their clip at the display item level because

There's a space inside the word "the".
Attachment #8753115 - Flags: review?(mstange) → review+
Comment on attachment 8753115 [details]
MozReview Request: Bug 1273250 - Simplify logic for determining whether a display item should be fixed to thew viewport. r=mstange

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53014/diff/1-2/
Depends on: 1287075
You need to log in before you can comment on or make changes to this bug.