Closed Bug 1341992 Opened 7 years ago Closed 7 years ago

On Windows, it's not possible to scroll using the scrollbar with touch gestures

Categories

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

Product:
Core
Component:
Panning and Zooming
Version:
51 Branch
Platform:
All
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 + verified
firefox55 --- verified

People

(Reporter: julienw, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [gfx-noted])

Attachments

(1 file)

Bug 1341992 - Only do hit-testing with the ignore-root-scroll-frame flag on zoomable platforms.
(deleted), text/x-review-board-request
tnikkel
: review+
jcristau
: approval-mozilla-beta+
Details 
STR:
1. Use a touch-enabled laptop on Windows. (Here it's a Windows 10 laptop)
2. Go on a page with vertical scrolling.
3. Try to touch the scrollbar to scroll.

=> The page scrolls as if the scrollbar is absent, not _with_ the scrollbar.
This makes it uneasy to scroll quickly on a long page.

Tried with Stable and Nightly.

This is working in Chrome and Edge on the same computer.


May be related to bug 1328065 ?
This works for me. If you go to about:support, what does it say under "Asynchronous Pan/Zoom"?
Flags: needinfo?(felash)
I have on Nightly:
wheel input enabled; touch input enabled; scrollbar drag enabled

While checking this, I also noticed that on about:support this actually works.

Examples of pages where this doesn't:
* https://everlong.org/blog
* https://linuxfr.org
* www.lemonde.fr

On Stable:
wheel input enabled only

Same behavior exactly: works on about:support, but not on the other pages.

On Stable I also see that the momentum works in about:support but not elsewhere.


Additional notes:
On Stable I had to set "browser.tabs.remote.force-enable" to true to enable e10s because of the accessibility API used by the touch interface (fixed in Beta). (On Nightly I don't need to set the pref to have e10s)

If I set it back to false, then I can scroll with scrollbar and touch.
Flags: needinfo?(felash)
Ok, I can reproduce this. It's probably another hit-testing error in APZ, although maybe we never hooked up touch-dragging of the scrollbar at all? I don't remember :/
Priority: -- → P3
Whiteboard: [gfx-noted]
Version: unspecified → 51 Branch
To be clear, I think this is a regression from when APZ is enabled. When APZ isn't then this works as expected.

The overall scrolling experience is a lot better with APZ in general, but for long pages where you'd need to scroll quicker this is still a real annoyance. I watched a real user -- my girlfriend -- using Firefox and she's not happy.
Keywords: regression
Yeah, that's fair. I'll see if I can find some time to dig into this.
status-firefox52: --- → affected
status-firefox53: --- → affected
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Also going to mark 51 as unaffected since APZ is off by default there. To the end user this will be a regression in 52 as e10s/APZ/touch is enabled.
status-firefox51: affected → unaffected
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Also going to mark 51 as unaffected since APZ is off by default there.

Do you mean touch support is off by default there?
(In reply to Botond Ballo [:botond] [standards meeting Feb 27 - Mar 4] from comment #7)
> Do you mean touch support is off by default there?

Well, e10s is off on touch devices, which means APZ is off on touch devices.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> (In reply to Botond Ballo [:botond] [standards meeting Feb 27 - Mar 4] from
> comment #7)
> > Do you mean touch support is off by default there?
> 
> Well, e10s is off on touch devices, which means APZ is off on touch devices.

FTR, this was bug 1325676 (Windows 8) and bug 1323521 (Windows 10)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> Well, e10s is off on touch devices, which means APZ is off on touch devices.

Looks like this bug is no harm to 52 on touch devices(e10s off and APZ off).
Correct me if I'm wrong.
status-firefox52: affected → fix-optional
I'm pretty sure 52 is affected, as bug 1323521 has been fixed in 52. But I haven't tried.
Yes, 52 is affected.
Well, it's a bit confusing to me. Anyway, it's too late to fix it on 52 now.
Parking this with me. I'll be low-priority background work for me, so if anybody has cycles to dedicate to this feel free to steal it.
Assignee: nobody → bugmail
Kats, maybe a good first step is to identify and describe what the issue is so that a newcomer could try to fix it more easily. Thanks :)
I don't think this is a good bug for a newcomer. It requires a fair amount of familiarity with the code and will require investigation into what is going on. I myself don't know offhand what the root cause/fix is here.
Too late for a fix for 53. 

Kats, given that this may affect google sheets behavior, do you want to bump the priority here or get help with investigating?
status-firefox53: affected → wontfix
status-firefox55: --- → ?
tracking-firefox54: --- → +
Flags: needinfo?(bugmail)
Too much to do, not enough time. I think anybody who could help here is already tied up with higher-priority quantum work. I'll leave this assigned to me for now.
Flags: needinfo?(bugmail)
I tested this issue on Windows 8.1 with the touch screen on FF Nightly 55.0a1(2017-05-11), I used the sites that are presented in the description and when I want to grab the scrollbar using my finger it doesn't work, it can't be selected. I also set the apz.drag on false and the result was the same.
status-firefox55: ? → affected
I chased this down - the scrollbar does already support being dragged around by touch, the problem is just that the hit-testing code isn't finding the scrollbar as the target. And only for touch events - mouse events work fine.

The problem seems to be this:
1) At [1] we use a flag of INPUT_IGNORE_ROOT_SCROLL_FRAME when doing the hit-test for touch events. This flag is not used for normal mouse events [2].
2) The INPUT_IGNORE_ROOT_SCROLL_FRAME ends up setting the ignore-scroll-frame property on the display list builder that is used for the hit test [3].
3) The ignoringThisScrollFrame flag becomes true at [4] for the root scrollframe, because aBuilder->GetIgnoreScrollFrame() == mOuter.
4) We then enter the block at [5], and set addScrollBars to false at [6] (in this condition, the aBuilder->IsForEventDelivery() is true, so that causes addScrollBars to end up false - however, usingDisplayPort is also false, because aBuilder->IsPaintingToWindow() is false at [7]).

