Closed Bug 1423011 Opened 2 years ago Closed Last year

Allow APZ to async-scroll the layout viewport, and have the main-thread scroll position reflect the layout viewport offset

Categories

(Core :: Panning and Zooming, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox59 --- wontfix
firefox63 --- fixed

People

(Reporter: botond, Assigned: kashav)

References

(Depends on 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 12 obsolete files)

59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
59 bytes, text/x-review-board-request
botond
: review+
Details
A major part of making our viewport handling more consistent with other browsers (bug 1123938) is fleshing out the distinction between the "visual viewport" and the "layout viewport" (a.k.a. "fixed viewport"). See [1] for some background on these concepts.

Gecko already has a distinction between these two concepts, although it doesn't call them those names:

  - The "visual viewport" is the rectangle whose origin is
    APZ's scroll offset for the RCD-RSF [2], and whose size is
    the composition bounds of the RCD-RSF.

  - The "layout viewport" is the rectangle whose origin is
    Gecko's scroll offset for the RCD-RSF, and whose size is
    the "CSS viewport size" (as computed by MobileViewportManager
    on mobile).

We successfully leverage this distinction to make zooming of overflow:hidden pages work much like it does in Chrome.

Of course, there are numerous other ways in which our behaviour in relation to these is different from other browsers; for example, we currently attach position:fixed elements to the visual viewport rather than the layout viewport (bug 656036).

One important change we'll need to make to implement some of these behaviours is to allow APZ to async-scroll the layout viewport. That is, APZ will need to track a copy of Gecko's scroll offset for the RCD-RSF which it can change asynchronously (but which is still distinct from the visual viewport offset of the RCD-RSF). This way, for example, fixed-position elements can be asynchronously translated to remain fixed to the layout viewport when the latter is async-scrolled.

[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md
[2] "Root content document's root scroll frame"
Blocks: 1336544
Priority: -- → P3
Whiteboard: [gfx-noted]
Some more detailed notes on this:

* Most of the changes here will be to the class AsyncPanZoomController [1] ("APZC" for short), which is the main APZ object for managing scrolling / zooming of a single scroll frame.

* This notion of having two viewports does not apply to all scroll frames, only to the RCD-RSF. In terms of code, that would be the APZC for which mFrameMetrics.mIsRootContent [2] [3] is true.

* The origin of the visual viewport is the value we already store in mFrameMetrics.mScrollOffset [4].

* The size of the visual viewport is the value we already store in mFrameMetrics.mCompositionBounds [5].

* The size of the layout viewport is the value we already store in mFrameMetrics.mViewport [6].

* The origin of the layout viewport is what we don't currently store, but need to.

* Any operation that moves the visual viewport (modifies mScrollOffset) potentially needs to move the layout viewport as well. The general logic is that if the new location of the visual viewport is no longer contained within the layout viewport, the layout viewport should be moved enough that it is contained. (You can see this behaviour in the demo [7]: once you drag the red rectangle (visual viewport) to the edge of the blue rectangle (layout viewport), it starts to drag the blue rectangle along with it.)

[1] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/apz/src/AsyncPanZoomController.h#147
[2] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/apz/src/AsyncPanZoomController.h#871
[3] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/FrameMetrics.h#663
[4] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/FrameMetrics.h#617
[5] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/FrameMetrics.h#544
[6] https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/gfx/layers/FrameMetrics.h#650
[7] http://bokand.github.io/viewport/index.html
(In reply to Botond Ballo [:botond] from comment #1)
> * The size of the layout viewport is the value we already store in
> mFrameMetrics.mViewport [6].
> 
> * The origin of the layout viewport is what we don't currently store, but
> need to.

On second thought, mViewport is a rect, so we can just use its origin (which I believe we currently ignore) to store the layout viewport origin.
As we are working through this, we realized that as part of this change, the RCD-RSF scroll offset stored by the main thread (in the nsIScrollableFrame) will change to be the layout viewport offset for all pages. (Currently, it is the layout viewport offset for overflow:hidden pages, and the visual viewport offset otherwise.)
Blocks: 1465616
Depends on: 1466611
Comment on attachment 8984296 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250108/#review256406

Cool stuff!

I'll review these when I get a chance, but in the meantime, please push the patch series to Try so that we can see whether the new tests are passing in automation, and whether the code changes break any existing tests. Thanks!
A few tests (e.g. [1]) are failing because of the removal of the overflow:hidden special case in ScrollFrameTo.

From my understanding, this is because the checks are still required for non-root frames. Would it be correct to re-add them but only for non-root frames? Is there any other way?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6405b602233a147725c5341b46f25f667b6e764&selectedJob=182414166
Flags: needinfo?(botond)
(In reply to Kashav Madan [:kashav] from comment #8)
> A few tests (e.g. [1]) are failing because of the removal of the
> overflow:hidden special case in ScrollFrameTo.
> 
> From my understanding, this is because the checks are still required for
> non-root frames. Would it be correct to re-add them but only for non-root
> frames? Is there any other way?

The checks shouldn't be required for non-root frames, because non-root frames cannot be zoomed, and in the absence of zooming, you shouldn't be able to scroll an overflow:hidden frame in the relevant direction at all (that is, APZ's scrollable rect should be sized in such a way that APZ knows not to generate a scroll offset change in the relevant direction in the first place).

All that is to say, I don't know why the tests are failing, but the above would be my direction of investigation.
Flags: needinfo?(botond)
(In reply to Botond Ballo [:botond] [standards meeting June 4-9] from comment #10)
> (that is, APZ's scrollable rect should be sized in such a way that APZ knows
> not to generate a scroll offset change in the relevant direction in the
> first place).

That should be handled by these checks here:

https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/layout/base/nsLayoutUtils.cpp#8730
Depends on: 1467873
(In reply to Botond Ballo [:botond] [standards meeting June 4-9] from comment #10)
> The checks shouldn't be required for non-root frames, because non-root
> frames cannot be zoomed, and in the absence of zooming, you shouldn't be
> able to scroll an overflow:hidden frame in the relevant direction at all
> (that is, APZ's scrollable rect should be sized in such a way that APZ knows
> not to generate a scroll offset change in the relevant direction in the
> first place).

The tests seem to pass again (locally) when we re-add the removed checks. Investigating why mochitests are timing out on Linux and Android (any ideas?), but will look more into this soon.
Comment on attachment 8984294 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/250104/#review256500

Thanks, this generally looks very good! I have a few comments below.

We'll also need to figure out the remaining test failures in the Try push before landing.

::: gfx/layers/FrameMetrics.h:518
(Diff revision 1)
>      return mIsScrollInfoLayer;
>    }
>  
> +  // Determine if the visual viewport is outside of the layout viewport and
> +  // adjust the x,y-offset in mViewport accordingly. This is necessary to
> +  // allow APZ to async-scroll the layout viewport.

Please add:

// This is a no-op if mIsRootContent is false.

::: gfx/layers/FrameMetrics.cpp:20
(Diff revision 1)
>  
>  void
> +FrameMetrics::RecalculateViewportOffset()
> +{
> +  CSSRect cb = CalculateCompositedRectInCssPixels();
> +  if (!mIsRootContent || mViewport.Contains(cb + mScrollOffset)) {

Couple of comments on this line:

- Check |mIsRootContent| before calling CalculateCompositedRectInCssPixels() (to avoid unnecessary computation).
- As explained in bug 1467873, the origin of the rect returned by CalculateCompositedRectInCssPixels() is not in the same coordinate space as mScrollOffset, so it does not make sense to add the two together. Instead, the visual viewport rectangle can be computed as CSSRect(mScrollOffset, CalculateCompositedSizeInCSSPixels()). (Feel free to get a GetVisualViewportRect() helper which returns that.)

::: gfx/layers/FrameMetrics.cpp:22
(Diff revision 1)
> +FrameMetrics::RecalculateViewportOffset()
> +{
> +  CSSRect cb = CalculateCompositedRectInCssPixels();
> +  if (!mIsRootContent || mViewport.Contains(cb + mScrollOffset)) {
> +    return;
> +  }

Let's MOZ_ASSERT that the visual viewport is no larger than the layout viewport in either dimension.

::: gfx/layers/FrameMetrics.cpp:25
(Diff revision 1)
> +  if (!mIsRootContent || mViewport.Contains(cb + mScrollOffset)) {
> +    return;
> +  }
> +  if (mScrollOffset.X() < mViewport.X()) {
> +    mViewport.MoveToX(mScrollOffset.X());
> +  } else if (mScrollOffset.X() + cb.Width() > mViewport.XMost()) {

If you store the visual viewport rect in a variable, you can use |visualViewport.XMost()| in place of |mScrollOffset.X() + cb.Width()|.

::: gfx/layers/FrameMetrics.cpp:30
(Diff revision 1)
> +  } else if (mScrollOffset.X() + cb.Width() > mViewport.XMost()) {
> +    mViewport.MoveByX(mScrollOffset.X() + cb.Width() - mViewport.XMost());
> +  }
> +  if (mScrollOffset.Y() < mViewport.Y()) {
> +    mViewport.MoveToY(mScrollOffset.Y());
> +  } else if (mScrollOffset.Y() + cb.Height() > mViewport.YMost()) {

Similarly, you could use visualViewport.YMost() here.

::: gfx/layers/apz/src/AsyncPanZoomController.h:669
(Diff revision 1)
>     * new one can properly take effect.
>     */
>    nsEventStatus OnCancelTap(const TapGestureInput& aEvent);
>  
>    /**
> +   * Scroll the scroll frame to an X,Y offset and recalculate the viewport

I would add a single comment like this at the top of this group of methods:

/**
 * The following five methods modify the scroll offset.
 * For the APZC representing the RCD-RSF, they also recalculate
 * the offset of the layout viewport.
 */

Then, the comments above the individual methods do not need to mention the recalculation.

::: gfx/layers/apz/src/AsyncPanZoomController.h:676
(Diff revision 1)
> +   */
> +  void SetScrollOffset(const CSSPoint& aOffset);
> +
> +  /**
> +   * Scroll the scroll frame to an X,Y offset, clamping the resulting scroll
> +   * offset to the scrollable rect and recalculate the viewport offset.

The scroll offset is clamped to the "scroll range", not the scrollable rect. The scroll range is the scrollable rect minus the visual viewport size.

For example, if the scrollable rect is (0,0,400,400), and the visual viewport size is (100,100), the scroll range is (0,0,300,300), and this is what the scroll offset is clamped to. (Since, if the scroll offset is at (300,300), you're alredy scrolled to the bottom-right corner of the page).

::: layout/base/nsLayoutUtils.cpp:9125
(Diff revision 1)
>    if (scrollableFrame) {
>      nsPoint scrollPosition = scrollableFrame->GetScrollPosition();
>      metrics.SetScrollOffset(CSSPoint::FromAppUnits(scrollPosition));
>  
> +    CSSRect viewport = metrics.GetViewport();
> +    viewport.MoveTo(CSSPoint::FromAppUnits(scrollPosition));

CSSPoint::FromAppUnits(scrollPosition) was just computed two lines above. Let's factor it out into a variable.
Attachment #8984294 - Flags: review?(botond)
I'd also like for Kats to have a look at the changes to APZCCallbackHelper, which I'm somewhat uncertain about.
Attachment #8984294 - Flags: review?(bugmail)
(In reply to Botond Ballo [:botond] from comment #15)
> I'd also like for Kats to have a look at the changes to APZCCallbackHelper,
> which I'm somewhat uncertain about.

(The reason I'm uncertain about it is that we've previously modified this code to shift the displayport in the !scrollUpdated case in bug 978248, but that caused a regression and we had to revert it in bug 1010119. But that was 4 years ago, the affected scenario was on B2G, and the code has changed a lot since then, and performing the shift is now necessary for correctness...)
Comment on attachment 8984294 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/250104/#review257050

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
(Diff revision 1)
> -  if (aFrame->GetScrollbarStyles().mVertical == NS_STYLE_OVERFLOW_HIDDEN) {
> -    targetScrollPosition.y = geckoScrollPosition.y;
> -  }
> -  if (aFrame->GetScrollbarStyles().mHorizontal == NS_STYLE_OVERFLOW_HIDDEN) {
> -    targetScrollPosition.x = geckoScrollPosition.x;
> -  }

If my understanding of this patch is correct, then I agree that removing this code is the right thing to do. If this is the root content and overflow:hidden, then the targetScrollPosition (which is now the layout viewport topleft) should be the same as geckoScrollPosition, because APZ should not allow scrolling in such a way that moves the layout viewport. And if this is overflow:hidden but NOT the root content, then the code at [1] should have set the scrollable rect to a size that prevents APZ from scrolling it at all.

However, it would be good add assertions to this effect here, so that we can scenarios where our understanding/assumptions are wrong. So I would keep the if conditions as-is, but replace the assignment inside the condition with a MOZ_ASSERT(targetScrollPosition.y == geckoScrollPosition.y); and equivalent for .x. And update the comment to reflect what I said above as to why these assertions should hold.

[1] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/base/nsLayoutUtils.cpp#8730-8740

::: gfx/layers/apz/util/APZCCallbackHelper.cpp:154
(Diff revision 1)
> +    // APZ uses the visual viewport's offset to calculate where to place the
> +    // display port, so the display port is misplaced when a pinch zoom occurs.

It's not clear to me under what scenario we'd enter this codepath to begin with. We would need to be in a case where scrollUpdated==false on the root content. For scrollUpdated to be false, one of the following three conditions need to have been true in ScrollFrameTo:
1) !aFrame - this is unlikely for the root content
2) !aMetrics.GetScrollOffsetUpdated() - this is unlikely after a pinch zoom, because the repaint triggered by a pinch zoom will be marked as triggered by a user action
3) scrollInProgress - while this is certainly possible it should also be rare. It would mean something triggered a main thread scroll concurrently with the user pinch zooming.

So while these situations could occur, I don't think they are exercised by any of our existing tests and it seems unlikely that you might have run into any of them during testing. So I suspect something else is going on here that I don't fully understand and that prompted you to add this code. Can you describe the scenario in which this codepath gets hit, and specifically let me know which of the above three conditions is getting hit? Understanding that will help me think about what the correct behaviour is in this codepath (and it may very well be that you have it right, I'm just not sure)
Attachment #8984294 - Flags: review?(bugmail)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17)
> If my understanding of this patch is correct, then I agree that removing
> this code is the right thing to do. If this is the root content and
> overflow:hidden, then the targetScrollPosition (which is now the layout
> viewport topleft) should be the same as geckoScrollPosition, because APZ
> should not allow scrolling in such a way that moves the layout viewport. And
> if this is overflow:hidden but NOT the root content, then the code at [1]
> should have set the scrollable rect to a size that prevents APZ from
> scrolling it at all.

Yup, exactly.

> However, it would be good add assertions to this effect here, so that we can
> scenarios where our understanding/assumptions are wrong. So I would keep the
> if conditions as-is, but replace the assignment inside the condition with a
> MOZ_ASSERT(targetScrollPosition.y == geckoScrollPosition.y); and equivalent
> for .x. And update the comment to reflect what I said above as to why these
> assertions should hold.

Good idea!

> ::: gfx/layers/apz/util/APZCCallbackHelper.cpp:154
> (Diff revision 1)
> > +    // APZ uses the visual viewport's offset to calculate where to place the
> > +    // display port, so the display port is misplaced when a pinch zoom occurs.
> 
> It's not clear to me under what scenario we'd enter this codepath to begin
> with. We would need to be in a case where scrollUpdated==false on the root
> content. For scrollUpdated to be false, one of the following three
> conditions need to have been true in ScrollFrameTo:
> 1) !aFrame - this is unlikely for the root content
> 2) !aMetrics.GetScrollOffsetUpdated() - this is unlikely after a pinch zoom,
> because the repaint triggered by a pinch zoom will be marked as triggered by
> a user action
> 3) scrollInProgress - while this is certainly possible it should also be
> rare. It would mean something triggered a main thread scroll concurrently
> with the user pinch zooming.
> 
> So while these situations could occur, I don't think they are exercised by
> any of our existing tests and it seems unlikely that you might have run into
> any of them during testing. So I suspect something else is going on here
> that I don't fully understand and that prompted you to add this code. Can
> you describe the scenario in which this codepath gets hit, and specifically
> let me know which of the above three conditions is getting hit?
> Understanding that will help me think about what the correct behaviour is in
> this codepath (and it may very well be that you have it right, I'm just not
> sure)

We definitely ran into this during testing, on a regular basis.

You're right that most repaints requests occur with aUserAction=true, including the one triggered by the end of pinch-zoom, but it's sufficient for one repaint to happen with aUserAction=false (and then to not get any other repaint with aUserAction=true right after) to mess up our displayport.

The repaint request with aUserAction=false was coming from NotifyLayersUpdated, though I don't remember which codepath was causing needContentRepaint to be set to true. Kashav, do you remember, or could you check?
Flags: needinfo?(kmadan)
(In reply to Botond Ballo [:botond] from comment #18)
> The repaint request with aUserAction=false was coming from
> NotifyLayersUpdated, though I don't remember which codepath was causing
> needContentRepaint to be set to true. Kashav, do you remember, or could you
> check?

No need. I can believe that NotifyLayersUpdated would send random repaint requests with aUserAction=false, it does that sort of thing quite often.

In that case, I can't think of any specific problem this would cause, but I'd feel more comfortable if we updated the "  } else if (aMetrics.IsRootContent()) {" condition to also have " && aMetrics.GetScrollOffset() != aMetrics.GetViewport().TopLeft()" to capture the fact that we need to do this when the async scroll offset has diverged from the layout viewport. This should limit any fallout from the change and make it a little more self-documenting.
Flags: needinfo?(kmadan)
Attachment #8984296 - Attachment is obsolete: true
Attachment #8984296 - Flags: review?(botond)
Attachment #8984295 - Attachment is obsolete: true
Attachment #8984295 - Flags: review?(botond)
Comment on attachment 8984294 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/250104/#review257278

The changes look good, thanks! Just a couple of minor comments.

::: gfx/layers/FrameMetrics.h:471
(Diff revisions 1 - 3)
>    const CSSRect& GetViewport() const
>    {
>      return mViewport;
>    }
>  
> +  const CSSRect GetVisualViewport() const

If you're returning something by value, there's no point in making the return type |const|.

::: gfx/layers/FrameMetrics.cpp:19
(Diff revisions 1 - 3)
>  const FrameMetrics::ViewID FrameMetrics::NULL_SCROLL_ID = 0;
>  
>  void
>  FrameMetrics::RecalculateViewportOffset()
>  {
> -  CSSRect cb = CalculateCompositedRectInCssPixels();
> +  CSSRect visualViewport;

Declare this below where it's initalized

::: gfx/layers/FrameMetrics.cpp:25
(Diff revisions 1 - 3)
> -  if (!mIsRootContent || mViewport.Contains(cb + mScrollOffset)) {
> +  if (!mIsRootContent) {
>      return;
>    }
> -  if (mScrollOffset.X() < mViewport.X()) {
> -    mViewport.MoveToX(mScrollOffset.X());
> -  } else if (mScrollOffset.X() + cb.Width() > mViewport.XMost()) {
> +  visualViewport = GetVisualViewport();
> +  if (mViewport.Contains(visualViewport) ||
> +      mViewport.Height() < visualViewport.Height() ||

Please add a comment explaining why the second and third conditions are necessary: because when the composition bounds changes (due to e.g. a device orientation change or a browser window resize), it can take a few frames for mViewport to be updated, and durign that time the visual viewport could be larger.
Attachment #8984294 - Flags: review?(botond)
Comment on attachment 8985307 [details]
Bug 1423011 - Part 2: Add gtests.

https://reviewboard.mozilla.org/r/250930/#review257290

Looks good, thanks! One minor comment:

::: gfx/layers/apz/test/gtest/TestPinching.cpp:22
(Diff revision 3)
>  
>  protected:
>    FrameMetrics GetPinchableFrameMetrics()
>    {
>      FrameMetrics fm;
> -    fm.SetCompositionBounds(ParentLayerRect(200, 200, 100, 200));
> +    // Offset in composition bounds must be (0, 0) for the RCD-RSF.

Now that we're no longer adding the scroll offset to the composition bounds origin in RecalculateViewportOffset(), this is not a  requirement any more.

We can keep the numerical change (as it reflects reality better, since we only allow zooming the RCD-RSF and its offset usually is (0,0)), but let's remove the comment because the "must be" is misleading.
Attachment #8985307 - Flags: review?(botond)
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review257336

::: gfx/layers/apz/test/mochitest/helper_pinched_pan.html:40
(Diff revision 3)
> +      var target = document.getElementById('content');
> +      var scrollbar = getScrollbarSize();
> +
> +      // Pan horizontally to exceed the right boundary by 50 pixels.
> +      yield synthesizeNativeTouchDrag(target, 0, 0,
> +          -(layout.width - scrollbar.width + TOUCH_SLOP + 50), 0, testDriver);

As discussed, this calculation happens to be correct when the resolution is 2, but it would be better to express it in a more general way where the resolution is a parameter to the calculation, and the calculation is correct with any resolution value.

I *suspect* (but I'm not sure) that if we do this, we'll no longer need the scrollbar size.

::: gfx/layers/apz/test/mochitest/helper_pinched_pan.html:42
(Diff revision 3)
> +
> +      // Pan horizontally to exceed the right boundary by 50 pixels.
> +      yield synthesizeNativeTouchDrag(target, 0, 0,
> +          -(layout.width - scrollbar.width + TOUCH_SLOP + 50), 0, testDriver);
> +      yield flushApzRepaints(testDriver);
> +      is(window.scrollX, 25, "main thread offset-x changed");

Likewise, let's express the 25 as '50 / resolution'.

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:41
(Diff revision 3)
>  
>  var isWindows = (getPlatform() == "windows");
>  touch_action_prefs.push(["apz.test.fails_with_native_injection", isWindows]);
>  
> +var pan_action_prefs = basic_pan_prefs.slice();
> +pan_action_prefs.push(["apz.allow_zooming", true], ["dom.meta-viewport.enabled", true])

These prefs have to do with zooming, not panning, so I would call them zoom_prefs.

(Also drop the word "action". In touch_action_prefs, it refers to the CSS touch-action property.)

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:93
(Diff revision 3)
>    {'file': 'helper_touch_action_complex.html', 'prefs': touch_action_prefs},
>    // Tests that touch-action CSS properties are handled in APZ without waiting
>    // on the main-thread, when possible
>    {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
> +
> +  // Tests that pan while we're pinched in and check that the main thread

"pinched in" -> "zoomed in" (or "pinch-zoomed in")

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:95
(Diff revision 3)
>    // on the main-thread, when possible
>    {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
> +
> +  // Tests that pan while we're pinched in and check that the main thread
> +  // receives layout viewport changes from APZ.
> +  {'file': 'helper_pinched_pan.html', 'prefs': pan_action_prefs},

I would call this helper_zoomed_pan.html, as we're not actually doing the pinching in the test.
Attachment #8985308 - Flags: review?(botond)
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review257452

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:34
(Diff revisions 3 - 5)
>    // position is synced back to the main thread. So we disable displayport
>    // expiry for these tests.
>    ["apz.displayport_expiry_ms", 0],
>  ];
>  
> -var touch_action_prefs = basic_pan_prefs.slice(); // make a copy
> +var touch_prefs = basic_pan_prefs.slice(); // make a copy

Sorry, I think I was unclear before: I was explaining what the "action" in "touch_action" meant, and why it shouldn't be in the name of our new variable. We should leave the name of "touch_action_prefs" itself alone.

Apologies for the confusion.

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:33
(Diff revision 5)
> +    utils.setResolutionAndScaleTo(RESOLUTION);
> +
> +    function getVisualBounds(resolution) {
> +      var prevResolution = {value: 1};
> +      utils.getResolution(prevResolution);
> +      utils.setResolutionAndScaleTo(resolution);

I'm concerned that changing the resolution back and forth (and particularly doing so inside test(), which is called after waitUntilApzStable()) is going to introduce intermittent issues.

I would rather getVisualBounds() not touch the resolution, and we call it at the top, before and after setting the resolution, respectively.

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:35
(Diff revision 5)
> +    function getVisualBounds(resolution) {
> +      var prevResolution = {value: 1};
> +      utils.getResolution(prevResolution);
> +      utils.setResolutionAndScaleTo(resolution);
> +      var bounds = {
> +        h: document.documentElement.clientHeight,

Let's avoid one-letter variable names and call the fields 'height' and 'width'.

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:46
(Diff revision 5)
> +
> +    function computeDelta(layout, visual) {
> +      // Compute the distance from the right / bottom edge of the visual
> +      // viewport to the same edge of the layout viewport and add the desired
> +      // offset to that.
> +      return (layout * RESOLUTION) - visual + OFFSET_SCREEN_PX + TOUCH_SLOP;

Makes sense now, thanks!

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:62
(Diff revision 5)
> +                                      -computeDelta(unit.w, curr.w),
> +                                      0,
> +                                      testDriver);
> +      yield flushApzRepaints(testDriver);
> +      is(window.scrollX, OFFSET_CSS_PX, "x-offset was adjusted");
> +      is(window.scrollY, 0, "y-offset was not effected");

effected -> affected

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:71
(Diff revision 5)
> +                                      0,
> +                                      0,
> +                                      -computeDelta(unit.h, curr.h),
> +                                      testDriver);
> +      yield flushApzRepaints(testDriver);
> +      is(window.scrollX, OFFSET_CSS_PX, "x-offset was not effected");

likewise
Attachment #8985308 - Flags: review?(botond)
One more thing: since Tanushree has contributed to these patches as well, please reflect that by including her in the commit author field.

If you have changeset evolution enabled, you can edit an author field like this:

hg up <commit>
hg commit --amend --user "Kashav Madan <kmadan@mozilla.com>, Tanushree Podder <tpodder@mozilla.com>"
hg evolve --all
Comment on attachment 8985307 [details]
Bug 1423011 - Part 2: Add gtests.

https://reviewboard.mozilla.org/r/250930/#review257514
Attachment #8985307 - Flags: review?(botond) → review+
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review257516

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:52
(Diff revisions 5 - 7)
>        return (layout * RESOLUTION) - visual + OFFSET_SCREEN_PX + TOUCH_SLOP;
>      }
>  
>      function* test(testDriver) {
> +      console.log(unit);
> +      console.log(zoom);

I don't think console.log() goes to any log you can access in a mochitest. I usually use |dump("string\n");| to print something to the log from a mochitest (I believe that's a facility provided by the mochitest harness).

In any case, I assume you intend to remove any logging statements before landing.
Attachment #8985308 - Flags: review?(botond)
Comment on attachment 8984294 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/250104/#review257518

Thanks, this looks good now. I'll hold off on giving r+ until the overflow:hidden test failure is resolved (as the fix will probably require changes to this patch).
Attachment #8984294 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #46)
> I don't think console.log() goes to any log you can access in a mochitest. I
> usually use |dump("string\n");| to print something to the log from a
> mochitest (I believe that's a facility provided by the mochitest harness).
> 
> In any case, I assume you intend to remove any logging statements before
> landing.

Yeah, forgot to remove them, updated.

For clarification, console.log output is viewable in the browser window, by removing the |subtestDone| call, so that the test window doesn't close automatically.
(In reply to Kashav Madan [:kashav] from comment #49)
> For clarification, console.log output is viewable in the browser window, by
> removing the |subtestDone| call, so that the test window doesn't close
> automatically.

Ah, for local runs on desktop. Makes sense!
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review258236

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:70
(Diff revisions 9 - 10)
> -                                      0,
> -                                      -computeDelta(visualDimensions.width),
> -                                      0,
> +                                        c.y,
> +                                        c.dx(document.documentElement.clientWidth),
> +                                        c.dy(document.documentElement.clientHeight),
> -                                      testDriver);
> +                                        testDriver);
> -      yield flushApzRepaints(testDriver);
> +        yield flushApzRepaints(testDriver);
> -      is(window.scrollX, OFFSET_CSS_PX, "x-offset was adjusted");
> +        yield waitForApzFlushedRepaints(testDriver);

We should not need both of these calls, just one should be sufficient (probably waitForApzFlushedRepaints() to be safe).

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:101
(Diff revisions 9 - 10)
>    // Tests that touch-action CSS properties are handled in APZ without waiting
>    // on the main-thread, when possible
>    {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
> +];
>  
> +if (!isAndroid()) {

I'd rather have this logic in the subtest file itself, similar to:

https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/layers/apz/test/mochitest/helper_bug1464568_opacity.html#52

::: gfx/layers/apz/test/mochitest/test_group_touchevents.html:102
(Diff revisions 9 - 10)
>    // on the main-thread, when possible
>    {'file': 'helper_touch_action_regions.html', 'prefs': touch_action_prefs},
> +];
>  
> +if (!isAndroid()) {
> +  // TODO: Determine why adding this subtest is causing timeouts on Android

Please mention in the comment the bug number of the bug tracking the enablement of this test on Android.
Attachment #8985308 - Flags: review?(botond)
A couple of things I realized:

  - Suggesting a test run without any of our patches to make
    sure the test-verify failure isn't a pre-existing issue,
    was somewhat misguided, because test-verify is special in
    that it doesn't run _every_ test 20 times, only the ones
    modified by the commits being pushed. We'd have to make a
    dummy modification to test_group_touchevents.html for
    us to actually get the baseline results we want.

  - Taking a closer look at the test-verify failure log, it
    seems the tests really are timing out because they are
    running very slowly, rather than because something is
    waiting for a notification that's never arriving.

I'm basing this latter point on the following section of the failure log:

[task 2018-06-20T21:47:36.824Z] 21:47:36     INFO -  41 INFO TEST-START | gfx/layers/apz/test/mochitest/test_group_touchevents.html
[task 2018-06-20T21:57:54.931Z] 21:57:54     INFO -  <snipped 12 output lines - if you need more context, please use SimpleTest.requestCompleteLog() in your test>
[task 2018-06-20T21:57:54.931Z] 21:57:54     INFO -  Buffered messages logged at 21:50:12
[task 2018-06-20T21:57:54.931Z] 21:57:54     INFO -  42 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_group_touchevents.html | helper_scrollto_tap.html?true | Document element didn't get painted
[task 2018-06-20T21:57:54.931Z] 21:57:54     INFO -  Buffered messages logged at 21:50:13
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  43 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_group_touchevents.html | helper_scrollto_tap.html?true | Clicked on button, yay! (at 13,54)
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  Buffered messages logged at 21:51:17
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  44 INFO must wait for load
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  Buffered messages logged at 21:51:41
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  45 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_group_touchevents.html | helper_scrollto_tap.html?false | Clicked on button, yay! (at 13,54)
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  Buffered messages logged at 21:51:55
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  46 INFO must wait for load
[task 2018-06-20T21:57:54.932Z] 21:57:54     INFO -  Buffered messages logged at 21:52:00
[task 2018-06-20T21:57:54.933Z] 21:57:54     INFO -  47 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_group_touchevents.html | helper_bug1162771.html | Set display to none on #video
[task 2018-06-20T21:57:54.934Z] 21:57:54     INFO -  Buffered messages logged at 21:52:01
...

You can see the test starts running at 21:47, and times out at 21:57 (suggesting a 10-minute timeout which sounds right). All the output is printed at 21:57 because it's buffered up and only printed at the end, but you can see the real timestamps in the content of some of the messages (e.g. "Buffered messages logged at 21:50:12") show that the test really is making progress over the course of this 10-minute interval, just not fast enough to complete within the 10-minute budget.

This suggests exploring a solution along the lines of splitting test_group_touchevents into multiple test groups, so each has 10 minutes to complete. I know we tried this once, but perhaps we just didn't get the balance of tests between the two pieces right.
Here's an attempt at a split that brought the test-verify failure rate down to 2/5:

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

Maybe we need to split it three ways...
>     We'd have to make a
>     dummy modification to test_group_touchevents.html for
>     us to actually get the baseline results we want.

I made such a dummy modification, pushed it to Try (without any of our patches here applied), and test-verify is failing 10/10 times:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18df9e3faecc61e255908f745321d558c7028a75

This suggests that the timeout is, in fact, a pre-existing issue. (Previously I said I didn't think it was, because the failure rate recorded in bug 1282638 which tracks existing intermittent timeouts of this test is quite low, but that was before I realized that on most Try pushes, test-verify does not run test_group_touchevents.)

So, I would suggest splitting test_group_touchevents into 2 parts for good measure (as the rate of intermittent failure will just go up as we keep adding new tests to it), but otherwise not to worry about this failure.
(In reply to Botond Ballo [:botond] from comment #57)
> So, I would suggest splitting test_group_touchevents into 2 parts for good
> measure (as the rate of intermittent failure will just go up as we keep
> adding new tests to it), but otherwise not to worry about this failure.

Ended up splitting the file into 3 parts as the split seemed more logical, let me know if I should update the commit message to explain why this was required.

Here's a Try push from earlier with this split:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=149fe9f3882c0959f5d4f11f924bfc9ddd1adeff&group_state=expanded
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review258562

Could we separate the splitting of the file into its own patch? And maybe put the common prefs into apz_test_utils.js?
Attachment #8985308 - Flags: review?(botond)
(In reply to Botond Ballo [:botond] from comment #62)
> Comment on attachment 8985308 [details]
> Bug 1423011 - Part 3: Add mochitests.
> 
> https://reviewboard.mozilla.org/r/250932/#review258562
> 
> Could we separate the splitting of the file into its own patch? And maybe
> put the common prefs into apz_test_utils.js?

Sure, will create a new issue.
Depends on: 1470251
Blocks: 1470267
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review258808

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:78
(Diff revision 13)
> +        is(window.scrollY, c.expected.y[0], c.expected.y[1]);
> +      }
> +    }
> +
> +    if (utils.layerManagerType === 'WebRender') {
> +      ok(true, "This test doesn't need to run on WebRender");

No need to disable this for WebRender on a per-file level. We'll disable the entire test group for WR when enabling it for desktop platforms.

::: gfx/layers/apz/test/mochitest/test_group_zoom.html:38
(Diff revision 13)
>  
> +var zoom_prefs = prefs.slice();
> +zoom_prefs.push(
> +  ["apz.allow_zooming", true],
> +  ["browser.chrome.dynamictoolbar", false],
> +  ["dom.meta-viewport.enabled", true],

apz.allow_zooming and dom.meta-viewport.enabled are not needed so long as this is only running on Android. Let's remove them for now, and add them back in the follow-up change to enable the test group on desktop platforms.
Attachment #8985308 - Flags: review?(botond)
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review258812
Attachment #8985308 - Flags: review?(botond) → review+
Comment on attachment 8985308 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/250932/#review258992

::: gfx/layers/apz/test/mochitest/helper_zoomed_pan.html:75
(Diff revision 14)
> +        is(window.scrollX, c.expected.x[0], c.expected.x[1]);
> +        is(window.scrollY, c.expected.y[0], c.expected.y[1]);
> +      }
> +    }
> +
> +    SpecialPowers.GetDOMWindowUtils(window).setResolutionAndScaleTo(RESOLUTION);

s/Get/get

This appears to be the reason tests are timing out:

06-22 15:53:24.396  2332  2348 E GeckoConsole: [JavaScript Error: "TypeError: SpecialPowers.GetDOMWindowUtils is not a function" {file: "http://mochi.test:8888/tests/gfx/layers/apz/test/mochitest/helper_zoomed_pan.html" line: 80}]

(via https://taskcluster-artifacts.net/DWJgbOa5QdKuJB79sTlC8A/0/public/test_info//logcat-emulator-5554.log)
Ah, dynamic typing strikes again :/

(This sort of thing has happened to me many times, and it always frustrates me because it's the sort of thing that gets caught at compile time in C++/Java/Rust.)
Note that the second patch (gtests) will need to be rebased across bug 1468804.
(In reply to Botond Ballo [:botond] from comment #70)
> Note that the second patch (gtests) will need to be rebased across bug
> 1468804.

Done. Also had to update the mochitests due to bug 1448513.
Comment on attachment 8984294 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/250104/#review258878

Going to r+ this as it's looking like the overflow:hidden test failure is an issue with the test, not this code.
Attachment #8984294 - Flags: review?(botond) → review+
Comment on attachment 8989521 [details]
Bug 1423011 - Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html.

https://reviewboard.mozilla.org/r/254558/#review261378

::: dom/events/test/window_wheel_default_action.html:917
(Diff revision 1)
>      } else {
>        doTestCurrentScroll();
>      }
>    }
>  
> +  function doWaitForPaintsAndScroll() {

Currently, this mochitest has other tests apart from doTestScroll with a similar structure to doTestScroll. However, the other tests have not been modified to wait for all paints during prepare. The other similar tests are passing without waiting for paints. However, it makes more sense theoretically to wait for all paints in other tests as well. Botond, what is your opinion on that?

Also, the previous try results did not have the zooming test failure noticed locally in doTestZoomedScroll. Therefore, I have submitted this patch to try to check if the failure is reproducible on try.
Comment on attachment 8989521 [details]
Bug 1423011 - Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html.

https://reviewboard.mozilla.org/r/254558/#review261694

Thanks!

::: commit-message-e0595:8
(Diff revision 1)
> +Scrolling is not permitted along the axis whose overflow style is hidden. In
> +the mochitest test_wheel_default_action, tests which checked if overflow hidden
> +pages were scrollable failed after the changes in the Part 1 patch were
> +submitted to try. The tests failed as overflow hidden pages were scrollable.
> +After investigating the sequence of events, we realized that the
> +overflow-x:hidden property was being set by the main thread after the

I would say "the overflow-x:hidden property was taking effect on the compositor thread after the scroll-wheel event was received on the compositor thread". (It's true that the main-thread paint tha picked up the overflow-x:hidden property also happened after the scroll-wheel event was received, but that's more of a detail.)

::: commit-message-e0595:12
(Diff revision 1)
> +After investigating the sequence of events, we realized that the
> +overflow-x:hidden property was being set by the main thread after the
> +scroll-wheel event was generated on the compositor thread. Therefore, scrolling
> +took place even before the scrollable rect was sized appropriately and
> +scrolling beyond the layout viewport size was possible. To solve this, we wait
> +for all repaints to occur before the test invokes a scroll-wheel event. Waiting

invokes -> sends

::: commit-message-e0595:14
(Diff revision 1)
> +scroll-wheel event was generated on the compositor thread. Therefore, scrolling
> +took place even before the scrollable rect was sized appropriately and
> +scrolling beyond the layout viewport size was possible. To solve this, we wait
> +for all repaints to occur before the test invokes a scroll-wheel event. Waiting
> +for all repaints guarantees that the overflow-x:hidden property will be set
> +before a scroll-wheel event occurs.

occurs -> is received

::: dom/events/test/window_wheel_default_action.html:911
(Diff revision 1)
>      gScrollableElement.scrollLeft = 1000;
>  
>      var currentTest = kTests[currentTestIndex];
>      description = "doTestScroll(aSettings=" + aSettings.description + "), " + currentTest.description + ": ";
>      if (currentTest.prepare) {
> -      currentTest.prepare(doTestCurrentScroll);
> +      currentTest.prepare(doWaitForPaintsAndScroll);

Please add a comment similar to the following:

// prepare() might make a change to the page,
// such as changing an 'overflow' property,
// that requires a repaint to take effect before
// sending events
Attachment #8989521 - Flags: review?(botond)
Comment on attachment 8989521 [details]
Bug 1423011 - Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html.

https://reviewboard.mozilla.org/r/254558/#review261726

The edits to the commit message requested in comment 85 remain to be made.
Attachment #8989521 - Flags: review?(botond)
Comment on attachment 8989521 [details]
Bug 1423011 - Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html.

https://reviewboard.mozilla.org/r/254558/#review262328
Attachment #8989521 - Flags: review?(botond) → review+
Tanushree, your Part 4 patch isn't actually applied on top of the first three patches. (Its ancestor revision is an upstream mozilla-central revision, not the Part 3 patch.)

(I'm not actually sure how you managed to push it without having it replace the other three patches, which is what MozReview usually does...)

Could you please rebase the patch locally onto the Part 3 patch, and re-push all four patches? Thanks!

This also means the Try push from comment 88 is meaningless since it doesn't actually have the first three patches applied, so we'll need to do another Try push after the patches are re-pushed.
Flags: needinfo?(tpodder)
(In reply to Botond Ballo [:botond] from comment #91)
> (I'm not actually sure how you managed to push it without having it replace
> the other three patches, which is what MozReview usually does...)

It looks like MozReview allows multiple separate patch series to be associated with a single bug if they are by different authors.

However, that's not what we want here: we want a single patch series containing all 4 patches, pushed in a single push.

(This is important because MozReview will land each patch series as a unit, which means tests will be run for each one. In our case, we'd have failing tests for the landing of Parts 1-3 without Part 4, causing Parts 1-3 to be backed out.)
Attachment #8990728 - Attachment is obsolete: true
Attachment #8990728 - Flags: review?(botond)
Attachment #8990729 - Attachment is obsolete: true
Attachment #8990729 - Flags: review?(botond)
Attachment #8990730 - Attachment is obsolete: true
Attachment #8990730 - Flags: review?(botond)
Comment on attachment 8990749 [details]
Bug 1423011 - Part 2: Add gtests.

https://reviewboard.mozilla.org/r/255812/#review262572

Carrying over r+ from previous version
Attachment #8990749 - Flags: review?(botond) → review+
Comment on attachment 8990750 [details]
Bug 1423011 - Part 3: Add mochitests.

https://reviewboard.mozilla.org/r/255814/#review262570

Carrying over r+ from previous version
Attachment #8990750 - Flags: review?(botond) → review+
Comment on attachment 8990748 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

Obsoleting old version of patch
Attachment #8990748 - Attachment is obsolete: true
Attachment #8990748 - Flags: review?(botond)
Comment on attachment 8990749 [details]
Bug 1423011 - Part 2: Add gtests.

Obsoleting old version of patch
Attachment #8990749 - Attachment is obsolete: true
Comment on attachment 8990750 [details]
Bug 1423011 - Part 3: Add mochitests.

Obsoleting old version of patch
Attachment #8990750 - Attachment is obsolete: true
Comment on attachment 8990748 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/255810/#review262568

Carrying over r+ from previous version
Attachment #8990748 - Flags: review+
I seem to have obsoleted the wrong attachments...
Attachment #8990748 - Attachment is obsolete: false
Attachment #8990749 - Attachment is obsolete: false
Attachment #8990750 - Attachment is obsolete: false
Attachment #8984294 - Attachment is obsolete: true
Attachment #8985307 - Attachment is obsolete: true
Attachment #8985308 - Attachment is obsolete: true
Flags: needinfo?(tpodder)
MozReview does not seem to support well the use case of different authors alternately pushing to the same bug. It just created another new review series, so I'll need to obsolete some patches again.
Attachment #8989521 - Attachment is obsolete: true
Attachment #8990748 - Attachment is obsolete: true
Attachment #8990749 - Attachment is obsolete: true
Attachment #8990750 - Attachment is obsolete: true
Attachment #8991369 - Flags: review?(botond) → review+
Attachment #8991370 - Flags: review?(botond) → review+
Attachment #8991371 - Flags: review?(botond) → review+
Attachment #8991372 - Flags: review?(botond) → review+
Comment on attachment 8991372 [details]
Bug 1423011 - Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html.

https://reviewboard.mozilla.org/r/256276/#review263228
Comment on attachment 8991369 [details]
Bug 1423011 - Part 1: Allow APZ to async-scroll the layout viewport.

https://reviewboard.mozilla.org/r/256270/#review263222
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/205325147b28
Part 1: Allow APZ to async-scroll the layout viewport. r=botond
https://hg.mozilla.org/integration/autoland/rev/639b78b57760
Part 2: Add gtests. r=botond
https://hg.mozilla.org/integration/autoland/rev/3dc215ffb585
Part 3: Add mochitests. r=botond
https://hg.mozilla.org/integration/autoland/rev/ecafdbe899a7
Part 4: Fix for overflow-x:hidden test failures in the mochitest test_wheel_default_action.html. r=botond
(I'm assigning the bug to Kashav since a bug can have only one assignee, but note that this was a group effort between Kashav and Tanushree.)
Assignee: nobody → kmadan
Depends on: 1478614
Depends on: 1478335
Depends on: 1484597
Depends on: 1482692
Depends on: 1492288
Depends on: 1498812
The title of this bug is somewhat inaccurate: in addition to allowing APZ to scroll the layout viewport, it also changed the main-thread scroll position to reflect the layout viewport offset, and this is in fact the more noticeable / significant change.

Adjusting title to reflect this.
Summary: Allow APZ to async-scroll the layout viewport → Allow APZ to async-scroll the layout viewport, and have the main-thread scroll position reflect the layout viewport offset
Depends on: 1499210
Depends on: 1516056
Depends on: 1516782
Depends on: 1509575
Regressions: 1519007
No longer depends on: 1516056
Regressions: 1516056
You need to log in before you can comment on or make changes to this bug.