1134642 - Unable to tap "confirm" or "cancel" on some settings dialog: APN deletion, data roaming activation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Shared, defect)

Description

[:gerard-majax]

When some system dialog are exposed, I have a very hard time tapping on the "Confirm" or "Cancel" on those.

STR:
 0. Use a device that reproduce. For sure, Aries and Shinano does, I'm pretty sure Hamachi does also. We have not been able to reproduce this on Flame.
 1. Trigger one of the system dialog: data roaming confirmation, APN deletion

Expected:
 I can tap "Cancel" or "Confirm" easily

Actual:
 I'm unable to trigger those touch target, most of the time. After trying dozen of times, if I tap exactly on the text, it seems to be able to trigger the action.

This is a regression for sure, given we see the behavior on Hamachi and it was not there. I'm pretty sure I saw this at least one or two months ago.

Kats, can it be related to touch target work that has been done recently?

Comment 1

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

Maybe. I'll check on Hamachi since it's the only one of the devices you listed that I have. Leaving ni for now.

Comment 2

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

As hamachi is not really supported any more and I can't get it to build successfully I can't investigate myself. At best I can provide a patch for additional logging. Let me know if you want to go that route.

Comment 3

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

So I think the best first step would be to (a) enable the layers tree dumping from the developer settings, (b) enable the APZCTM logging at [1], InputQueue logging at [2], and APZCCH logging at [3]. Grab a log with that and it should get us started with tracking down the problem.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=901326c8a66c#33
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputQueue.cpp?rev=901326c8a66c#16
[3] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/util/APZCCallbackHelper.cpp?rev=901326c8a66c#18

Comment 4

[:gerard-majax]

Attachment: bug-confirm2.log

Comment 5

[:gerard-majax]

Attachment: bug-confirm.log

Comment 6

[:gerard-majax]

Two dumps with all the debug bits asked for enabled. Each one should only match one tap on the Confirm button of the dialog.

Comment 7

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

Not sure if you saw my IRC messages:

13:01:40 kats: gerard-majax: so yeah it looks like the touch events are getting preventDefaulted. but also it looks like the touch events are hitting stuff in the child process. if this is a system dialog then i would expect that to hit stuff in the parent process
13:02:48 kats: gerard-majax: from the layer dump i see most of the stuff in the parent process is in the top 100 pixels of the screen
13:03:26 kats: so if there's a dialog on the screen it's gotta be in the child process

Comment 8

[:gerard-majax]

Yes, that's in the settings app

Comment 9

[:gerard-majax]

From WebIDE I can see that Confirm and Cancel buttons are both with only one 'click' event handler.

Comment 10

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

Not sure what to tell you then. There's definitely a listener somewhere that's calling preventDefault. Maybe WebIDE can't see it because it's registered at the window level or something.

Comment 11

[:gerard-majax]

Can you help us?

Comment 12

[:gerard-majax]

Woot, indeed I found the culprit: the workaround of bug 918288 in https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_confirm/script.js#L20

If I remove the 'touchmove' from there, then it works :)

Comment 13

[GitHub Autolander Bot]

Attachment: [gaia] lissyx:bug1134642 > mozilla-b2g:master

Comment 14

[:gerard-majax]

Attachment: Gaia PR

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8567488 [details] [review]
[gaia] lissyx:bug1134642 > mozilla-b2g:master