Anyhow, this results in the scroll display items not getting added to the display list, which means the hit-test misses them, and so the touch event ends up hitting the background of the page instead.

This code flow also explains why scrollbars of subframes *are* touch-draggable, because for subframes the ignoringThisScrollFrame flag is false and we unconditionally add the scrollbar display items.

I suspect that we want to not use the INPUT_IGNORE_ROOT_SCROLL_FRAME flag on desktop because I think it only makes sense to use when pinch zooming is enabled.

[1] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/base/PresShell.cpp#7480
[2] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/base/PresShell.cpp#7558
[3] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/base/nsLayoutUtils.cpp#3272
[4] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/generic/nsGfxScrollFrame.cpp#3268
[5] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/generic/nsGfxScrollFrame.cpp#3311
[6] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/generic/nsGfxScrollFrame.cpp#3319
[7] http://searchfox.org/mozilla-central/rev/2933592c4a01b634ab53315ce2d0e43fccb82181/layout/generic/nsGfxScrollFrame.cpp#3286
Comment on attachment 8870977 [details]
Bug 1341992 - Only do hit-testing with the ignore-root-scroll-frame flag on zoomable platforms.

https://reviewboard.mozilla.org/r/142542/#review146166
Attachment #8870977 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e72ec95b6b62
Only do hit-testing with the ignore-root-scroll-frame flag on zoomable platforms. r=tnikkel
Comment on attachment 8870977 [details]
Bug 1341992 - Only do hit-testing with the ignore-root-scroll-frame flag on zoomable platforms.

Approval Request Comment
[Feature/Bug causing the regression]: touch event support on desktop
[User impact if declined]: user cannot touch-drag scrollbars to scroll on the root scrollable frame (windows and linux)
[Is this code covered by automated tests?]: not really, dragging is pretty hard to automate
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes. STR in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: small risk
[Why is the change risky/not risky?]: it's a small one-line patch that's basically the safest of the various options we considered to fix this. the fix affects only hit-testing of touch events on desktop platforms so if there's any fallout it would be limited to that.
[String changes made/needed]: none
Attachment #8870977 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/e72ec95b6b62
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi :julienw,
Can you help check if this issue is fixed in the latest nightly?
Flags: needinfo?(felash)
the fix works as expected when i test it on today's nightly with a win10 touch-screen device.
status-firefox55: fixed → verified
Setting qe-verify+ per comment 29, STR in comment 0
Flags: qe-verify+
Comment on attachment 8870977 [details]
Bug 1341992 - Only do hit-testing with the ignore-root-scroll-frame flag on zoomable platforms.

fix for touch on windows, verified in nightly, should be in 54.0b12
Attachment #8870977 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170601030206

Verified fixed using the latest Nightly(20170601030206)on Windows 8.1 with the touch screen.
Latest nightly works, but it's not smooth when the site is still loading. Once the site is loaded this is smooth.
Scrolling by swiping inside the page is always smooth even while loading.

I tried on http://lemonde.fr.

Should I file a separate bug ?
Flags: needinfo?(felash)
No need, it's covered by bug 1367765.
I also tested this issue on Windows 8.1 with Nightly 55.0a1(2017-06-05) using this site: https://staktrace.com/resources/extras/spout/scroll-behavior.html and the scrolling is not smooth even after the site is loaded.
That's expected, it is also covered by the bug referenced in comment 38.
Thanks for clearing that out, I can confirm the fix. I verified this on Windows 8.1 x64 with the touch screen on Nightly 55.0a1 (2017-06-06).
Status: RESOLVED → VERIFIED
Verified using Firefox 54.0 RC with a Microsoft Surface Pro 2 device running Win 10 Insider Preview build.
Dragging the scrollbar on about:config and https://people-mozilla.org/~kgupta/stall.html is also slow to respond, but considering that bug 1367765 will fix those too, I'm marking Firefox 54 as verified.
status-firefox54: fixed → verified
Seems edge case enough for the ESR52 crowd that we can probably call it wontfix. Feel free to change the status back and request approval if you feel otherwise, though.
status-firefox-esr52: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: