Closed Bug 1201889 Opened 9 years ago Closed 9 years ago

Not correctly handling position:fixed inside a scrolled ContainerLayer

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox43 --- affected
firefox44 --- fixed

People

(Reporter: mstange, Assigned: botond)

References

Details

Attachments

(3 files)

testcase
567 bytes, text/html
Details
MozReview Request: Bug 1201889 - When adjusting fixed and sticky layers in AsyncCompsitionManager, unapply all async transforms on the path from the fied layer to the layer its fixed with respect to. r=kats
40 bytes, text/x-review-board-request
kats
: review+
Details
MozReview Request: Bug 1201889 - Reftest. r=mstange
40 bytes, text/x-review-board-request
mstange
: review+
Details
Attached file testcase — 
When scrolling in the box with the black border, the blue square jiggles.

(Ignore the fact that sometimes parts of the black border are missing. That's bug 1194851.)

I think some part of https://etherpad.mozilla.org/apz-background-fixed describes what we need to do to fix this. I don't remember specifics, though.
With bug 1166301 landed, I don't see the blue square jiggle any more. Did I misunderstand the STR, or has this been fixed by bug 1166301?
Flags: needinfo?(mstange)
Doh. I was testing with APZ disabled. I can reproduce the problem with bug 1166301 fixed and APZ enabled. Sorry for the noise.
Flags: needinfo?(mstange)
The problem here is what I anticipated (and described an as "edge case") in bug 1187804 comment 7:

(In reply to Botond Ballo [:botond] from comment #7)
> The remaining part is
> handling the case where in between a fixed layer and the ancestor scroll
> frame that it's fixed with respect to, there are additional scroll frames
> with async transforms that also need to be un-applied. This is more of an
> edge case so I'm going to leave fixing it for a follow-up, but for the
> record the general idea of the fix I had in mind is changing the condition
> "is aTransformScrollId equal to the the fixed layer's target scroll id" to
> "is aTransformScrollId on the path from the fixed layer to its target scroll
> id".

Here, the relevant part of the layer structure is:

 R
 |
 C
 |
 F

where R is the layer with the scrollable metrics for the RSF, C is the layer with the scrollable metrics for box with the black border, and F is the fixed layer (the blue box). F is fixed with respect to R, and accordingly we unapply R's async transform to it to keep it fixed. However, in the STR described, C is being async-scrolled, and *its* async transform also needs to be unapplied.
Assignee: nobody → botond
Here's a patch that implements the proposed approach. It turned out to be trickier and less clean than I thought, but it does work and doesn't regress existing async scrolling reftests.

I still need to turn the test case into a new async scrolling reftest.
Bug 1201889 - When adjusting fixed and sticky layers in AsyncCompsitionManager, unapply all async transforms on the path from the fied layer to the layer its fixed with respect to. r=kats
Comment on attachment 8662154 [details]
MozReview Request: Bug 1201889 - When adjusting fixed and sticky layers in AsyncCompsitionManager, unapply all async transforms on the path from the fied layer to the layer its fixed with respect to. r=kats

Bug 1201889 - When adjusting fixed and sticky layers in AsyncCompsitionManager, unapply all async transforms on the path from the fied layer to the layer its fixed with respect to. r=kats
Attachment #8662154 - Flags: review?(bugmail.mozilla)
Bug 1201889 - Reftest. r=mstange
Attachment #8666966 - Flags: review?(mstange)
Kats, if you have any suggestions for how we could handle this more cleanly, please let me know.
Comment on attachment 8666966 [details]
MozReview Request: Bug 1201889 - Reftest. r=mstange

https://reviewboard.mozilla.org/r/20627/#review18497

::: layout/reftests/async-scrolling/position-fixed-in-scroll-container.html:3
(Diff revision 1)
> +<head>

if you insist... :)

(I usually leave <head>/<body> tags out, because they're optional in HTML (the elements get created anyway), but I suppose it's clearer with them included.)
Attachment #8666966 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #9)
> ::: layout/reftests/async-scrolling/position-fixed-in-scroll-container.html:3
> (Diff revision 1)
> > +<head>
> 
> if you insist... :)
> 
> (I usually leave <head>/<body> tags out, because they're optional in HTML
> (the elements get created anyway), but I suppose it's clearer with them
> included.)

They were present in your testcase, which I adapted to produce the reftest files :)

Anyways, I removed them. Try push that includes these patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4203c34c0f82
Comment on attachment 8662154 [details]
MozReview Request: Bug 1201889 - When adjusting fixed and sticky layers in AsyncCompsitionManager, unapply all async transforms on the path from the fied layer to the layer its fixed with respect to. r=kats

https://reviewboard.mozilla.org/r/19527/#review18509

s/fied/fixed/ and s/its/it's/ in the commit message also.

::: gfx/layers/composite/AsyncCompositionManager.cpp:319
(Diff revision 2)
> +      encounteredFixedToLayer = true;

You can get rid of encounteredFixedToLayer entirely, if you just return |encounteredTransformedLayer| inside this if condition, and change the last line in the function to |return false|. I think that would be a little easier to follow because there's fewer logical states and more early-returns.

::: gfx/layers/composite/AsyncCompositionManager.cpp:326
(Diff revision 2)
> +    // srolling). In such a case, stop the walk, as a new layers id could

s/srolling/scrolling/

::: gfx/layers/composite/AsyncCompositionManager.cpp:345
(Diff revision 2)
> -    aLayer->GetFixedPositionScrollContainerId() == aTransformScrollId &&
> +  bool isRootFixed = aLayer->GetIsFixedPosition() &&

Since you're touching this code anyway could you rename this to "isRootOfFixedSubtree"? I always get confused with the variable names in this function.
Attachment #8662154 - Flags: review?(bugmail.mozilla) → review+
Addressed review comments. The Try run also showed that I was failing to guard for a null pointer before calling AsRefLayer(), fixed that too.

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc059714640b
https://hg.mozilla.org/mozilla-central/rev/773c4f8ab8bd
https://hg.mozilla.org/mozilla-central/rev/c50c8ee77ac7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: