1128672 - The gaia edge swipe detector eats clicks

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

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

STR:

- In the B2G settings app, go to the developer settings
- Click on the rightmost edge of a checkbox (e.g. "Flash repainted area").

Expected:

- checkbox flips state

Actual:

- it doesn't.

Note that if you click on the "Flash repainted area" text itself it works, it's just the rightmost 30 pixels of the screen that are nonresponsive. The reason this happens is because the edge swipe areas (which you can see if actually enable paint flashing) eat the touch events as well as the mouse events that get generated by the tap. Then they inject new touch events to the content process, but no corresponding mouse events.

This probably didn't happen prior to bug 920036 because the gesture detection happened after going through the edge swipe detector. We will probably need to fiddle with injectTouchEvent to make this work.

Comment 1

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

etienne, do you have any thoughts on if it's easy/possible to redo the edge swipe detector to not rely on injectTouchEvent? That would be preferable to me if possible.

Comment 2

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

A few other things I want to note:

1) There is code at [1] that can be removed now, I think with no negative (or positive) impact.

2) The edge swipe detector panels [2] are styled to be opacity:0 but they still participate in hit testing and event dispatch (obviously, otherwise they wouldn't be able to listen for touch events). However this means that even with the mouseevent preventDefault removed, the mouse events generated from a click in the root process don't end up going to the child process, because they hit the detector panels instead. If the detector panels are going to consume the mouse events like this, then they must be also be responsible for redispatching the mouse events.

[1] https://github.com/mozilla-b2g/gaia/blob/ae5a1580da948c3b9f93528146b007fc4f6a712b/apps/system/js/edge_swipe_detector.js#L99-L105
[2] https://github.com/mozilla-b2g/gaia/blob/ae5a1580da948c3b9f93528146b007fc4f6a712b/apps/system/index.html#L1219-L1220

Comment 3

[GitHub Autolander Bot]

Attachment: [PullReq] staktrace:forward_clicks to mozilla-b2g:master

Comment 4

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

Comment on attachment 8558144 [details] [review]
[PullReq] staktrace:forward_clicks to mozilla-b2g:master

While I would still like to get rid of the injectTouchEvents thing entirely, this seems like a low-risk fix for this bug (i.e. upliftable to 2.2). It's not perfect because the mouse events will get delivered to the child process before the corresponding touch events but that's a relatively minor issue I think.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8558144 [details] [review]
[PullReq] staktrace:forward_clicks to mozilla-b2g:master

Sounds about right.
I'm going to go ahead and just revert part of bug 1072498 this way I don't have to bug you about tests ;)

Comment 6

[GitHub Autolander Bot]

Attachment: [PullReq] etiennesegonzac:bug-1128672 to mozilla-b2g:master

Comment 7

[Etienne Segonzac (:etienne)]

Comment on attachment 8558431 [details] [review]
[PullReq] etiennesegonzac:bug-1128672 to mozilla-b2g:master

Just bringing back some code removed in bug 1072498.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8558144 [details] [review]
> [PullReq] staktrace:forward_clicks to mozilla-b2g:master
> 
> While I would still like to get rid of the injectTouchEvents thing entirely,
> this seems like a low-risk fix for this bug (i.e. upliftable to 2.2). 

Are we uplifting APZC in the main process to 2.2 ? :)

Comment 9

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

Hah, i forgot I removed that code! :) thanks for the patch! And yes, we are planning to uplift parent process APZ to 2.2.

Comment 10

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 11

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Hah, i forgot I removed that code! :) thanks for the patch! And yes, we are
> planning to uplift parent process APZ to 2.2.

Cool! Marking this one as blocking so it gets picked up in the uplift.

Comment 13

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/0f953e0139339b5cf0c726227879bfaa670eb8be

Comment 14

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

Thanks!

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

noming for 2.2 based on comments.

Comment 16

[Gregor Wagner [:gwagner]]

(In reply to Etienne Segonzac (:etienne) from comment #12)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> > Hah, i forgot I removed that code! :) thanks for the patch! And yes, we are
> > planning to uplift parent process APZ to 2.2.
> 
> Cool! Marking this one as blocking so it gets picked up in the uplift.

You still have to ask for uplift approval.

Comment 17

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

I'm not sure if this is safe to uplift by itself (i.e. without the parent process APZ work). If not I can request approval for it when we uplift everything else, since there's a large set of bugs that will need uplifting together.

Comment 18

[Etienne Segonzac (:etienne)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> I'm not sure if this is safe to uplift by itself (i.e. without the parent
> process APZ work). If not I can request approval for it when we uplift
> everything else, since there's a large set of bugs that will need uplifting
> together.

Sounds good.

Comment 19

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

Comment on attachment 8558431 [details] [review]
[PullReq] etiennesegonzac:bug-1128672 to mozilla-b2g:master

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none

Comment 20

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

checkin-needed for uplifting this to 2.2. I just uplifted all the gecko-side patches that go with this, and it would be nice to get this into the same build as those.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/6edc547f94206ad85575e55f66a81e1b1b3fa31c

Comment 22

[Shally Li（leave from Mozilla）]

This bug has been verified as "pass" on latest nightly build of Flame v2.2&master and Nexus5 v2.2&master by the STR in comment 0.

Actual results: Clicking on the rightmost edge of a checkbox (ex: "Flash repainted area"), the checkbox flips state.
See attachment: verified_latest_Flame_v2.2.3gp and verified_latest_Flame_master.3gp 
Reproduce rate: 0/10


-----------------------------------------------------------------------------
Note:
This bug had been fixed on Flame master and Nexus5 master in the past, just as mentioned in comment 13, but later it has changed the design (after changed, it has no response when clicking on the rightmost edge of a checkbox). I don't know which bug modified that.

-----------------------------------------------------------------------------
Device: Flame v2.2 (Verified) 
Build ID               20150720002503
Gaia Revision          e1e6317f17a840b19af9dbb25f5a771d8d9fa161
Gaia Date              2015-07-15 21:05:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e2d1f1f55803
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150720.042247
Firmware Date          Mon Jul 20 04:22:58 EDT 2015
Bootloader             L1TC000118D0

Device: Flame master (Verified)
Build ID               20150720010206
Gaia Revision          3fac3ed7b8c887351098ffc677769ddc36abb3d0
Gaia Date              2015-07-17 17:53:41
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/202e9233d130
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150720.044158
Firmware Date          Mon Jul 20 04:42:10 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 v2.2 (Verified) 
Build ID               20150720002503
Gaia Revision          e1e6317f17a840b19af9dbb25f5a771d8d9fa161
Gaia Date              2015-07-15 21:05:11
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e2d1f1f55803
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150720.041924
Firmware Date          Mon Jul 20 04:19:43 EDT 2015
Bootloader             HHZ12f

Device: Nexus5 master (Verified)
Build ID               20150720160206
Gaia Revision          4fe0507781f3ed56c8ae5e66dd9489165d1ff68e
Gaia Date              2015-07-20 16:09:12
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/d00e4167b482
Gecko Version          42.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150720.191854
Firmware Date          Mon Jul 20 19:19:13 EDT 2015
Bootloader             HHZ12f

Comment 23

[Shally Li（leave from Mozilla）]

Attachment: verified_latest_Flame_v2.2.3gp

Comment 24

[Shally Li（leave from Mozilla）]

Attachment: verified_latest_Flame_master.3gp