725458 - After window with touchmove event preventDefault(), can't long tap to open context menu anymore

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Steps to reproduce:
- Tap on the "open window with touchmove preventdefault and tap on the window.close button" button.
- In the opened window, tap on the "Close this window" button
- Long tap on the link or the text input on the testcase

Expected result:
- Context menu of link/text input pops up 

Actual result:
- No context menu comes up

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Is this not working like the spec says it should?

Comment 2

[Wesley Johnston (:wesj)]

Hmm.. this is a weird case. Yes. I'll look at this this afternoon.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2

This will fix this, but I think just proves there's some other problem here. Feel free to hold off on the review if you want Matt.

We currently don't set a timer to enable panning (or long tap in this case) until touchup or the first touchmove. That's because i thought a user might tap for a long time and then start moving their finger and only at that point would we really be able to find out if we should prevent those actions.

For things like longtap, that's just plain horrible. Best case scenario for long tap, the user waits forever, gives up and lifts their finger, and then we set a timer which will fire the longtap a few hundred milliseconds later.

I think something similar was happening here, although even on touchend we still weren't firing longtap (although we do if you touch for a bit and then move your finger). And we shouldn't even be waiting here because the page has no touch listeners and we don't want to wait then.

So I'd like to look into it a bit more, but this fixes the timer bug. It will break things on pages that want to prevent panning on touchmove if the user taps, waits for awhile, and then expects panning to not happen.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

Not sure if you can review this or not felipe, but thought I'd try. We're not always notifying java when we have touch listeners correctly. I saw this on Google Maps at least. For some reason, we seem to think that maps is not focused and don't fire the notification. This just moves us out of this check, which I'm told is only there for Win7 anyway. We have some alternatives like #ifdef'ing stuff if we'd rather. I also am not exactly sure why Google Maps seems to think its not focused.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 600269 [details] [diff] [review]
Patch 2/2

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

So you're only interested in changing the call position of the notifyObservers call, and the {Un}RegisterTouchWindow call will keep the same behavior, right? If so, that's fine. The Windows widget is only interested in the {Un}RegiserTouchWindow part. Maybe in the future I could change it to use the observers too but then I'll think about what that will change.

Note that the focus manager calls UpdateTouchState, so you won't get an observer notification from that call (http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1736), but if it's not tied to focus anymore that won't mean anything.

Technically I'm not a reviewer in this module, so maybe smaug could rubberstamp it.

(nit: there is some whitespace being added to one line)

Comment 6

[:Felipe Gomes (needinfo for replies!)]

And FTR, the reason we can't change the behavior of {Un}RegisterTouchWindow (to not be based on focus) is that otherwise a tab with a touch listener would disable gestures for every tab on that window

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 600269 [details] [diff] [review]
Patch 2/2

smaug should probably look too.

We're (currently) caching the state in Java as well since we don't have access to the window there, so I think we're good not getting notifications on focus.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 600267 [details] [diff] [review]
Patch 1/2

I think I can actually do a lot more clean up here if we do this. Putting off the review for a bit.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 600269 [details] [diff] [review]
Patch 2/2

(just re-setting my r+ again that was lost)

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

So I like how this works and its quite a bit simpler than what we had before. The only times I see failures, they're entirely because Gecko isn't responding within time. We still need to detect when that happens and at least fire bounce() so that we revert any overscroll that accidently happened.

There's still an issue with knowing whether a tab has touchListeners. I think I messed up that patch when it landed the first time, but I tried to fix it here by registering for OnTabChanged in GeckoApp. Turns out, that fires more than once per new tab created (Once when its made, Once at document stop, etc.) and only once when a tab is closed. In that case, it passes us the tab that was closed, so we don't get to update with the selected tab. Grrr... I'll try to get that patch on Monday. I wonder if we need to pass a reason along with the onTabChanged messages... or we can just fire this update from every place in GeckoApp where a new tab is selected/locationchanged/etc. like I originally tried.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 600269 [details] [diff] [review]
Patch 2/2

>-void nsGlobalWindow::UpdateTouchState()
>-{
>-  FORWARD_TO_INNER_VOID(UpdateTouchState, ());
>-
>-  nsCOMPtr<nsIWidget> mainWidget = GetMainWidget();
>-  if (!mainWidget)
>-    return;
> 
>   if (mMayHaveTouchEventListener) {
>-    mainWidget->RegisterTouchWindow();
>-
>     nsCOMPtr<nsIObserverService> observerService =
>       do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>-
>+  
Some extra spaces here.

>+void nsGlobalWindow::UpdateTouchState()
>+{
>+  FORWARD_TO_INNER_VOID(UpdateTouchState, ());
>+
>+  nsCOMPtr<nsIWidget> mainWidget = GetMainWidget();
>+  if (!mainWidget)
>+    return;
if (expr) {
  stmt;
}

Comment 12

[Wesley Johnston (:wesj)]

I split off the other (probably the real) part of this bug to bug 725458 and threw the review at Margaret so that I could have someone else get a bit familiar with this code. I'll request feedback from you too mbrubeck.

Comment 13

[Wesley Johnston (:wesj)]

platform piece:
https://hg.mozilla.org/integration/mozilla-inbound/rev/080fde330ce0

Comment 14

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/080fde330ce0

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/107beeff501d

Comment 16

[Wesley Johnston (:wesj)]

whoops. bad merge on my part meant a backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f20ece686f

and back in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7309f41d24f9

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7309f41d24f9

Comment 18

[Catalin Suciu [:csuciu]]

Verified fixed on:
Nightly 14.0a1 (2012-04-12) 20120412030726
Samsung Galaxy SII (Android 2.3.4)