1119355 - [Settings] User is able to use slider and scroll at the same time

Status: VERIFIED FIXED

(Core :: Panning and Zooming, defect)

Description

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

Attachment: Slider Issue Logcat

Description:
Some of the sliders in the settings of the phone can be scrubbed left to right at the same time the user is scrolling the current page up and down


Repro Steps:
1) Update a Flame to 20150108010221
2) Open Settings app> Select "Display"
3) Tap on the brightness slider line and hold
4) scroll up and down


Actual:
User is able to scroll up and down and move the slider left and right at the same time


Expected:
User is able to slide brightness left and right OR scroll up and down. Not both simultaneously.

Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150108010221
Gaia: d4dac29613076bdba3cb8adc217deadb08a2ac20
Gecko: 70de2960aa87
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 37.0a1 (2.2)
Firmware Version: v18D
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Notes: The "Sound" sliders are not affected in settings, but the "System Alarm Volume" slider that is found when creating a new alarm IS affected.

Repro frequency: 10/10 100%
See attached: Logcat, Video - http://youtu.be/XDiqe5xXFrQ

Comment 1

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This issue does NOT occur on Flame 2.1

The user is only able to scroll OR use the slider

Device: Flame 2.1 (319mb)(Kitkat)(Full Flash)
BuildID: 20150108001214
Gaia: ed2e278753e8c9301ba322dcf2c3591f5928408d
Gecko: 127a0ead5f83
Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76
Version: 34.0 (2.1)
Firmware: V18D
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Comment 2

[Peter Bylenga [:PBylenga]]

Functional regression leading to poor UX.

Requesting a window.

Comment 3

[Brogan Zumwalt [Inactive]]

Mozilla-Inbound Regression Window:

Last Working Mozilla-Inbound Build:
20150102135212
First Broken Mozilla-Inbound Build:
20150102140811

Working:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20150102135212
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 9689b122bc76
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Broken:
Device: Flame 2.2 Master (319mb)(Kitkat Base)(Shallow Flash)
BuildID: 20150102140811
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 8564f04a6f14
Version: 37.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 8564f04a6f14

First Broken gaia / Last Working gecko - Issue does NOT occur
Gaia: 698e6e8a098cc060b26cd6f25171633c4c7e739d
Gecko: 9689b122bc76

Gecko Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9689b122bc76&tochange=8564f04a6f14

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

Kartikaya, can you take a look at this issue please? This seems to have been caused by the uplift for bug 928833.

Comment 5

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

I can repro, investigating.

Comment 6

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

Attachment: Treat nsRangeFrame as having touch listeners

This happens with the event-regions enabled because the <input type="range"> element reacts to touch events, but does so without explicitly listening for touch events via the listener mechanism. So layout doesn't know to put range inputs into the dispatch-to-content region, and APZ doesn't wait for the main thread to process those events before scrolling.

My fix basically always puts nsRangeFrames into the dispatch-to-content region, but I'm open to other suggestions on how to fix this. f? to jwatt also in case he's aware of some more general way to detect frames/content that have specific behaviour in response to touch events.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8546725 [details] [diff] [review]
Treat nsRangeFrame as having touch listeners

Review of attachment 8546725 [details] [diff] [review]:
-----------------------------------------------------------------

This code is super-hot since it's in BuildDisplayListForChild, so I begrudge the virtual call added here.

How about we have nsRangeFrame add a dummy touch listener? That would obviate the need for SetHasTouchEventListeners in nsRangeFrame::Init too.

Comment 8

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

Attachment: Make nsRangeFrame register a dummy touch listener

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> 
> This code is super-hot since it's in BuildDisplayListForChild, so I begrudge
> the virtual call added here.

Fair point.

> 
> How about we have nsRangeFrame add a dummy touch listener? That would
> obviate the need for SetHasTouchEventListeners in nsRangeFrame::Init too.

Sounds good! I totally forgot about the SetHasTouchEventListeners code in nsRangeFrame::Init. Updated patch attached.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8547662 [details] [diff] [review]
Make nsRangeFrame register a dummy touch listener

Review of attachment 8547662 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet!

Comment 10

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

Unfortunately that failed my try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac05b6cda84c) as the refcounted DummyTouchListener has a public destructor. I'll make the destructor private and make mDummyTouchListener an nsRefPtr<> instead.

Comment 11

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

Try with that change is green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0a97660e9b9

Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fa41494e58

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/86fa41494e58

Comment 13

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

Comment on attachment 8547662 [details] [diff] [review]
Make nsRangeFrame register a dummy touch listener

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 928833
User impact if declined: User is able to scroll the page while adjusting range input elements. This can be disconcerting.
Testing completed: locally
Risk to taking this patch (and alternatives if risky): fairly low-risk
String or UUID changes made by this patch: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/5453ae19d3f6

Comment 15

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame Master.

Result: The user is unable to scroll the page when moving the slider in Settings > Display and Alarm.
 
Device: Flame Master
BuildID: 20150121010204
Gaia: 5e98dc164b17fd6decb48a9eaddef0e55b82e249
Gecko: 540077a30866
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 ( Master)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
===========================================
Leaving verifyme for 2.2 verification.

Comment 16

[KTucker [:KTucker][Inactive 3/4/2016]]

Yeojin, can you please verify this on 2.2?

Comment 17

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.2.

Result: The user is unable to scroll the page when moving the slider in Settings > Display and Alarm.

Environmental Variables:
Device: Flame 2.2 (319mb, full flash)
BuildID: 20150121002607
Gaia: e4f9b5da3751798f9cc5d95f302c30722cc11fca
Gecko: 75a462a58d7a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware: V18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0