952324 - Tab modal prompts fail to accept touch input if opened during a touch input event

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Input, defect, P2)

Description

[Rodrigo Silveira [:rsilveira] [:rodms]]

This was initially fixed by bug 840855 but recently bug 944215 regressed it. I can reproduce it very consistently with prompt dialogs in http://dolske.net/mozilla/tests/prompt/sizes.html

Comment 1

[Matt Brubeck (:mbrubeck)]

In particular, this seems to happen if you use a touch screen to *tap* on a link or button that opens a dialog, and then *tap* the button in the dialog.  If you use a mouse to click either or both buttons, it does not cause a problem.

So I think this is caused by the fact that the dialog was opened during a simulated click event that was generated by a touch-screen tap.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #1)
> If you use a mouse to click either or both buttons, it does not cause a
> problem.

Actually, I was wrong.  If you tap on the first button (the one that opens the dialog), then Firefox freezes whether you click or tap on the second button (the one in the dialog).

Comment 3

[Matt Brubeck (:mbrubeck)]

When we are processing the NS_MOUSE_BUTTON_UP event generated by HandleTap, the DispatchEvent call on this line never returns, because it's waiting on the modal dialog to close:
http://hg.mozilla.org/mozilla-central/file/73c93e4a4d30/widget/windows/winrt/MetroInput.cpp#l1034

That means we never reach the code a few lines down where we call InputEventsDispatched which enables native events again.

Comment 4

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This makes MetroAppShell honor the same timeout it passes to nsBaseAppShell, rather than blocking native events indefinitely.

Taking this bug for iteration 21, work estimate p=2.

Comment 5

[Marco Mucci [:MarcoM]]

Added to IT#21.

Comment 6

[Jim Mathies [:jimm]]

Bummer. I tested with the astronomy app which reproduced bug 840855 reliably, but didn't test with other dialogs. 

We should work up a test using the synth apis. I can get to that after the break of no one else gets to it.

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 8350470 [details] [diff] [review]
patch

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

Passing to jimm since he's a better reviewer for this

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8350470 [details] [diff] [review]
patch

Confirmed this fixes the regression. The only suggestion/nit I'd throw out there would be convert this over to Timestamp/TimeDuration so we're not tangled up in PR gunk.

Also a test would be great.

Comment 9

[Matt Brubeck (:mbrubeck)]

Changed to use TimeStamp:
https://hg.mozilla.org/mozilla-central/rev/1a2895701eef

I'll post a test next as a separate patch.

Comment 10

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8350470 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 944215

User impact if declined: Metro Firefox freezes completely when content or chrome opens a modal dialog in response to a single-tap.

Testing completed (on m-c, etc.): Landed on m-c on 12/20.  Tested by multiple developers before landing; will be tested by QA in 12/21 nightly.

Risk to taking this patch (and alternatives if risky): Very low-risk Metro-only patch.  Main risk is there is a chance this could regress bug 944215 in specific situations (very slow hardware or slow scripts), but that is better than the alternative which is to back out bug 944215 completely.

String or IDL/UUID changes made by this patch: None.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: WIP test

This is an attempt to write an automated test for this bug, but the test seems to succeed even without the fix.  Maybe the injected native touch events are not affected by the block, or maybe there's something wrong with my test logic.  Can you spot any problems?

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea0884724339

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8350755 [details] [diff] [review]
WIP test

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

::: browser/metro/base/tests/mochitest/res/prompt.html
@@ +4,5 @@
> +    <title>prompt test</title>
> +    <meta charset="UTF-8">
> +  </head>
> +  <body>
> +    <a id="link" onclick="alert('test')">Click here</a>

Hmm, not sure. Maybe the onclick comes in outside of the tapped processing? There's a define at the top of MetroInput that turns on a bunch of logging that can be used in seeing the events as they come in.

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Went through the following issue for iteration #21 without any issues. Used the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-13-00-40-02-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-01-13-03-02-03-mozilla-central/

I reproduced the original issue mentioned in comment #0 and comment #1 using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-19-03-02-02-mozilla-central/

- Went through all the modal windows under "alert()" using the website from comment #0
- Went through all the modal windows under "confirm()" using the website from comment #0
- Went through all the modal windows under "prompt()" using the website from comment #0
- Ensured that taping on either "OK" or "Cancel" dismisses the modal window without any issues
- Ensured that taping on the text field in the modal window slides in the OSK without any issues
- Ensured that the OSK is being dismissed when taping on either "OK" or "Cancel"
- Ensured that you can create new tabs using the overlay button when the modal window is visible
- Ensured that you can create new tabs using the "+" under the "Navigation App Bar" when the modal window is visible
- Ensured that you can go back using the overlay button when the modal window is visible
- Ensured that you can slide in the Navigation App Bar when the modal window is visible
- Ensured that you can close the tab through the "Navigation App Bar" while the modal window is visible
- Ensured that you can slide in the "Windows Charm" while the modal window is visible
- Ensured that you can slide applications (Desktop) from the left side of the screen while the modal window is visible
- Went through all of the above test cases in several filled view modes (1/2, 1/3, 2/3 etc..)