1246290 - Add a way to disable APZ on a per-page (or similar) basis

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

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

We should add a mechanism that allows us to disable APZ on specific content. Ideally we would just disable APZ and fall back to sync scrolling on that content without affecting other things, but if that's not feasible we should just add a thing to disable it at the widget level that we can control on a per-page basis.

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I would be interested in this for DevTools UI, at least until we're able to resolve outstanding issues with CodeMirror (or, at least ones that are triggered by APZ of course).

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=078dba89bc44

Comment 3

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

I added another patch on top of what I have in the try push above; the extra patch adds a chrome-only property on HTML elements that allows chrome code to disable APZ scrolling on specific scrollframes (such as the devtools codemirror scrollframe). The patches are at https://github.com/staktrace/gecko-dev/commits/apz-per-page while I mull them over and see if anything else is needed.

Comment 4

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

Attachment: MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

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

Comment 5

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

Attachment: MozReview Request: Bug 1246290 - Add a bit to FrameMetrics to indicate if APZ-scrolling should be disabled on that APZC. r=botond

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

Comment 6

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

Attachment: MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

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

Comment 7

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

Attachment: MozReview Request: Bug 1246290 - Add a pref to allow disabling APZ on documents which have scroll-linked effects. r=botond

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

Comment 8

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

Attachment: MozReview Request: Bug 1246290 - Add a reftest for the apz.disable_for_scroll_linked_effects pref. r=botond

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

Comment 9

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

Attachment: MozReview Request: Bug 1246290 - Add a disableapz HTML attribute that allows disabling APZ on specific scrollframes in chrome documents. r?botond,ehsan

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

Comment 10

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

Part 6 will need ehsan (or other DOM peer)'s review as well, I'll flag him once he's back from PTO.

Comment 11

[Markus Stange [:mstange]]

If we only ever want to turn off async scrolling for content under our own control (like the devtools), we could instead change that content to preventDefault all wheel events and have it do its own scrolling based on the wheel events' deltas.

Comment 12

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

That's a good point. I don't really like the idea of turning off APZ partially with these patches because it's pretty risky and won't have good testing coverage. It's meant to be a "backup plan" in case we find a lot of people complaining about pages with scroll-linked effects, but if it's a backup plan it better be working reliably, and that's going to be hard to guarantee. If there's a lot of complaints I'd rather disable APZ entirely because then we fall back to a code path we know will work well.

:jryans, do you mind trying what Markus suggested in comment 11 and seeing if it works well for the DevTools UI case?

Comment 13

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

Comment on attachment 8721017 [details]
MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

Clearing review for now, until we hear back from jryans.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: prevent-wheel

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> That's a good point. I don't really like the idea of turning off APZ
> partially with these patches because it's pretty risky and won't have good
> testing coverage. It's meant to be a "backup plan" in case we find a lot of
> people complaining about pages with scroll-linked effects, but if it's a
> backup plan it better be working reliably, and that's going to be hard to
> guarantee. If there's a lot of complaints I'd rather disable APZ entirely
> because then we fall back to a code path we know will work well.
> 
> :jryans, do you mind trying what Markus suggested in comment 11 and seeing
> if it works well for the DevTools UI case?

Hmm.  Well, I am pretty confused.  Is there a good way to test "this element is currently using APZ"?

I first tried what you suggested (I think!), which you can see in the attached patch.  I am listening wheel events from CodeMirror and calling preventDefault().  I am getting the wheel events, but I did notice a change in behavior.

Then I tried disabling APZ altogether with layers.async-pan-zoom.enabled = false, but that seemed to have no effect on CodeMirror either.

The main visual glitch I am seeing (apparently no matter whether APZ is on or off, somehow) is that the line numbers slides around horizontally.  I had thought this behavior only started when APZ was first enabled, but I may have remembered wrong.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: codemirror-scrolling.gif

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:kats, am I totally misunderstanding things here?  Check out the recording above.

Comment 17

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> I first tried what you suggested (I think!), which you can see in the
> attached patch.  I am listening wheel events from CodeMirror and calling
> preventDefault().  I am getting the wheel events, but I did notice a change
> in behavior.

