APZ scrolling broken in the log preview on treeherder, which has a scrollbox inside a position:fixed body element

RESOLVED FIXED in Firefox 47

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: botond)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 wontfix, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Created attachment 8706383 [details]
testcase

STR:
 1. Load the attached testcase.
 2. In the scroll frame at the bottom, scroll down until you're roughly in the middle of the scrollable range (to work around bug 1192910).
 3. Slowly scroll up and down.

Instead of scrolling smoothly, the scrolled layer will only update its position when the display port changes or when something else triggers a main thread paint, e.g. when you hover over a scrollbar. Interestingly, the scrollbar thumbs are still moving the way you'd expect them to.
(Reporter)

Comment 1

2 years ago
This is due to body { position: fixed; }. Botond, do you want to take a look?
Flags: needinfo?(botond)
Summary: APZ scrolling broken in the log preview on treeherder → APZ scrolling broken in the log preview on treeherder, which has a scrollbox inside a position:fixed body element
(Reporter)

Comment 2

2 years ago
Created attachment 8706396 [details]
reduced testcase
(Assignee)

Comment 3

2 years ago
(In reply to Markus Stange [:mstange] from comment #1)
> This is due to body { position: fixed; }. Botond, do you want to take a look?

Sure.
Assignee: nobody → botond
Flags: needinfo?(botond)
(Assignee)

Comment 4

2 years ago
I can't reproduce the problem on Linux desktop using today's nightly build.
(Assignee)

Comment 5

2 years ago
(In reply to Botond Ballo [:botond] from comment #4)
> I can't reproduce the problem on Linux desktop using today's nightly build.

Nor on a Linux laptop using the trackpad.

Markus, assuming you were testing on OS X with a trackpad, is it possible the issue is specific to that input method? Could you test this by attaching a mouse and trying a wheel-scroll?

Alternatively, do you have any local patches applied that might be causing this?
Flags: needinfo?(mstange)
(Assignee)

Comment 6

2 years ago
(In reply to Botond Ballo [:botond] from comment #4)
> I can't reproduce the problem on Linux desktop using today's nightly build.

Actually, that seems to be because we're painting after just about every composite, which I'm guessing is a bug unto its own.

I do see the scroll pause when painting gets backed up and we do a few composites without painting, indicating that there is indeed a problem in AsyncCompositionManager.
Flags: needinfo?(mstange)
(Assignee)

Comment 7

2 years ago
(In reply to Botond Ballo [:botond] from comment #6)
> (In reply to Botond Ballo [:botond] from comment #4)
> > I can't reproduce the problem on Linux desktop using today's nightly build.
> 
> Actually, that seems to be because we're painting after just about every
> composite, which I'm guessing is a bug unto its own.

The optimization put in place in bug 1187432 (to avoid a paint when the displayport didn't move) is firing, but it looks like something else is scheduling a paint every composite.
(Assignee)

Comment 8

2 years ago
(In reply to Botond Ballo [:botond] from comment #7)
> (In reply to Botond Ballo [:botond] from comment #6)
> > (In reply to Botond Ballo [:botond] from comment #4)
> > > I can't reproduce the problem on Linux desktop using today's nightly build.
> > 
> > Actually, that seems to be because we're painting after just about every
> > composite, which I'm guessing is a bug unto its own.
> 
> The optimization put in place in bug 1187432 (to avoid a paint when the
> displayport didn't move) is firing, but it looks like something else is
> scheduling a paint every composite.

Filed bug 1238805 for this; will continue investigation there.
(Assignee)

Comment 9

2 years ago
diagnosis
So on this page, we have:

  - a non-scrollable RSF, which is also position:fixed
  - a scrollable subframe

Neither the scrollframes nor the position:fixed create a ContainerLayer, so we end up with a PaintedLayer which has the following annotations:

  - metrics for the RSF
  - metrics for the subframe
  - a "fixed" annotation that says it's fixed with respect to the RSF

It gets the fixed annotation because it's one of the layers that contains content inside the fixed-position frame.

The problem is, APZ can't distinguish this from a situation where the layer represents content inside the scrollable subframe with that content being marked fixed-position, which produces the same set of annotations. APZ interprets the annotations as indicating this latter scenario, and accordingly untransforms the layer by the async transform of the subframe to keep the content fixed with respect to the RSF.
(Assignee)

Comment 10

2 years ago
Matt Woodrow is working on having fixed-position frames create ContainerLayers in bug 1231538. I think that's going to fix this problem, so I'll wait for that to land.
Depends on: 1231538
(Assignee)

Comment 11

2 years ago
Created attachment 8707683 [details] [diff] [review]
Reftest

Here's the reduced testcase turned into a reftest. However we end up fixing this bug, we should land the reftest.
(Assignee)

Updated

2 years ago
See Also: → bug 1239943
(Assignee)

Comment 12

2 years ago
(In reply to Botond Ballo [:botond] from comment #10)
> Matt Woodrow is working on having fixed-position frames create
> ContainerLayers in bug 1231538. I think that's going to fix this problem, so
> I'll wait for that to land.

Confirmed that the patch in bug 1231538 (more precisely, I pulled from [1]) fixes this bug.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c557f064a4
(Assignee)

Comment 13

2 years ago
Bug 1231538 has now landed. Remaining step is to land the reftest here.
(Assignee)

Comment 14

2 years ago
Created attachment 8713837 [details]
MozReview Request: Bug 1238571 - Reftest. r=mstange

Review commit: https://reviewboard.mozilla.org/r/32833/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32833/
Attachment #8713837 - Flags: review?(mstange)
(Assignee)

Comment 15

2 years ago
Oh, Markus is away until Feb 15. I will ask Matt for review instead.
(Assignee)

Updated

2 years ago
Attachment #8713837 - Flags: review?(mstange) → review?(matt.woodrow)
(Assignee)

Comment 16

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3817fd9116f3
(Reporter)

Comment 17

2 years ago
Comment on attachment 8713837 [details]
MozReview Request: Bug 1238571 - Reftest. r=mstange

https://reviewboard.mozilla.org/r/32833/#review29701

here's a last minute review
Attachment #8713837 - Flags: review+
(Assignee)

Comment 18

2 years ago
Comment on attachment 8713837 [details]
MozReview Request: Bug 1238571 - Reftest. r=mstange

Thanks!
Attachment #8713837 - Flags: review?(matt.woodrow)

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f08d02ec2952

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f08d02ec2952
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
status-firefox46: affected → wontfix
You need to log in before you can comment on or make changes to this bug.