Google Spreadsheets scrollbar dragging no longer works

VERIFIED FIXED in Firefox 54

Status

()

P3
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: botond)

Tracking

({regression})

53 Branch
mozilla54
x86_64
Windows 10
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53 unaffected, firefox54+ verified)

Details

(Whiteboard: [gfx-noted], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
[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.
Flags: needinfo?(kevin.m.wern)
(Assignee)

Comment 1

2 years ago
This sounds like bug 1326290.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(kevin.m.wern)
Resolution: --- → DUPLICATE
Duplicate of bug: 1326290
Changing status to get it off the rel health dashboard.
status-firefox53: affected → fix-optional
tracking-firefox53: ? → ---
[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.
Status: RESOLVED → REOPENED
status-firefox53: fix-optional → affected
tracking-firefox53: --- → ?
Depends on: 1326290
Resolution: DUPLICATE → ---
(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)
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.
OS: Windows → Windows 10
Priority: -- → P3
Hardware: Unspecified → x86_64
Whiteboard: [gfx-noted]
Version: unspecified → 53 Branch
Tracking 53+ to keep this regression on the radar.
tracking-firefox53: ? → +
Created attachment 8824477 [details]
Layers dump

Here's a sample layers dump
Parking this with me for further investigation, as Botond is unable to reproduce this on his Win 10 device.
Assignee: nobody → bugmail
Any update now that this is on Aurora?
Flags: needinfo?(bugmail)
(Assignee)

Comment 10

2 years ago
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
(Assignee)

Comment 11

2 years ago
[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.
status-firefox53: affected → unaffected
status-firefox54: --- → affected
tracking-firefox53: + → ---
tracking-firefox54: --- → ?
Flags: needinfo?(bugmail)
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.
Tracking 54+ per Comment 11.
tracking-firefox54: ? → +
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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 hidden (mozreview-request)
(Assignee)

Comment 18

2 years ago
mozreview-review
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
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.
(Assignee)

Comment 20

2 years ago
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 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.
Attachment #8831159 - Flags: review?(botond)
(Assignee)

Comment 22

2 years ago
Oh yeah whoops, I meant to clear that review. I hope to have something to post here later today.
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
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?
Attachment #8832713 - Flags: feedback?(bugmail)
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!
Attachment #8832713 - Flags: feedback?(bugmail) → feedback+
... although it might be disabling APZ scrollbar drag on other pages too. Let me investigate more.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(Assignee)

Updated

2 years ago
Attachment #8832713 - Flags: review?(bugmail)
(Assignee)

Comment 28

2 years ago
Posting patch for review, since it seems to be working.
(Assignee)

Comment 29

2 years ago
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.
Attachment #8831159 - Attachment is obsolete: true

Comment 30

2 years ago
mozreview-review
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?
Attachment #8832713 - Flags: review?(bugmail) → review+
(Assignee)

Comment 31

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 hidden (mozreview-request)
(Assignee)

Comment 33

2 years ago
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 :)
(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).
Assignee: bugmail → botond
(Assignee)

Comment 35

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 hidden (mozreview-request)

Comment 37

2 years ago
mozreview-review
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
Attachment #8832713 - Flags: review?(tnikkel) → review+

Comment 38

2 years ago
Pushed by bballo@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8f467ac2fa6c
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
You need to log in before you can comment on or make changes to this bug.