So when I apply your patch, I'm unable to scroll in the devtools debugger at all, which is what I would expect.

> Then I tried disabling APZ altogether with layers.async-pan-zoom.enabled =
> false, but that seemed to have no effect on CodeMirror either.

It should - did you restart the browser?

> The main visual glitch I am seeing (apparently no matter whether APZ is on
> or off, somehow) is that the line numbers slides around horizontally.  I had
> thought this behavior only started when APZ was first enabled, but I may
> have remembered wrong.

Yeah this glitch should only happen with APZ enabled.

I noticed also in the screenrecording that you seem to be using DevEdition (it has the dark theme) - was that the same build on which you applied the patch, or was that recording just for illustrative purposes? (I'm just grasping at straws here, since it what you did seems to be the correct thing, and it worked as expected for me).

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: disable-apz-for-codemirror

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> > I first tried what you suggested (I think!), which you can see in the
> > attached patch.  I am listening wheel events from CodeMirror and calling
> > preventDefault().  I am getting the wheel events, but I did notice a change
> > in behavior.
> 
> So when I apply your patch, I'm unable to scroll in the devtools debugger at
> all, which is what I would expect.

So...  I rebuilt again with the patch.  Now I see what you're seeing: scrolling is completely disabled in the debugger.  Unsure what was going on before, but at least it matches now.

I have extended the patch to call `scrollBy` to do the scrolling, and this does appear to disable APZ glitches for this element which is great!  So, it seems like this should work for DevTools cases, thanks!

I am still a bit confused about general APZ settings though, see below...

> > Then I tried disabling APZ altogether with layers.async-pan-zoom.enabled =
> > false, but that seemed to have no effect on CodeMirror either.
> 
> It should - did you restart the browser?

Yes, I did restart the browser after changing the pref value.  (For testing just the APZ pref, I am on a local build, but without the CodeMirror specific preventDefault patch, since I would expect a global behavior change.)  about:support says "Asynchronous Pan/Zoom none".  This part is still true after rebuilding, setting layers.async-pan-zoom.enabled = false still has no effect on CodeMirror.  Is it because it's a chrome document?  Is chrome always APZ on despite the pref?

I also tried a fresh profile without my usual development junk, and only set layers.async-pan-zoom.enabled = false, restarted, and confirmed about:support said APZ: none.  CodeMirror continues to glitch as in the recording.

> > The main visual glitch I am seeing (apparently no matter whether APZ is on
> > or off, somehow) is that the line numbers slides around horizontally.  I had
> > thought this behavior only started when APZ was first enabled, but I may
> > have remembered wrong.
> 
> Yeah this glitch should only happen with APZ enabled.
> 
> I noticed also in the screenrecording that you seem to be using DevEdition
> (it has the dark theme) - was that the same build on which you applied the
> patch, or was that recording just for illustrative purposes? (I'm just
> grasping at straws here, since it what you did seems to be the correct
> thing, and it worked as expected for me).

Yes, that's the build with the patch applied.  My development profile has the DevTools theme enabled, that's all.  It's a local build from fx-team plus the patch.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I have filed bug 1250398 to use this approach to disable APZ for DevTools usages of CodeMirror.

Comment 20

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

Hm, interesting. I can reproduce this behaviour and I'm not entirely sure why it's happening. It might be a result of bug 1209970. I've filed bug 1250550 for this issue, since it seems to be an independent regression.

Comment 21

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

Since the devtools scenario is taken care of we don't have any immediate need for these patches any more. I'll leave this bug open for now in case somebody decides that the scroll-linked-effect problem is too severe in content and we should still disable APZ on those pages, but we can close this once APZ is shipped.

Comment 22

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

WONTFIX for now, but we can reopen it and dust off the patches if needed.

Comment 23

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

During the triage meeting today (in particular while discussing bug 1238503 and bug 1238504) I mentioned this bug, and Brad suggested that we should keep it open and have product make the call as to whether or not we should use this mechanism to avoid the slew of apz-evangelism bugs we have.

Comment 24

[Mike Taylor [:miketaylr]]

So the idea here is when scroll-linked effects are detected, we don't APZ?

Would we still show the error in the console?

Comment 25

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

(In reply to Mike Taylor [:miketaylr] from comment #24)
> So the idea here is when scroll-linked effects are detected, we don't APZ?

Yes.

> Would we still show the error in the console?

Yes.

The caveat is that there are some pages which we can't properly detect are doing scroll-linked effects. Bugs that are marked apz-evangelism but are not dependent on this one are in that category, for example.

Comment 26

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

Comment on attachment 8721017 [details]
MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35541/diff/1-2/

Comment 27

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

Comment on attachment 8721018 [details]
MozReview Request: Bug 1246290 - Add a bit to FrameMetrics to indicate if APZ-scrolling should be disabled on that APZC. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35543/diff/1-2/

Comment 28

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

Comment on attachment 8721019 [details]
MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35545/diff/1-2/

Comment 29

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

Comment on attachment 8721020 [details]
MozReview Request: Bug 1246290 - Add a pref to allow disabling APZ on documents which have scroll-linked effects. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35547/diff/1-2/

Comment 30

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

Comment on attachment 8721021 [details]
MozReview Request: Bug 1246290 - Add a reftest for the apz.disable_for_scroll_linked_effects pref. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35549/diff/1-2/

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8721017 [details]
MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

https://reviewboard.mozilla.org/r/35541/#review38635

Thanks, this is definitely nicer!

Comment 32

[Botond Ballo [:botond]]

So, in the approach being taken here, we're not async-transforming scroll frames which are marked as "force disable APZ", but we're still layerizing them. Do we get any benefit from that? If not, can we avoid layerizing them, perhaps by reusing the scroll info layer mechanism?

Comment 33

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

Hm, good point (about layerization). We don't get any benefit from if scrolling (i.e. the setting of a displayport) is the only reason it was layerized. Maybe I can short-circuit the displayport setting somehow to prevent layerization. On the other hand, I also want to keep the two codepaths in sync as much as possible, since it's unlikely that this pref will get exercised much initially, and I don't want flipping it to cause things to behave very differently.

Comment 34

[Markus Stange [:mstange]]

Huh? We had layerization of scroll frames before APZ, and it was beneficial...

Comment 35

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

That's true. (Beneficial to avoid unnecessary repainting of things that didn't move). Given that, and what I said in comment 33 I'll leave the patches as-is for now. We can decide if there's still a compelling reason to avoid layerization and if so, which cases to avoid it in.

Comment 36

[Botond Ballo [:botond]]

So I've been thinking about this a lot more, and talked about it with Markus a bit.

There are a few different choices to make here:

  - Whether we layerize the scroll frame. As Markus pointed out, this was done
    pre-APZ as well, to reduce invalidation. We probably want to keep doing this.
    
    The current patch does this, which is good. Reusing the scroll info layer
    mechanism wouldn't have done this.

  - Whether we paint a displayport (with margins larger than zero) for the scroll
    frame. We probably don't want to do this, because it shifts work from being
    evenly distributed across paints, to being stockpiled in a few individual
    paints, leading to jank.

    The patch currently suppresses the displayport, so that's good.

  - Whether we want an APZC created for the scroll frame (and events to flow
    through that APZC). Both the current patch, and using scroll info layers,
    have this effect. It's also the more future-proof approach because eventually
    we'd like to remove the main-thread code paths for processing events that
    are handled by APZ.

    We've had this kind of setup (having APZCs but not async-transforming content) 
    before, when inactive scroll frames got scroll info layers, so it's not
    entirely new, though it probably hasn't been tested much recently. I'm down
    to give it a try. We may want to be cautious with how far we uplift it.

So, overall, I think the direction of the patch is a good one.

I have one remaining significant concern: 

GetCurrentAsyncTransform() and similar are called not just by AsyncCompositionManager to determine how to transform the layers visually, but also by the APZ hit testing and event processing code. Is having those processes (APZ hit testing and event processing) *not* reflect the async transforms, what we want? This would mean that processing of a later event may not "see" scrolling caused by processing of an earlier event, which is a different behaviour than either pure APZ scrolling (where the events and their effects are sequenced in APZ) or pure main thread scrolling (where the events and their effects are sequenced on the main thread).

Comment 37

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

(In reply to Botond Ballo [:botond] from comment #36)
> GetCurrentAsyncTransform() and similar are called not just by
> AsyncCompositionManager to determine how to transform the layers visually,
> but also by the APZ hit testing and event processing code. Is having those
> processes (APZ hit testing and event processing) *not* reflect the async
> transforms, what we want? This would mean that processing of a later event
> may not "see" scrolling caused by processing of an earlier event, which is a
> different behaviour than either pure APZ scrolling (where the events and
> their effects are sequenced in APZ) or pure main thread scrolling (where the
> events and their effects are sequenced on the main thread).

I remember thinking this through when I wrote the patch and convinced myself that they had the correct behaviour. But thinking it through again, I think I was wrong. Some braindump:

When doing the untransform on input events, we take the input event, then apply GetScreenToApzcTransform followed by GetApzcToGeckoTransform. In the first transformation (GetScreenToApzcTransform), we don't use the async transform of the target APZC anyway, so that doesn't change. The only thing we use from target APZC is the ancestorUntransform which is unaffected by these patches.

In the second transformation (GetApzcToGeckoTransform) we unapply GetCurrentAsyncTransformWithOverscroll(), and then apply GetTransformToLastDispatchedPaint(). The first basically unapplies the async transform that APZC has and puts the event back into the "mLastContentPaintMetrics" space, and then the second takes it from the "mLastContentPaintMetrics" space to the "mExpectedGeckoMetrics" space.

Let's say the user starts with a page at y=0, does a wheel-scroll by 10px followed immediately by a click at y=10 on the screen. With pure main thread scrolling, the scroll would occur before the click is processed, and the click would hit the page at y=20. Likewise with pure APZ, we would visually transform the page from y=0 to y=10, and the user's click would land at y=20. That process is a little more complex though, here's what the various FrameMetrics would look like after the wheel scroll:

mLastContentPaintMetrics: y=0
mFrameMetrics: y=10
mExpectedGeckoMetrics: y=10 (because we update this on a repaint request)

and this is what the click would go through:
initially: y=10
After GetScreenToApzcTransform: y=10
After unapplying GetCurrentAsyncTransformWithOverscroll: y=20
After applying GetTransformToLastDispatchedPaint: y=10
After applying scroll position on main thread: y=20

With the APZ force-disable code, the various FrameMetrics would look the same:

mLastContentPaintMetrics: y=0
mFrameMetrics: y=10
mExpectedGeckoMetrics: y=10

and the click would get untransformed like so:
initially: y=10
After GetScreenToApzcTransform: y=10
After unapplying GetCurrentAsyncTransformWithOverscroll, which is empty: y=10
After applying GetTransformToLastDispatchedPaint: y=0
After applying scroll position on main thread: y=10

So there is a difference here, and the behaviour is wrong. I think if we make GetTransformToLastDispatchedPaint also return an empty transform it might fix the problem. However I'll try to write some gtests to exercise this code and verify.

Comment 38

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

So I wrote a gtest and verified that I need to have GetTransformToLastDispatchedPaint also return an empty transform if the flag is set. Updated patches coming.

Comment 39

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

Attachment: MozReview Request: Bug 1246290 - Add a simple gtest to exercise the force-disabled-APZ codepaths. r?botond

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

Comment 40

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

Comment on attachment 8721017 [details]
MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35541/diff/2-3/

Comment 41

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

Comment on attachment 8721018 [details]
MozReview Request: Bug 1246290 - Add a bit to FrameMetrics to indicate if APZ-scrolling should be disabled on that APZC. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35543/diff/2-3/

Comment 42

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

Comment on attachment 8721019 [details]
MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35545/diff/2-3/

Comment 43

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

Comment on attachment 8721020 [details]
MozReview Request: Bug 1246290 - Add a pref to allow disabling APZ on documents which have scroll-linked effects. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35547/diff/2-3/

Comment 44

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

Comment on attachment 8721021 [details]
MozReview Request: Bug 1246290 - Add a reftest for the apz.disable_for_scroll_linked_effects pref. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35549/diff/2-3/

Comment 45

[Botond Ballo [:botond]]

Comment on attachment 8721018 [details]
MozReview Request: Bug 1246290 - Add a bit to FrameMetrics to indicate if APZ-scrolling should be disabled on that APZC. r=botond

https://reviewboard.mozilla.org/r/35543/#review39955

Pretty sure this patch isn't going to change.

::: gfx/layers/FrameMetrics.h:787
(Diff revision 2)
>  
>    // Whether or not this frame has a "scroll info layer" to capture events.
>    bool mIsScrollInfoLayer:1;
>  
> +  // Whether or not the compositor should actually do APZ-scrolling on this
> +  // scrollframe or not.

One too many "or not"s.

Comment 46

[Botond Ballo [:botond]]

Comment on attachment 8721019 [details]
MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

https://reviewboard.mozilla.org/r/35545/#review39957

This patch will need to change to address the issue we discussed on IRC:

20:38	botond	kats: consider a scenario where you have a root scroll frame R and a subframe S
20:38	botond	kats: suppose the mouse is over R, just below the area of S, and suppose you send two mouse wheel events
20:39	botond	kats: the first scrolls R, bringing S under the mouse. the second scrolls S, because that's what's now under the mouse
20:39	botond	kats: (i'm ignoring wheel transactions here. substitute touch pan gestures for wheel events if that bothers you)
20:39	kats	i'm not sure touch can be substituted for wheel here, but never mind - continue
20:40	botond	kats: if APZ hit testing is ignoring async transforms, it can yield R as a result for the second event
20:42	kats	hm yeah that's true
20:42	kats	let me think about that
20:43	botond	kats: the approach i had in mind, is to have _only_ AsyncCompositionManager ignore the async transform, not callers internal to APZ (and correspondingly, GetTransformToLastDispatchedPaint, which only has APZ-internal callers, would be left alone)
20:44	kats	botond: that's a good idea. it would be more in line with what happens for scrollinfo layers
20:44	botond	kats: exactly
20:45	kats	botond: ok, i'll give it some more thought and update the patches once i convince myself there's no issues

Comment 47

[Botond Ballo [:botond]]

Comment on attachment 8721020 [details]
MozReview Request: Bug 1246290 - Add a pref to allow disabling APZ on documents which have scroll-linked effects. r=botond

https://reviewboard.mozilla.org/r/35547/#review39959

::: gfx/layers/apz/src/AsyncPanZoomController.cpp:158
(Diff revision 3)
>   * a state where we can't keep up with scrolling. The danger zone prefs specify
>   * how wide this margin is; in the above example a y-axis danger zone of 10
>   * pixels would make us drop to low-res at y=490...990.\n
>   * This value is in layer pixels.
>   *
> + * \li\b apz.disable_for_sle_pages

I mildly prefer a longer name, like "disable_for_scroll_linked_effects", than an opaque acronym like "sle".

::: gfx/thebes/gfxPrefs.h:151
(Diff revision 3)
>    DECL_GFX_PREF(Live, "apz.axis_lock.lock_angle",              APZAxisLockAngle, float, float(M_PI / 6.0) /* 30 degrees */);
>    DECL_GFX_PREF(Live, "apz.axis_lock.mode",                    APZAxisLockMode, int32_t, 0);
>    DECL_GFX_PREF(Live, "apz.content_response_timeout",          APZContentResponseTimeout, int32_t, 300);
>    DECL_GFX_PREF(Live, "apz.danger_zone_x",                     APZDangerZoneX, int32_t, 50);
>    DECL_GFX_PREF(Live, "apz.danger_zone_y",                     APZDangerZoneY, int32_t, 100);
> +  DECL_GFX_PREF(Live, "apz.disable_for_sle_pages",             APZDisableForSLEPages, bool, false);

That goes for this function name too.

::: layout/base/nsLayoutUtils.cpp:1062
(Diff revision 3)
>  
>    return result;
>  }
>  
>  static bool
> +DisableApzForElement(nsIContent* aContent)

ShouldDisableApzForElement(), as per the convention you established :)

::: layout/base/nsLayoutUtils.cpp:1066
(Diff revision 3)
>  static bool
> +DisableApzForElement(nsIContent* aContent)
> +{
> +  if (gfxPrefs::APZDisableForSLEPages() && aContent) {
> +    nsIDocument* doc = aContent->GetComposedDoc();
> +    return (doc && doc->HasScrollLinkedEffect());

It doesn't look like there's a mechanism for a document to have its mHasScrollLinkedEffect flag cleared. Should we try to add one, so the document isn't stuck with main-thread scrolling for its lifetime?

Comment 48

[Botond Ballo [:botond]]

Comment on attachment 8721021 [details]
MozReview Request: Bug 1246290 - Add a reftest for the apz.disable_for_scroll_linked_effects pref. r=botond

https://reviewboard.mozilla.org/r/35549/#review39961

::: layout/reftests/async-scrolling/disable-apz-for-sle-pages.html:3
(Diff revision 3)
> +<!DOCTYPE html>
> +<html class="reftest-wait" reftest-async-scroll
> +      reftest-async-scroll-y="200">

Do we not need the reftest-displayport-\* attributes for the reftest-async-scroll-\* to have a chance of taking effect to begin with? That's my impression based on a quick look at reftest-content.js, but I haven't tested to confirm.

::: layout/reftests/async-scrolling/disable-apz-for-sle-pages.html:21
(Diff revision 3)
> +        document.getElementById('fake-fixed').style.top = window.scrollY + "px";
> +    }, false);
> +
> +    addEventListener('load', function() {
> +        window.scrollTo(0, 100);
> +        setTimeout(done, 0);

Does this give the scroll handler a chance to run?

Comment 49

[Botond Ballo [:botond]]

Comment on attachment 8736420 [details]
MozReview Request: Bug 1246290 - Add a simple gtest to exercise the force-disabled-APZ codepaths. r?botond

https://reviewboard.mozilla.org/r/43277/#review39963

Dropping review for now as the "untransforming events" part might change.

Comment 50

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

(In reply to Botond Ballo [:botond] from comment #45)
> One too many "or not"s.

Fixed.

(In reply to Botond Ballo [:botond] from comment #46)
> This patch will need to change to address the issue we discussed on IRC:

I updated it so that those APZC functions take a parameter. The parameter indicates whether or not the function should respect the disable flag, and I updated all the call sites to pass the appropriate flag. The stuff in AsyncCompositionManager and most things in ContainerLayerComposite got the RESPECT_FORCE_DISABLE while the rest got the regular behaviour. I considered having a default value for this parameter but I thought it would be better to make it explicit in case we add new call sites.

(In reply to Botond Ballo [:botond] from comment #47)
> > + * \li\b apz.disable_for_sle_pages
> 
> I mildly prefer a longer name, like "disable_for_scroll_linked_effects",
> than an opaque acronym like "sle".

Sounds good, updated.

> ::: gfx/thebes/gfxPrefs.h:151
> > +  DECL_GFX_PREF(Live, "apz.disable_for_sle_pages",             APZDisableForSLEPages, bool, false);
> 
> That goes for this function name too.

Done

> ::: layout/base/nsLayoutUtils.cpp:1062
> > +DisableApzForElement(nsIContent* aContent)
> 
> ShouldDisableApzForElement(), as per the convention you established :)

Done :)

> ::: layout/base/nsLayoutUtils.cpp:1066
> It doesn't look like there's a mechanism for a document to have its
> mHasScrollLinkedEffect flag cleared. Should we try to add one, so the
> document isn't stuck with main-thread scrolling for its lifetime?

I think it would be pretty hard to add one, since a scroll listener might apply a scroll-linked effect (i.e. adjust a CSS property) based on some other condition. Reliably turning it off without having to turn it back on would be hard. We could have it turn off and then turn back on later but it seems like more effort than it's worth. I'd rather make it more obvious to web developers that scroll-linked effects in JS are going to result in janky scrolling.

(In reply to Botond Ballo [:botond] from comment #48)
> Do we not need the reftest-displayport-\* attributes for the
> reftest-async-scroll-\* to have a chance of taking effect to begin with?
> That's my impression based on a quick look at reftest-content.js, but I
> haven't tested to confirm.

No, the reftest-async-scroll-* attributes are applied as long as there is a reftest-async-scroll attribute present [1]. If there is no displayport explicitly set, we'll get the default displayport that the code sets up, which can result in checkerboarding if it's not large enough. You may have been thinking of [2] which is the inverse - you need to have reftest-async-scroll set on the root in order for subframes' displayports to get set up.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js?rev=46180e3cd0c6#310
[2] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest-content.js?rev=46180e3cd0c6#257

> Does this give the scroll handler a chance to run?

Hm, actually the setTimeout doesn't seem to be needed. I think it might be left over from an earlier version of the test. The scroll handler gets triggered after the call to scrollTo, during the next paint cycle. I can take it out.

Comment 51

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

Comment on attachment 8721017 [details]
MozReview Request: Bug 1246290 - Refactoring to get rid of SampleContentTransformForFrame from AsyncPanZoomController. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35541/diff/3-4/

Comment 52

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

Comment on attachment 8721018 [details]
MozReview Request: Bug 1246290 - Add a bit to FrameMetrics to indicate if APZ-scrolling should be disabled on that APZC. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35543/diff/3-4/

Comment 53

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

Comment on attachment 8721019 [details]
MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35545/diff/3-4/

Comment 54

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

Comment on attachment 8721020 [details]
MozReview Request: Bug 1246290 - Add a pref to allow disabling APZ on documents which have scroll-linked effects. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35547/diff/3-4/

Comment 55

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

Comment on attachment 8721021 [details]
MozReview Request: Bug 1246290 - Add a reftest for the apz.disable_for_scroll_linked_effects pref. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35549/diff/3-4/

Comment 56

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

Comment on attachment 8736420 [details]
MozReview Request: Bug 1246290 - Add a simple gtest to exercise the force-disabled-APZ codepaths. r?botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43277/diff/1-2/

Comment 57

[Botond Ballo [:botond]]

Comment on attachment 8721019 [details]
MozReview Request: Bug 1246290 - Add the ability for APZCs to not expose async transforms. r?botond

https://reviewboard.mozilla.org/r/35545/#review40467

::: gfx/layers/apz/src/AsyncPanZoomController.h:723
(Diff revision 4)
> +   * the metrics has the mForceDisableApz flag set. In general the latter should
> +   * only be used by call sites that are applying the transform to update
> +   * a layer's position.
> +   */
> +  enum AsyncMode {
> +    NORMAL,

We're a bit inconsistent with our casing convention for enumerator names. GestureBehavior, AxisLockMode, and PanZoomState use YELLING_CASE, while HitTestingResult, CancelAnimationFlags, ScrollSource, PixelCastJustification, and RendertraceProperty use CamelCase.

I don't care very much, but we may want to standardize on one at some point.

::: gfx/layers/composite/ContainerLayerComposite.cpp:815
(Diff revision 4)
>      // on it down to the bottom using GetFirstChild and not worry about walking onto another
>      // underlying layer.
>      for (LayerMetricsWrapper i(aContainer); i; i = i.GetFirstChild()) {
>        if (AsyncPanZoomController* apzc = i.GetApzc()) {
>          if (!apzc->GetAsyncTransformAppliedToContent()
> -            && !AsyncTransformComponentMatrix(apzc->GetCurrentAsyncTransform()).IsIdentity()) {
> +            && !AsyncTransformComponentMatrix(apzc->GetCurrentAsyncTransform(AsyncPanZoomController::NORMAL)).IsIdentity()) {

Making this RESPECT_FORCE_DISABLE would cause this warning box to appear for force-disabled APZCs. Might we want that, as a diagnostic tool?

Comment 58

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

(In reply to Botond Ballo [:botond] from comment #57)
> Making this RESPECT_FORCE_DISABLE would cause this warning box to appear for
> force-disabled APZCs. Might we want that, as a diagnostic tool?

I didn't think that would be particularly useful, since if we have a document with a scroll-linked effect all the APZCs in it will be disabled, and there's already a console warning to warn when that happens. However in practice I don't use this diagnostic tool ever so if you feel it would be more useful to change I'm happy to do it.

Comment 59

[Botond Ballo [:botond]]

Comment on attachment 8736420 [details]
MozReview Request: Bug 1246290 - Add a simple gtest to exercise the force-disabled-APZ codepaths. r?botond

https://reviewboard.mozilla.org/r/43277/#review40473

Comment 60

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

Latest try push is also clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=254ed714c259&group_state=expanded

Comment 61

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9de61f3bbcc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a515266e0dba
https://hg.mozilla.org/integration/mozilla-inbound/rev/110538d89648
https://hg.mozilla.org/integration/mozilla-inbound/rev/224ef65456d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e76831655536
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ebc66fa9bb

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9de61f3bbcc
https://hg.mozilla.org/mozilla-central/rev/a515266e0dba
https://hg.mozilla.org/mozilla-central/rev/110538d89648
https://hg.mozilla.org/mozilla-central/rev/224ef65456d2
https://hg.mozilla.org/mozilla-central/rev/e76831655536
https://hg.mozilla.org/mozilla-central/rev/b0ebc66fa9bb

Comment 63

[Andrei Vaida [:avaida]]

Kats, is there something Manual QA can help with here? 

If that is the case, could you please expand a bit on what should we focus on while testing this feature?

Comment 64

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

I think it would be worth a little bit of manual testing, yes. The way this feature works is if you go to about:config and enable the apz.disable_for_scroll_linked_effects pref, it turns off APZ on certain pages where we detect a "scroll linked effect". The most obvious example is the python docs page in bug 997269 - with the pref set to false you can see the menu on the left lagging behind the scroll, but with the pref set to true you see the menu stays fixed. All of the open bugs that are dependent on this bug have similar issues that should be fixed by this pref. So manual testing would be:
- Enable the pref, and make sure the dependent bugs no longer manifest
- Also test some other popular websites and make sure that scrolling is still smooth and behaves well.

We are planning to leave the pref off by default for now, although we may decide to flip it on later.

Comment 65

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

Not worth uplifting since e10s (and therefore APZ) is not going to release on 47 (as per the current plan).

Comment 66

[Andrei Vaida [:avaida]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #64)
> I think it would be worth a little bit of manual testing, yes. The way this
> feature works is if you go to about:config and enable the
> apz.disable_for_scroll_linked_effects pref, it turns off APZ on certain
> pages where we detect a "scroll linked effect". The most obvious example is
> the python docs page in bug 997269 - with the pref set to false you can see
> the menu on the left lagging behind the scroll, but with the pref set to
> true you see the menu stays fixed. All of the open bugs that are dependent
> on this bug have similar issues that should be fixed by this pref. So manual
> testing would be:
> - Enable the pref, and make sure the dependent bugs no longer manifest
> - Also test some other popular websites and make sure that scrolling is
> still smooth and behaves well.
> 
> We are planning to leave the pref off by default for now, although we may
> decide to flip it on later.

Thank you for taking the time to explain how it works, it's very helpful. We'll make sure this gets looked at as soon as possible -- it's very likely that we're only going to be able to test this feature on Aurora 48.

Comment 67

[Petruta Horea [:phorea], Desktop Test Engineering]

I've verified all the blocking bugs and filed follow-ups where needed.
I've also tested high load websites having the pref set to both True and False values, across platforms. 

Marking as verified based on this.