It appears that even though bug 918288 has landed, if you remove this workaround you re-introduce what it was trying to fix, the scrolling under the dialog =(

Kats - is there more work needed for APZ testing? Any chance you could give this patch a try and see what's causing us to scroll on the home screen after opening up a confirmation dialog? You should see the dialog if you try to delete an application or smart collection.

Comment 16

[:gerard-majax]

(In reply to Kevin Grandon :kgrandon from comment #15)
> Comment on attachment 8567488 [details] [review]
> [gaia] lissyx:bug1134642 > mozilla-b2g:master
> 
> It appears that even though bug 918288 has landed, if you remove this
> workaround you re-introduce what it was trying to fix, the scrolling under
> the dialog =(

Then we need another fix, because this is badly breaking some features on Shinano and Aries devices, at least :(

Comment 17

[:gerard-majax]

FYI Kevin, I have not seen the behavior you are talking about of moving to the homescreen on the devices I tested.

Comment 18

[Kevin Grandon :kgrandon]

Ok, I tested this again, and it appears to not happen in *all* cases. It does happen on a flame though when trying to remove a smart collection. It won't happen with an app uninstall dialog as we've moved that to the system process, so I guess the dialog must be in the same process, and probably on a scrollable page.

There may be another workaround we can use, but I haven't figured out what that would be yet.

Comment 19

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

I looked into this and it appears the dialog screen is a fixed-position child of the root scrollable element. That's why scrolling on the dialog causes the root scrollable element to scroll (because it's the nearest scrolling ancestor). The same behaviour occurs on e.g. http://chrislord.net/files/mozilla/fixed.html if you start a scroll from on top of one of fixed-position corner blocks. I can see how this is unexpected though, so maybe we should detect that in the APZ code and not let scrolling happen. I can write a patch to do this if that makes sense.

Comment 20

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

Thinking about this more I'm not sure we should change this on the APZ side. Open to discussion over in bug 1135538. The alternative for this bug would be to actually push those dialogs into the parent process, or somehow lift them out of the homescreen's scrollable element. I'm guessing that's the <body> which would would mean we'd have to push the icons into a <div> instead and have the dialog live as a sibling of that div. Not pretty, which is why I'd rather use a parent-process dialog instead.

Comment 21

[Kevin Grandon :kgrandon]

We don't want to put the homescreen scrollable area in a div right now because we are trying to capture mozbrowser scroll events for it in the system app. 

Moving the dialogs out of the home screen might be an option, but that's something a third party developer couldn't really do so it would be good to find a solution for them as well.

Comment 22

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

Could you throw an overflow:hidden on the body while the dialog is in front?

Comment 23

[Kevin Grandon :kgrandon]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> Could you throw an overflow:hidden on the body while the dialog is in front?

It should be possible. I will try to take a look at this later.

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1134642_lissyx_fix_and_overflow_fix > mozilla-b2g:master

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8568877 [details] [review]
[gaia] KevinGrandon:bug_1134642_lissyx_fix_and_overflow_fix > mozilla-b2g:master

I think this works well for now, and is probably a good idea to implement on gaia side so the platform doesn't diverge from Fennec/Android.

Chris - Can you review the home screen changes here? The switch statement abuse is starting to feel a bit bad, but refactoring it is a bigger change than I really want to make right now.

I don't really think this is worth writing a test for yet since the APZ logic is so different on desktop vs device.

Comment 26

[:gerard-majax]

I'm going to give this a try.

Comment 27

[:gerard-majax]

Comment on attachment 8568877 [details] [review]
[gaia] KevinGrandon:bug_1134642_lissyx_fix_and_overflow_fix > mozilla-b2g:master

That still fixes my issue :)

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 8568877 [details] [review]
[gaia] KevinGrandon:bug_1134642_lissyx_fix_and_overflow_fix > mozilla-b2g:master

LGTM.

Comment 29

[Kevin Grandon :kgrandon]

Thanks for the review. In master: https://github.com/mozilla-b2g/gaia/commit/b56d199dd5653313bf0ab03084913fa57ec16dcb

Comment 30

[Tony(Leave from Mozilla)]

This bug has been verified as "pass" on the latest build of Aries KK v2.6 by the STR in comment 0.

Actual results:  user can tap "Cancel" or "Confirm" easily.
See attachment: verified_Aries KK_v2.6.3gp.
Reproduce rate: 0/10.

Device: Aries KK v2.6(master) (Pass)
Build ID               20151105105137
Gaia Revision          e68d693cb55fb5d8946498eb2bdb63f55116d38e
Gaia Date              2015-11-05 01:05:27
Gecko Revision         n/a
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.rose.20151103.153542
Firmware Date          Tue Nov 3 15:36:03 CST 2015
Bootloader             s1

Comment 31

[Tony(Leave from Mozilla)]

Attachment: verified_Aries KK_v2.6.3gp