Closed Bug 1253683 Opened 5 years ago Closed 5 years ago

APZ Triggers layers when scrolling a non-scrollable subframe

Categories

(Core :: Panning and Zooming, defect)

48 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed

People

(Reporter: BenWa, Assigned: kats)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

Attached image 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.
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.
Blocks: apz-desktop
Keywords: perf
Whiteboard: [gfx-noted]
Version: unspecified → 48 Branch
Attachment #8766430 - Attachment mime type: text/plain → text/html
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
Blocks: 1126090
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.
Assignee: nobody → bugmail.mozilla
Attached patch Patch (obsolete) — Splinter Review
Attachment #8766439 - Attachment is obsolete: true
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 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
Attachment #8766469 - Flags: review?(tnikkel) → review+
Try push (triggered from MozReview) seems green enough, landing.
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/36f3e97c79d1
https://hg.mozilla.org/mozilla-central/rev/a5516d8f4ec8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.