Closed Bug 725458 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 13
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: martijn.martijn, Assigned: wesj)

References

Details

(Keywords: testcase)

Attachments

(3 files, 1 obsolete file)

Attached file 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
tracking-fennec: --- → ?
Wes - Is this not working like the spec says it should?
Assignee: nobody → wjohnston
Hmm.. this is a weird case. Yes. I'll look at this this afternoon.
Attached patch Patch 1/2 (obsolete) — Splinter Review
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.
Attachment #600267 - Flags: review?(mbrubeck)
Attached patch Patch 2/2Splinter Review
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.
Attachment #600269 - Flags: review?(felipc)
blocking-fennec1.0: --- → beta+
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)
Attachment #600269 - Flags: review?(felipc) → review+
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 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.
Attachment #600269 - Flags: review+ → review?(bugs)
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.
Attachment #600267 - Flags: review?(mbrubeck)
Status: NEW → ASSIGNED
Comment on attachment 600269 [details] [diff] [review]
Patch 2/2

(just re-setting my r+ again that was lost)
Attachment #600269 - Flags: review+
Attached patch WIP PatchSplinter Review
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.
Attachment #600267 - Attachment is obsolete: true
Attachment #600589 - Flags: review?(mbrubeck)
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;
}
Attachment #600269 - Flags: review?(bugs) → review+
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.
platform piece:
https://hg.mozilla.org/integration/mozilla-inbound/rev/080fde330ce0
Whiteboard: [mtd][inbound]
Attachment #600589 - Flags: review?(mbrubeck) → review+
Whiteboard: [mtd]
https://hg.mozilla.org/mozilla-central/rev/7309f41d24f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 13
Blocks: 728946
Verified fixed on:
Nightly 14.0a1 (2012-04-12) 20120412030726
Samsung Galaxy SII (Android 2.3.4)
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.