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

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: rsilveira, Assigned: mbrubeck)

Tracking

({regression})

unspecified
Firefox 29
x86_64
Windows 8.1
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox27 unaffected, firefox28 verified, firefox29 verified)

Details

(Whiteboard: [beta28] p=2)

Attachments

(2 attachments)

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
Whiteboard: [triage]
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.
Blocks: 944215
Depends on: 840855
Keywords: regression
Summary: Tab modal prompts intermittently fail to accept touch input → Tab modal prompts fail to accept touch input if opened during a touch input event
Whiteboard: [triage] → [beta28]
(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).
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.
Posted patch patchSplinter Review
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.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #8350470 - Flags: review?(netzen)
Flags: needinfo?(mmucci)
Added to IT#21.
Blocks: metrov1it21
No longer blocks: metrov1backlog
Flags: needinfo?(mmucci)
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] → [beta28] p=2
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 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
Attachment #8350470 - Flags: review?(netzen) → review?(jmathies)
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.
Attachment #8350470 - Flags: review?(jmathies) → review+
Changed to use TimeStamp:
https://hg.mozilla.org/mozilla-central/rev/1a2895701eef

I'll post a test next as a separate patch.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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.
Attachment #8350470 - Flags: approval-mozilla-aurora?
Attachment #8350470 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Posted patch WIP testSplinter Review
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?
Attachment #8350755 - Flags: feedback?(jmathies)
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.
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..)
Attachment #8350755 - Flags: feedback?(jmathies)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.