1253683 - APZ Triggers layers when scrolling a non-scrollable subframe

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: bug.gif

STR:
1) Load https://www.topcoder.com/community/data-science/data-science-tutorials/power-up-c-with-the-standard-template-library-part-1/
2) Scroll some of the code samples subframes that have disabled scrollbars.

Comment 1

[Botond Ballo [:botond]]

I can repro this.

What's also interesting is that both a ContainerLayer and a PaintedLayer get created, and after a while the PaintedLayer goes away (I'm guessing due to displayport expiry), but the ContainerLayer sticks around until you next scroll the page, at which point the ContainerLayer goes away.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Reduced test case

Comment 3

[Kartikaya Gupta (email:[e-mail])]

This appears to be happening because those elements are overflow:scroll. That makes the scrollbars visible (even if the element can't scroll), and that makes GetPerceivedScrollingDirections() return both horizontal and vertical directions [1]. This makes the element WantAsyncScroll() and so it gets a displayport and layerized. We should probably stop using GetPerceivedScrollingDirections() and just check if the scroll range is nonzero. We used to do that but it was changed in bug 1126090 [2], incorrectly I think.

[1] http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/layout/generic/nsGfxScrollFrame.cpp#5804
[2] https://hg.mozilla.org/mozilla-central/rev/5921ef66656c

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I have a patch, pretty simple but I don't think we need to uplift this. I would like to write a test as well.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1253683 - Don't layerize scrollframes which are overflow:scroll but not actually scrollable.

Review commit: https://reviewboard.mozilla.org/r/61334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61334/

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1253683 - Add a test.

Review commit: https://reviewboard.mozilla.org/r/61336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61336/

Comment 8

[Kartikaya Gupta (email:[e-mail])]

https://reviewboard.mozilla.org/r/61336/#review58210

::: gfx/layers/apz/test/mochitest/test_bug1253683.html:32
(Diff revision 1)
> +
> +  // Check initial state
> +  is(container.scrollTop, 0, "Initial scrollY should be 0");
> +  ok(!isLayerized('no_layer'), "initially 'no_layer' should not be layerized");
> +
> +  // Scrolling over outer1 should layerize outer1, but not inner1.

Whoops - this comment is copypasta, I'll update it.

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8766469 [details]
Bug 1253683 - Don't layerize scrollframes which are overflow:scroll but not actually scrollable.

https://reviewboard.mozilla.org/r/61334/#review58218

Comment 10

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8766470 [details]
Bug 1253683 - Add a test.

https://reviewboard.mozilla.org/r/61336/#review58220

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Try push (triggered from MozReview) seems green enough, landing.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f3e97c79d1
Don't layerize scrollframes which are overflow:scroll but not actually scrollable. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/a5516d8f4ec8
Add a test. r=tnikkel

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36f3e97c79d1
https://hg.mozilla.org/mozilla-central/rev/a5516d8f4ec8