1328658 - Google Spreadsheets scrollbar dragging no longer works

Status: VERIFIED FIXED

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

Description

[Chris Peterson [:cpeterson]]

[Tracking Requested - why for this release]:

Kevin, I think this bug is a regression from enabling APZ scrollbar dragging in bug 1324117.

STR:
1. Load a Google Spreadsheet such as https://docs.google.com/spreadsheets/d/1-sxk6DRmTUPagh5nrZkRwvE9ewLzB6P7A8yulSTH-F0/
2. Try to drag the vertical scrollbar within the spreadsheet.

RESULT:
Nothing happens! The spreadsheet doesn't scroll.

I am running Nightly 53 on a Windows 10 laptop.

Comment 1

[Botond Ballo [:botond]]

This sounds like bug 1326290.

Comment 2

[Marcia Knous [:marcia]]

Changing status to get it off the rel health dashboard.

Comment 3

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

[Tracking Requested - why for this release]:

Reopening since it turns out that fixing bug 1326290 didn't fix it for everybody. cpeterson is still seeing this issue, as am I. There is a more detailed description of the symptoms in bug 1326290 comment 0 and bug 1326290 comment 25.

Comment 4

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

(That being said I'm not sure if this actually worth tracking for 53 because it's a nightly-only regression and will not ride the trains until it's good and ready)

Comment 5

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

So, (unsurprisingly) this seems to be because the page is doing some shifty shenanigans with respect to the scrollbar. The scrollbar is a "real" scrollbar in that it is drawn by the OS, but the thing it's really scrolling is a 1-pixel-wide div. There are additional mouse and scroll listeners that do the actual scrolling of the spreadsheet based on how this 1-px div scrolls and what the mouse is doing. The layer tree on the page has a layer with vscrollbar=9 but I don't see any scrollId=9 anywhere, so that's also kind of fishy.

Comment 6

[Marcia Knous [:marcia]]

Tracking 53+ to keep this regression on the radar.

Comment 7

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

Attachment: Layers dump

Here's a sample layers dump

Comment 8

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

Parking this with me for further investigation, as Botond is unable to reproduce this on his Win 10 device.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Any update now that this is on Aurora?

Comment 10

[Botond Ballo [:botond]]

It shouldn't be on aurora - APZ scrollbar dragging is #ifdef NIGHTLY_BUILD [1].

[1] http://searchfox.org/mozilla-central/rev/bf98cd4315b5efa1b28831001ad27d54df7bbb68/modules/libpref/init/all.js#636

Comment 11

[Botond Ballo [:botond]]

[Tracking Requested - why for this release]:
Just carrying over the tracking flag from 53. The regressing feature is nightly-only for now, so 53 is no longer affected.

Comment 12

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

Thanks Botond. I'll dig into this more tomorrow when I'm back at home and have access to the machine that repro's this. It's been on my radar, just busy with other things.

Comment 13

[Marcia Knous [:marcia]]

Tracking 54+ per Comment 11.

Comment 14

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

So what seems to be happening here is a mismatch between the layout code and the layer tree. The layer that gets generated has a scrollbar layer (i.e. a layer tagged with vscrollbar=<scrollid>) but there is no layer with that scrollid in the layer tree. I'm not really sure what's causing that, but the result is that nsSliderFrame.cpp gets mouse events and thinks the scrollbar can be async-dragged. StartAPZDrag completes successfully, and so nsSliderFrame sets mScrollingWithAPZ to true [1]. However, the scroll target id that gets sent over to APZ in the AsyncDragMetrics is bogus, in that there's no layer or APZ matching it. So APZ ends up eating those events. The result is that neither the main thread nor the APZ code ends up doing the scrolling.

Right now there's mechanism for APZ to tell the main-thread code "hey, that scroll id is bogus". APZ could easily enough detect, even during mouse-down, that there's no matching content, but the main-thread side code would still go ahead and set mScrollingWithAPZ. So I think the fix for this needs to be on the layout side of things.

I tried (briefly) getting a display list dump and FLB dump to see what happened to the layer but it was getting truncated. I'll try harder tomorrow.

[1] http://searchfox.org/mozilla-central/rev/7da3c9dcf467964f2fb82f3a4c63972ee79bf696/layout/xul/nsSliderFrame.cpp#1138

Comment 15

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> So, (unsurprisingly) this seems to be because the page is doing some shifty
> shenanigans with respect to the scrollbar. The scrollbar is a "real"
> scrollbar in that it is drawn by the OS, but the thing it's really scrolling
> is a 1-pixel-wide div.

If that 1-pixel-wide div doesn't draw anything then there won't be any layer with that scrollid of the scrollbar. So what you said makes sense.

Comment 16

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

Ok, so if that's expected behaviour then we need to change the nsSliderFrame.cpp code to detect this case and return false from StartAPZDrag. We can probably expose the mScrollableByAPZ flag from nsIScrollableFrame for this purpose.

Comment 17

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

Attachment: Bug 1328658 - Restrict APZ scrollbar dragging to when we know the content is APZ-scrollable.

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

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8831159 [details]
Bug 1328658 - Restrict APZ scrollbar dragging to when we know the content is APZ-scrollable.

https://reviewboard.mozilla.org/r/107778/#review108944

::: layout/xul/nsSliderFrame.cpp:1009
(Diff revision 1)
>  
> -  // APZ dragging requires the scrollbar to be layerized, which doesn't
> -  // happen for scroll info layers.
> -  if (ScrollFrameWillBuildScrollInfoLayer(scrollFrame)) {
> +  // APZ dragging requires both the scrollbar and content to be layerized.
> +  // This check checks that the content is layerized, and that the content
> +  // isn't a scrollinfo layer (which therefore means the scrollbar is also
> +  // layerized).
> +  if (!scrollFrameAsScrollable->IsScrollableByAPZ()) {

Does this work when the scrollable layer doesn't have a displayport at the time the drag is started?

I recall considering doing this for bug 1331693, but then deciding against it, because mScrollableByAPZ is only set the first time APZ sends content a repaint request [1], so it won't be true in nsSliderFrame::StartAPZDrag() if the displayport was just created earlier in the same TabChild::RecvRealMouseButtonEvent() call [2] that's upstack from StartAPZDrag().

[1] http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/gfx/layers/apz/util/APZCCallbackHelper.cpp#136
[2] http://searchfox.org/mozilla-central/rev/8fa84ca6444e2e01fb405e078f6d2c8da0e55723/dom/ipc/TabChild.cpp#1614

Comment 19

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

Hm, good point. It probably doesn't work in that case. So what are our options then? I think the only thing left is to add some machinery that runs after the client-side layer tree is built, looks at what scroll ids are available, and stashes a list somewhere that the nsSliderFrame can query. Maybe we can use the retained layer tree for this, I'm not sure.

Comment 20

[Botond Ballo [:botond]]

I wonder if we could do something along these lines:

  - Have APZ "respond" to the StartAsyncScrollbarDrag request with 
    a "yes" or a "no".

  - If the response is a "no", have nsSliderFrame revert to
    main-thread scrolling.

If we can get this to work, we could rely on it to catch the scroll info layer case as well if we wanted, and it would also catch other cases that might arise in the future where APZ can't accept the drag for some reason.

Comment 21

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

Comment on attachment 8831159 [details]
Bug 1328658 - Restrict APZ scrollbar dragging to when we know the content is APZ-scrollable.

Dropping review request for now. Botond is working on an alternate approach, as described above.

Comment 22

[Botond Ballo [:botond]]

Oh yeah whoops, I meant to clear that review. I hope to have something to post here later today.

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

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

Comment 24

[Botond Ballo [:botond]]

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

Kats, could you give this patch a whirl and see if it fixes the issue?

Comment 25

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

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

Yup, this patch seems to do the job!

Comment 26

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

... although it might be disabling APZ scrollbar drag on other pages too. Let me investigate more.

Comment 27

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #26)
> ... although it might be disabling APZ scrollbar drag on other pages too.
> Let me investigate more.

Ignore this, user error on my part.

Comment 28

[Botond Ballo [:botond]]

Posting patch for review, since it seems to be working.

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8831159 [details]
Bug 1328658 - Restrict APZ scrollbar dragging to when we know the content is APZ-scrollable.

Marking patch for old approach as obsolete to avoid confusion.

Comment 30

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

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

https://reviewboard.mozilla.org/r/108912/#review110746

r+ with comments addressed.

::: gfx/layers/apz/src/APZCTreeManager.cpp:56
(Diff revision 1)
>  
>  typedef mozilla::gfx::Point Point;
>  typedef mozilla::gfx::Point4D Point4D;
>  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
>  
> +typedef CompositorBridgeParent::LayerTreeState LayerTreeState;

Unrelated change? Probably doesn't belong in this patch.

::: gfx/layers/apz/src/APZCTreeManager.cpp:456
(Diff revision 1)
>  
> +void
> +APZCTreeManager::NotifyScrollbarDragRejected(const ScrollableLayerGuid& aGuid) const
> +{
> +  if (LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(aGuid.mLayersId)) {
> +    if (state->mController) {

We probably want to write this more like FlushApzRepaints is written, so that we can assert (state && state->mController). If we don't find a mController here then the rejection never goes through and we're back to square one, so it would be good to catch those scenarios.

::: gfx/layers/ipc/PAPZ.ipdl:66
(Diff revision 1)
>    async NotifyMozMouseScrollEvent(ViewID aScrollId, nsString aEvent);
>  
>    async NotifyAPZStateChange(ScrollableLayerGuid aGuid, APZStateChange aChange, int aArg);
>  
>    async NotifyFlushComplete();
> +  

nit: whitespace

::: layout/generic/nsGfxScrollFrame.cpp:6259
(Diff revision 1)
> +    for (nsIFrame* frame : childLists.CurrentList()) {
> +      if (nsSliderFrame* sliderFrame = do_QueryFrame(frame)) {
> +        sliderFrame->AsyncScrollbarDragRejected();
> +      }

Timothy should probably review this bit. I'm not sure of the exact frame-tree layout of the slider frame and the scrollboxes.

::: layout/xul/nsSliderFrame.cpp:1546
(Diff revision 1)
> +  mScrollingWithAPZ = false;
> +
> +  if (!mSuppressionActive) {
> +    MOZ_ASSERT(PresContext()->PresShell());
> +    APZCCallbackHelper::SuppressDisplayport(true, PresContext()->PresShell());
> +    mSuppressionActive = true;
> +  }

This code is duplicated in StopDrag(), can we factor it out into a helper?

Comment 31

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> ::: gfx/layers/apz/src/APZCTreeManager.cpp:56
> (Diff revision 1)
> >  
> >  typedef mozilla::gfx::Point Point;
> >  typedef mozilla::gfx::Point4D Point4D;
> >  typedef mozilla::gfx::Matrix4x4 Matrix4x4;
> >  
> > +typedef CompositorBridgeParent::LayerTreeState LayerTreeState;
> 
> Unrelated change? Probably doesn't belong in this patch.

I'm introducing a new reference to LayerTreeState in this patch, so I think of it as akin to renaming a variable as part of a change that adds new uses of it. 

Will keep unless you feel strongly about extracting it to a separate patch.

> ::: gfx/layers/apz/src/APZCTreeManager.cpp:456
> (Diff revision 1)
> >  
> > +void
> > +APZCTreeManager::NotifyScrollbarDragRejected(const ScrollableLayerGuid& aGuid) const
> > +{
> > +  if (LayerTreeState* state = CompositorBridgeParent::GetIndirectShadowTree(aGuid.mLayersId)) {
> > +    if (state->mController) {
> 
> We probably want to write this more like FlushApzRepaints is written, so
> that we can assert (state && state->mController). If we don't find a
> mController here then the rejection never goes through and we're back to
> square one, so it would be good to catch those scenarios.

Good point, done.

> ::: gfx/layers/ipc/PAPZ.ipdl:66
> (Diff revision 1)
> >    async NotifyMozMouseScrollEvent(ViewID aScrollId, nsString aEvent);
> >  
> >    async NotifyAPZStateChange(ScrollableLayerGuid aGuid, APZStateChange aChange, int aArg);
> >  
> >    async NotifyFlushComplete();
> > +  
> 
> nit: whitespace

Fixed. (Eclipse's remove-trailing-whitespace-on-save feature is specific to C++ file types...)

> ::: layout/generic/nsGfxScrollFrame.cpp:6259
> (Diff revision 1)
> > +    for (nsIFrame* frame : childLists.CurrentList()) {
> > +      if (nsSliderFrame* sliderFrame = do_QueryFrame(frame)) {
> > +        sliderFrame->AsyncScrollbarDragRejected();
> > +      }
> 
> Timothy should probably review this bit. I'm not sure of the exact
> frame-tree layout of the slider frame and the scrollboxes.

I did talk to Timothy about this and wrote it following his guidance, but for good measure I'll flag him for review on this part.

> ::: layout/xul/nsSliderFrame.cpp:1546
> (Diff revision 1)
> > +  mScrollingWithAPZ = false;
> > +
> > +  if (!mSuppressionActive) {
> > +    MOZ_ASSERT(PresContext()->PresShell());
> > +    APZCCallbackHelper::SuppressDisplayport(true, PresContext()->PresShell());
> > +    mSuppressionActive = true;
> > +  }
> 
> This code is duplicated in StopDrag(), can we factor it out into a helper?

StopDrag() does the opposite, but the end of StartDrag() does have this exact code. I introduced helpers SuppressDisplayport() and UnsuppressDisplayport().

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

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

Comment 33

[Botond Ballo [:botond]]

Timothy, I'm asking you for review specifically on the changes to layout/generic/nsGfxScrollFrame.cpp. Feel free to ignore the rest of the patch :)

Comment 34

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

(In reply to Botond Ballo [:botond] from comment #31)
> Will keep unless you feel strongly about extracting it to a separate patch.

I don't feel strongly.

> > ::: layout/xul/nsSliderFrame.cpp:1546
> > (Diff revision 1)
> > > +  mScrollingWithAPZ = false;
> > > +
> > > +  if (!mSuppressionActive) {
> > > +    MOZ_ASSERT(PresContext()->PresShell());
> > > +    APZCCallbackHelper::SuppressDisplayport(true, PresContext()->PresShell());
> > > +    mSuppressionActive = true;
> > > +  }
> > 
> > This code is duplicated in StopDrag(), can we factor it out into a helper?
> 
> StopDrag() does the opposite, but the end of StartDrag() does have this
> exact code. I introduced helpers SuppressDisplayport() and
> UnsuppressDisplayport().

Oh, my bad - I misread the code. In that case is there a way we can check to make sure that a drag is still in progress before we activate displayport suppression? I'm concerned that the mouse drag may have ended by the time this rejection notice comes back, and then we start suppression too late and never unsuppress (or at least not right away).

Comment 35

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #34)
> Oh, my bad - I misread the code. In that case is there a way we can check to
> make sure that a drag is still in progress before we activate displayport
> suppression? I'm concerned that the mouse drag may have ended by the time
> this rejection notice comes back, and then we start suppression too late and
> never unsuppress (or at least not right away).

Good catch! I modified AsyncScrollbarDragRejected() to guard the call to SuppressDisplayport() on isDraggingThumb().

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

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

Comment 37

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8832713 [details]
Bug 1328658 - Notify main thread of a failed attempt to start an APZ scrollbar drag.

https://reviewboard.mozilla.org/r/108912/#review110836

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8f467ac2fa6c
Notify main thread of a failed attempt to start an APZ scrollbar drag. r=kats,tnikkel

Comment 39

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f467ac2fa6c

Comment 40

[ovidiu boca[:Ovidiu]]

I verified this issue on FF Beta 54.0b5 on Mac OS X 10.10, Ubuntu 16.04, Windows 10 x32 and MacBook Pro OS X 10.12 and I can't reproduce the initial problem. I will mark this as a verified fix.