Closed
Bug 823619
Opened 12 years ago
Closed 12 years ago
Make mouse events synthesized from touch events comply with spec (in apps)
Categories
(Core :: DOM: Events, defect, P1)
Tracking
()
RESOLVED
FIXED
B2G C4 (2jan on)
People
(Reporter: cjones, Assigned: cjones)
References
Details
(Whiteboard: [LOE:S])
Attachments
(4 files, 23 obsolete files)
+++ This bug was initially created as a clone of Bug #774458 +++ Updating this to track reality. This blocks numerous blockers. We need to make a decision based on below statim. Leaving blocking+ for decision. I think we should very seriously consider this. To do that we need to estimate the work to convert gaia. Left on the table if we decide not to do this is - breaking touch event standard and being stuck with that "forever". Fixing that is currently blocking+. - compatibility fallout from that - different behavior in "browser" and "app" content, visible to authors - bug 814252 and several blocking+ followups that require it - bug 819593, blocker, and bug 819595, soft-blocker currently So my initial idea was to make a shim that implements the necessary part of the pointer events proposal (not much). This is a straightforward change and is somewhat forwards compatible. My concern is that the gesture detector is overkill for the users who just need sane down/move*/up events for the primary touch point, and it moves more of platform "look and feel" into gaia. I know we've been losing that battle bigtime for v1, but I don't want to give more ground ;). We have 54 references to mousemove, which probably means <27 real users. That's a lot. The remaining data I'm going to gather is how painful converting a few to pointer events would be. Let's say homescreen and lockscreen. djf, if you want to do the same with gesture_detector, please do. That's another option we can consider.
Assignee | ||
Comment 1•12 years ago
|
||
https://bugzilla.mozilla.org/attachment.cgi?id=694092 is a WIP of what we need here.
Comment 2•12 years ago
|
||
This is a complete work in progress, but I'll go ahead and try to test it.
Attachment #694494 -
Flags: feedback?(jones.chris.g)
Assignee | ||
Comment 3•12 years ago
|
||
FTR, I think we all agree at this point that directly converting all the gaia apps that rely on the buggy current gecko behavior to spec compliance is infeasible for v1.
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 694494 [details]
wip synthetic mouse event shim library
This is pretty much what I had, except using pointer. This approach will result in a smaller gaia patch so let's do it.
Attachment #694494 -
Flags: feedback?(jones.chris.g) → feedback+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #1) > https://bugzilla.mozilla.org/attachment.cgi?id=694092 is a WIP of what we > need here. Correction: https://bug766066.bugzilla.mozilla.org/attachment.cgi?id=693969
Comment 8•12 years ago
|
||
For reference, here is the Pointer Events spec that Microsoft submitted to the W3C: http://www.w3.org/Submission/pointer-events/
Comment 9•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #2) > Created attachment 694494 [details] > wip synthetic mouse event shim library mbrubeck suggested we ignore multitouch events (event.touches.length > 1). It avoids some shim complexity and most people have only one mouse. :) He is taking this approach for the Firefox Metro port: https://hg.mozilla.org/projects/elm/rev/b36e74e1a195#l10.240 He also wondered whether calling preventDefault() on touchstart or touchmove might break panning. Just something to watch out for. re mouseover/mouseenter/mouseout/mouseleave: grepping through Gaia's apps, external-apps, and shared js shows: 1 mouseover (pdfjs) 0 mouseenters 2 mouseouts (pdfjs and sms) 3 mouseleaves (contacts, dialer, and value_selector)
Comment 10•12 years ago
|
||
Here is a simple test I used compare browsers' touch and mouse event behavior.
Comment 11•12 years ago
|
||
I'm attaching an updated version of my synthetic events shim. This one actually works. Here's what I've learned so far. 1) cpeterson's TabChild mouse events patch (attached to this bug) does not apply cleanly to today's m-c, but it is trivial to fix the rejected hunk. 2) Touch events don't work correctly in today's m-c tree, but with these patches applied, they do work: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7f3548011c and https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbe2966ec8a 3) With those three patches applied, I cannot pan the homescreen anymore. This is expected 4) If I add my synthetic_mouse_events.js shim to apps/homescreeen/index.html, then I can pan the homescreen 5) contextevents are not working in my build of gecko. I don't know if that is the fault of cpeterson's patch or the other two patches I applied, or if they are broken for some other reason. But it means that I can't test the homescreen code that drags app icons around the screen. 6) With these gecko patches applied, I can't set an alarm in the Clock app: I can't swipe up and down on the numbers to select the time I want. This is expected. 7) If I add my synthetic_mouse_events.js shim to apps/clock/index.html, then the numbers start moving again. But they move much, much too slowly. The reason is that the clock code is computing the velocity of my swipe by comparing the timestamps of the mousedown and mouseend events. Real gecko events have timestamps in milliseconds. But synthetic ones (created with document.createEvent()) use microseconds. So the time interval comes out 1000 times too big. I work around this in gesture detector.js. The clock app will need to include a similar workaround to determine the timestamp units so that it works both on the desktop (with native mouse events) and on the phone (with synthetic mouse events).
Attachment #694494 -
Attachment is obsolete: true
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #11) > Created attachment 694562 [details] > synthetic mouse event shim, wip version 2 > > 5) contextevents are not working in my build of gecko. I don't know if that > is the fault of cpeterson's patch or the other two patches I applied, or if > they are broken for some other reason. contextmenu is synthesized based on waiting for a delay between mousedown and mouseup. Hmmmmmmm ... that's a bad one. I think we'd need to fix cpeterson's patch to do that. cpeterson, is that feasible? Not looking good here ... :(
Comment 13•12 years ago
|
||
More experimental results: 8) The FM Radio app uses the same basic code as the Clock does for the radio tuner, so it also needs the divide the time interval by 1000 fix that the clock needs 9) The Music app listens for mousemove events for seeking within the song. But it accepts any mousemove not just mousemoves followed by clicks. So on desktop it responds to the mouse moving over it, and on the phone, you have to tap. Taps work both with and without these patches. When I add my library nothing seems to change. I'm not sure whether anything needs to be fixed for this app or not. 10) The video app doesn't allow you to drag the play head to seek through the video with these patches applied. Adding synthetic_mouse_events.js seems to fix it. 11) The camera and gallery can also play videos, but the UI they use is based on gesture_detector.js and it seems to work okay without changes.
Comment 14•12 years ago
|
||
David, can emitEvent() fudge the MouseEvent time like this? var synthetic = document.createEvent('MouseEvents'); synthetic.timeStamp /= 1000;
Comment 15•12 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #14) > David, can emitEvent() fudge the MouseEvent time like this? > > var synthetic = document.createEvent('MouseEvents'); > synthetic.timeStamp /= 1000; I see the answer is no. :\
Comment 16•12 years ago
|
||
More: 12) With cpeterson's patch, there is no way to answer an incoming call from the lock screen, because we can't swipe up. But with my shim it works.
Comment 17•12 years ago
|
||
(In reply to Chris Peterson (:cpeterson) from comment #15) > (In reply to Chris Peterson (:cpeterson) from comment #14) > > David, can emitEvent() fudge the MouseEvent time like this? > > > > var synthetic = document.createEvent('MouseEvents'); > > synthetic.timeStamp /= 1000; > > I see the answer is no. :\ The way I worked around this in gesture detector is to assume that the timestamp is actually based on the current time. Then if e.timestamp > 2*Date.now() divide it by 1000. It seems to work, though I should probably double-check my assumption that these timestamps are about 1000*Date.now()
Assignee | ||
Comment 18•12 years ago
|
||
Comment on attachment 694502 [details] [diff] [review] WIP-TabChild-mouse-events.patch >diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp >+ // If the user single-taps (touchstart followed by touchend), then send >+ // synthetic mousemove, mousedown, and mouseup events. mousedown will >+ // trigger a focus event. mouseup will trigger a click event. >+ if (aEvent.message == NS_TOUCH_END && mLastTouchEvent == NS_TOUCH_START) { >+ nsIntPoint refPoint(0, 0); >+ if (aEvent.touches.Length()) { >+ refPoint = aEvent.touches[0]->mRefPoint; >+ } >+ >+ DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, aEvent.time, refPoint); >+ DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_DOWN, aEvent.time, refPoint); >+ DispatchSynthesizedMouseEvent(NS_MOUSE_BUTTON_UP, aEvent.time, refPoint); If the duration that elapses between the touchstart and touchend is greater than ui.click_hold_context_menus.delay, then I /think/ we can just also dispatch a contextmenu event here.
Comment 19•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #18) > If the duration that elapses between the touchstart and touchend is greater > than ui.click_hold_context_menus.delay, then I /think/ we can just also > dispatch a contextmenu event here. Building this into EventStateManager behind a pref seems like a valid thing to do too. Doing it with mouseevents is already there behind a similar pref.
Assignee | ||
Comment 20•12 years ago
|
||
I really wish we had done that a long time ago, but I'm afraid that would require teaching a lot of ESM about touch events.
Comment 21•12 years ago
|
||
This new attachment is a link to a github pull request that includes the shared/js/synthetic_mouse_events.js shim and changes to these apps to make them use it: homescreen video clock fm radio dialer (incoming calls) I still need to update the shim to do mouseover/out and mouseenter/leave events. There are still issues with scrolling not working and contextmenu events not working. And the gesture detector-based Gallery app is getting double clicks when I just tap once and I haven't tried to figure that out yet. It would be interesting to know if those bugs are side effects of cpeterson's patch or if they are distinct bugs in m-c.
Attachment #694562 -
Attachment is obsolete: true
Comment 22•12 years ago
|
||
If I apply the github pull request in the attachment to a nightly b2g18 build from pvtbuilds.mozilla.org I find that the apps modified to work with the events shim still seem to work. But, and this is a big bug: the shim breaks context menus and scrolling in the apps that use it. In this b2g18 build, contextmenu events work in apps (like Music) that do not use the shim and scrolling works in apps (like Gallery) that do not use the shim. But the contextmenu event does not work in Homescreen (which uses the shim) and scrolling does not work in Video (which uses the shim). So the fact that the shim calls preventDefault() on all of the touch events it processes means that the apps using it don't get duplicate mouse events. But it also means that other important side effects of those events get prevented. So: it is not safe to land synthetic_mouse_events.js in gaia without landing the gecko change at the same time.
Comment 24•12 years ago
|
||
I've applied the patch from bug 814252 and it indeed fixes scrolling for apps that do not use my shim. But it looks like apps that do use my shim have broken scrolling. So calling preventDefault on the touch events may not be the right thing to do. Does the code in 814252 handle the events after content handles them and does it honor defaultPrevented? My goal was to prevent gecko from generating mouse events for taps, so I've called preventDefault() on everything. But maybe I should be not prevent any events, and only emit mouse events when there is a touchmove? If no move, then just let gecko handle it the mouse events. Otherwise, if there is a touch move, then generate all the events myself. Does that sound like it could work?
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #24) > I've applied the patch from bug 814252 and it indeed fixes scrolling for > apps that do not use my shim. But it looks like apps that do use my shim > have broken scrolling. To be clear, is this with cpeterson's patch applied? > Does the code in 814252 handle the events after content handles them and > does it honor defaultPrevented? It honors preventDefault(). It should run after content handlers, but I'm not 100% sure. > My goal was to prevent gecko from generating mouse events for taps, so I've > called preventDefault() on everything. But maybe I should be not prevent > any events, and only emit mouse events when there is a touchmove? If no > move, then just let gecko handle it the mouse events. Otherwise, if there is > a touch move, then generate all the events myself. Does that sound like it > could work? After cpeterson's patch, there shouldn't be any mouse events (except for tap).
Comment 26•12 years ago
|
||
1. Here is a WIP patch that synthesizes an NS_CONTEXTMENU event from TabChild.cpp. I compare the elapsed touch time and the "ui.click_hold_context_menus.delay" pref value. 2. WARNING! Unfortunately, I have not actually been able to test the context menu with my patch! When I press and hold on the homescreen, the homescreen app crashes and restarts. I suspect the problem is a regression from the bug 814252 patch I applied because the homescreen still crashes if I comment out my NS_CONTEXTMENU changes. 3. btw, "ui.click_hold_context_menus.delay" pref's default value is 1000 ms, which feels like a really long time. Android's default long press delay is 500 ms: https://github.com/android/platform_frameworks_base/blob/a454767b09ecb7d25d00beae0e5a1fdd48605c63/core/java/android/view/ViewConfiguration.java#L80
Attachment #694502 -
Attachment is obsolete: true
Attachment #694669 -
Flags: feedback?(jones.chris.g)
Attachment #694669 -
Flags: feedback?(dflanagan)
Comment 27•12 years ago
|
||
Comment on attachment 694669 [details] [diff] [review] WIP-TabChild-mouse-events-v3.patch Review of attachment 694669 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1380,5 @@ > + > + DispatchSynthesizedMouseEvent(NS_MOUSE_MOVE, time, refPoint); > + > + if (aEvent.time > mLastTouchStartTime + mContextMenuDelay) { > + DispatchSynthesizedMouseEvent(NS_CONTEXTMENU, time, refPoint); I don't think you can wait for the touchend event like this. Contextmenu has to fire after 1000ms (or whatever the interval is) even if the user's finger is still down, right? It seems like it would be a lot easier to do in js than in c++, but I don't understand the layers of gecko and chrome well enough to know why we can't just modify the code that is currently generating the contextmenu event.
Attachment #694669 -
Flags: feedback?(dflanagan) → feedback-
Comment 28•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25) > (In reply to David Flanagan [:djf] from comment #24) > > I've applied the patch from bug 814252 and it indeed fixes scrolling for > > apps that do not use my shim. But it looks like apps that do use my shim > > have broken scrolling. > > To be clear, is this with cpeterson's patch applied? > Yes. > > Does the code in 814252 handle the events after content handles them and > > does it honor defaultPrevented? > > It honors preventDefault(). It should run after content handlers, but I'm > not 100% sure. Okay, so calling preventDefault() is the wrong thing to do. I'll take that out. > > My goal was to prevent gecko from generating mouse events for taps, so I've > > called preventDefault() on everything. But maybe I should be not prevent > > any events, and only emit mouse events when there is a touchmove? If no > > move, then just let gecko handle it the mouse events. Otherwise, if there is > > a touch move, then generate all the events myself. Does that sound like it > > could work? > > After cpeterson's patch, there shouldn't be any mouse events (except for > tap). Right, but my shim duplicates those events, so I was calling preventDefault hoping that I could make cpeterson's mouse events go away. I realize now that I was completely misunderstanding the layers of event code in gecko. There's no way I can cancel those events so I need to not generate them. It makes your suggestion that we use pointer events seem like an appealing approach. But I'll try again with this shim and see if I can modify it to just supply the missing mouse events
Assignee | ||
Comment 29•12 years ago
|
||
Ugh. cpeterson's code is correct. I think the gecko bowels are wrong here. preventDefault()ing the touchend should work though.
Comment 30•12 years ago
|
||
I've simplified my synthetic_mouse_events.js shim and pushed a new version of the PR to github. This one does not try to prevent default anything, and only generates mouse events when there is a touchmove. So touchstart/touchend are handled by gecko. If there is a touchmove in between, then the shim kicks in and supplies the mouse events. With the TabChild patch and the scrolling patch applied, this shim makes the homescreen, clock, fm radio and video apps work as before, and it also allows scrolling to work.
Assignee | ||
Comment 31•12 years ago
|
||
Oh, actually we should be checking |nsIPresShell::gPreventMouseEvents| and "default prevented". So the gecko bowels should be ok.
Comment 32•12 years ago
|
||
The latest version of my shim supports mouseover/mouseout events. No apps use mouseover, but the sms app uses mouse out. I've just tested, and it works as it should with the shim. (Though the use of mouseout is kind of bogus and the code should probably be changed.)
Comment 33•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #31) > Oh, actually we should be checking |nsIPresShell::gPreventMouseEvents| and > "default prevented". So the gecko bowels should be ok. Are you saying that if I call preventDefault() on a touchstart event in content, that should get back to the TabChild.cpp code and prevent the generation of the mouse events? If so, I wonder if my shim could call preventDefault() on touchstart to prevent the mouse events there but could still have scrolling work even with that initial touchstart event prevented... The SMS app tries to use a mousedown event to highlight a button, to show that it is active. It would work in the desktop model, but with the TabChild patch, the mousedown doens't get generated until after the touchend, so the highlight never shows up. If my shim could cancel the tab child stuff without breaking scrolling, then the shim could send a mousedown event right after the touchstart event.
Comment 34•12 years ago
|
||
Okay, if I call preventDefault() on the touchstart() event then it does seem to actually stop Gecko from sending mouse events. (It makes it very, very hard to click on icons from the homescreen, but once in 20 times I can actually launch an app; I'm not sure what is happening, but it does seem that preventDefault does the right thing here.) Unfortunately, calling preventDefault() on that initial touchstart also prevents scrolling, so I can't use that approach to always send a mousedown right after the touchstart.
Assignee | ||
Comment 35•12 years ago
|
||
We got an alternate fix for bug 814252. That changes the calculus here significantly, since its tree of blockers are no longer blocked by this. cpeterson's patch has some issues I'm going to take about an hour to fix. If closing this out looks like more than a person-day of work (including gaia) with neglible risk, we might need to cut our losses for bug 819595.
Assignee | ||
Comment 36•12 years ago
|
||
Assignee | ||
Comment 37•12 years ago
|
||
Comment 38•12 years ago
|
||
https://github.com/mozilla-b2g/gaia/pull/7120 now includes two versions of my shim. The first version is in shared/js/synthetic_mouse_events.js. It only synthesizes events when it sees a mousemove, and lets Gecko send synthetic events for tap gestures. It never calls preventDefault() on anything. This first version has been tested with cpeterson's first version of the ChildTab patch. The second version is shared/js/synthetic_mouse_events2.js. It calls preventDefault() on touchend events that do not have a touchmove before them, and in doing so tries to get Gecko not to send synthetic events. Since it does this, it synthesizes its own mouse events for all touch events. This means that it can send a mousedown as soon as it sees the touchstart. It doesn't need to wait for the touchend to know whether it is a tap or not. In this case, the shim has to decide whether or not to send a click event, since it won't be automatically generated from the synthetic mousedown/mouseup pair. This second version has not been tested, but is intended for use with cjones's modification of the ChildTab patch. The github pull request also includes modifications of some gaia apps to make them use the shim. Neither version of the shim does mouseenter/mouseleave events yet, but I can add that pretty easily if we need it.
Assignee | ||
Comment 39•12 years ago
|
||
We're possibly looking at a backout of bug 766066, so the calculus changes again. I was testing this while those bugs were being filed. Environment - build from 42213ca59024 (that is, without bug 766066) - apply 8cbe2966ec8a - apply cd7f3548011c The patches here fix all the bugs that this blocks. But it's not all roses. There are regressions with both the v1 and v2 synthetic_mouse_event files (oddly). 1. homescreen fast-pan. Pan between pages as fast as you can, nothing happens. Major user-perceived perf issue. 2. dialer key long-press. No tone, no visual indicator. Certification blocker. 3. as expected, tap detection is bad with the heuristics in synthetic_mouse_event 5. bug 821583 6. bug 822590 7. bug 822592 My guess is that (1) is a timestamp issue. That's scary. The rest look like broken gaia code that should be easily fixable. I think we can spare a few (but no more!) more person-hours on quick analysis of above. If (1) can't be fixed, we abort, for sure. I'll try to look before I collapse.
Comment 40•12 years ago
|
||
calling prevent default on touchend should not do anything as per the touchevents spec.
Comment 41•12 years ago
|
||
Chris, Looks like the homescreen panning issues is apps/homesscreen/js/grid/js:207 Add a divide by 1000 and I bet it works. My fix in the other apps that have this issue is to test the timestamp to see if it is much bigger than Date.now(). If so, divide by 1000. cpeterson suggests a utility function bundled with the shim for getting timestamps from events. Now that we need that in at least 3 places in the code, I think it is a good idea.
Comment 42•12 years ago
|
||
Note, if you use event.timestamp information for anything, .timestamp will change at some point. Gecko has had buggy .timestamp since the beginning and so far it hasn't been too critical to fix (in other words, there has always been something more important).
Comment 43•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #39) > We're possibly looking at a backout of bug 766066, so the calculus changes > again. > ... > I think we can spare a few (but no more!) more person-hours on quick > analysis of above. If (1) can't be fixed, we abort, for sure. I'll try to > look before I collapse. What is the current thinking? Should I back out bug 766066?
Assignee | ||
Comment 44•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #40) > calling prevent default on touchend should not do anything as per the > touchevents spec. Do you know what the motivation for that is? If it's important, then I don't believe we'll be able to make a working mouseevent shim.
Assignee | ||
Comment 45•12 years ago
|
||
Comment 46•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #44) > (In reply to Wesley Johnston (:wesj) from comment #40) > > calling prevent default on touchend should not do anything as per the > > touchevents spec. > > Do you know what the motivation for that is? If it's important, then I > don't believe we'll be able to make a working mouseevent shim. The primary goal of the touch events v1 spec was just to document the behavior of the iPhone's browser. This is one of those "quirks" of it. TBH, I'd be fine with removing this restriction for us, although its non-trivial and might lead to larger delays between taps turning into clicks (since we'd have to wait for content to make that decision). The iPhone only allows preventDefault on touchstart or the first touchmove associated with a point, presumably to keep pages from blocking async panning.
Assignee | ||
Comment 47•12 years ago
|
||
To set up $ hg up -r 42213ca59024 $ hg qimport -n touch-hell https://bugzilla.mozilla.org/attachment.cgi?id=694933 then rebuild.
Comment 48•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #44) > (In reply to Wesley Johnston (:wesj) from comment #40) > > calling prevent default on touchend should not do anything as per the > > touchevents spec. > > Do you know what the motivation for that is? If it's important, then I > don't believe we'll be able to make a working mouseevent shim. I don't know the motivation. I wouldn't want to deviate from spec on the touchdown or touchmove events. But it seems safer to deviate for touchend. If we decide to abide by the spec we can still create a shim that works for most apps. But mousedown events will be delayed until after a touchmove or touchend. This means that longpress in the dialer won't work and we'll have to manually convert that app to use touch events. Shouldn't be too hard. The shim will still work for swipes, because they get move events right away.
Assignee | ||
Comment 49•12 years ago
|
||
Remaining questions 1. Why is the homescreen fast-panning not working 2. Why are dialer long-presses not working 3. Why is the gallery edit-mode slider not working
Comment 50•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #47) > To set up > > $ hg up -r 42213ca59024 > $ hg qimport -n touch-hell > https://bugzilla.mozilla.org/attachment.cgi?id=694933 > > then rebuild. Also, I needed to add $ hg qpush
Assignee | ||
Comment 51•12 years ago
|
||
One more difficult test case (bug 821583) STR: ==================== 0. 2012/12/14 mozilla-central build. 1. Add a new alarm (the time picker is still working). 2. Reset that alarm. 3. When entering the time picker page that contains a "Delete" button below. Actual: ==================== When scrolling the timer picker, the whole page will be scrolled, thus failing to set the time. Expected: ==================== The whole page shouldn't be scrolled.
Assignee | ||
Comment 52•12 years ago
|
||
qparent, dammit.
Attachment #694933 -
Attachment is obsolete: true
Comment 53•12 years ago
|
||
I can't reproduce the alarm bug in comment 51. Last time I used the clock, I saw what you meant about the alarm button off the bottom of the screen. But now everything fits on the screen, so there is nothing to scroll. With the latest version of my shim I do see another bug however: each alarm is getting set twice. So there must be a duplicate event. (In reply to Chris Jones [:cjones] [:warhammer] from comment #49) > Remaining questions > > 1. Why is the homescreen fast-panning not working Calling elementFromPoint() on each touchmove event is making the frame rate substantially slower. The shim could make that optional, but it still might need to be ported to use touch events natively for maximum speed. > 2. Why are dialer long-presses not working Long presses are working for me. But, as with the clock above, dialing is very difficult because I'm getting duplicate events. More shim debugging is needed. > 3. Why is the gallery edit-mode slider not working The exposure slider and the crop controls all use mouse events (for desktop compatibility). But Gallery also uses gesture_detector, and including the shim along with gesture detector seems to cause problems. So that code might need to be ported by hand. Or I need to make the shim and the gesture detector play nicely together.
Comment 54•12 years ago
|
||
Argh. Its stopImmediatePropagation(), not preventImmediatePropagation()! That would explain all the double clicks
Comment 55•12 years ago
|
||
I've fixed the double alarm and double dial problems by using stopImmediatePropagation. Dialer is one of the apps that depends on mouseleave events and the shim doesn't send those yet. So it is back to the state where if you start a long press and then slide your finger to another key and release, the tone doesn't stop sounding. Bad bug, but we know how to fix it.
Comment 56•12 years ago
|
||
Once again, I've updated my pull request at https://github.com/mozilla-b2g/gaia/pull/7120 The shim has a new name: shared/js/mouse_event_shim.js All of the modified apps should now be using that new version of this shim. I've left the old versions around for reference in case they work better. This new version applies cpeterson's trick of capturing and discarding the real mouse events from gecko. This new version also defines a global MouseEventShim object. Set MouseEventShim.trackMouseMoves to false if you don't want the overhead of elementFromPoint(). You should be able to flip this switch between any two events but that is not tested. Use MouseEventShim.getEventTimestamp() to portably get a timestamp value in milliseconds. The pull request uses this utility in homescreen, clock and fm radio Other updates: This shim and gesture detector do seem to work okay together afterall. Adding the shim to gallery fixes the slider and crop handles without seeming to break anything. This is the best I've seen gallery working. Long presses are working in dialer, though it still needs mouseleave support in the shim, or the dialer needs to be converted to use mouseout instead I can't reproduce the alarm clock bug. The clock app might need to call preventDefault on the mousedown event it handles, and that should now get passed on. Moving icons around on the homescreen isn't working right. This may be a scrolling while dragging issue... Maybe a preventDefault() call would fix that. The shim does not propagate preventDefault calls on mousemove events to the touchmove. I think maybe it needs to do this. And with that, I'm out of time. Back at work full time on the 27th.
Assignee | ||
Comment 57•12 years ago
|
||
We're already in for about 2 person-days investment here, and in exchange we're looking at 5 (maybe more) gaia blockers that we probably just can't fix otherwise. Engineering time on hacks has been proven to fail twice in the current setup, so probably not worth investing. Spec compliance is also on the table. Thankfully, sunk costs in backed-out hackery have been incorporated here. djf's mouse events shim is as minimally invasive as could be hoped for common cases. That experiment was a success. What remains to be demonstrated is that important perf targets can be fixed. I'll invest my 0-person-days weekend time in that tomorrow. If we prove perf, I think we should do this.
Comment 58•12 years ago
|
||
add C3 milestone as this bug blocks other C3 bugs.
Target Milestone: --- → B2G C3 (12dec-1jan)
Comment 59•12 years ago
|
||
I just tested mouse_event_shim.js in the UI Tests keyboard panel and in the feedback app, because I was worried that stopping the propagation of the mouse events gecko generates would stop the focus event from being delivered and prevent the keyboard from popping up. But it doesn't seem to do that. Apps that use the keyboard can apparently use this shim safely.
Assignee | ||
Comment 60•12 years ago
|
||
Comment on attachment 694669 [details] [diff] [review] WIP-TabChild-mouse-events-v3.patch Has some issues, fixed in updated patch.
Attachment #694669 -
Flags: feedback?(jones.chris.g)
Assignee | ||
Comment 61•12 years ago
|
||
Note the XXX comments.
Attachment #694503 -
Attachment is obsolete: true
Attachment #694669 -
Attachment is obsolete: true
Attachment #694721 -
Attachment is obsolete: true
Attachment #694726 -
Attachment is obsolete: true
Attachment #694973 -
Attachment is obsolete: true
Attachment #696179 -
Flags: review?(mwu)
Attachment #696179 -
Flags: feedback?(wjohnston)
Assignee | ||
Comment 62•12 years ago
|
||
This is Shih-Chiang's patch from 814252, slightly cleaned up. There's a bug in it in which click events are being eaten for remote content on tap gestures.
Attachment #696181 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 63•12 years ago
|
||
Comment on attachment 696181 [details] [diff] [review] Use touch event for scrolling if available >diff --git a/dom/browser-element/BrowserElementScrolling.js b/dom/browser-element/BrowserElementScrolling.js > onTouchEnd: function cp_onTouchEnd(evt) { >+ this.isScrolling = false; > > this.onTouchMove(evt); >+ let click = (this.watchedEventsType == 'touch') ? true : evt.detail; > if (this.target && click && (this.panning || this.preventNextClick)) { The issue here is that if we detect |this.isScrolling|, we don't have to block the next click because a click event would never be synthesized. By definition !tap <=> isScrolling. In this code however, we reset isScrolling before onTouchMove, which means we can't fix this if() condition. Just moving it below the if() block fixes that.
Attachment #696181 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 64•12 years ago
|
||
Address cjones's review comment ;).
Attachment #696185 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #696181 -
Attachment is obsolete: true
Assignee | ||
Comment 65•12 years ago
|
||
Also, the call here to onTouchMove() in onTouchEnd() is completely useless, because we reset dragging = false before doing it. (That existed before bug 814252.) So I'm going to remove that too.
Assignee | ||
Comment 66•12 years ago
|
||
Ran into an issue that required some relatively nontrivial changes, so requesting review. Shih-Chiang, I would recommend looking at the interdiff between this and v2. (In reply to Chris Jones [:cjones] [:warhammer] from comment #63) > Comment on attachment 696181 [details] [diff] [review] > Use touch event for scrolling if available > > By definition !tap <=> isScrolling. > This is true, except that BrowserElementScrolling doesn't attempt to detect isScrolling if there's not a potential scroll target. So we don't always know for sure whether we want to block clicks. The big change here is using the KineticPanning gesture detector to figure out if a touch series is a click or not. We use that in onTouchEnd() when we're watching touch events. The second change was a small simplification to the isScrolling logic. It's now explicitly, "is detecting scroll".
Attachment #696207 -
Flags: review?(schien)
Assignee | ||
Updated•12 years ago
|
Attachment #696185 -
Attachment is obsolete: true
Assignee | ||
Comment 67•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #61) > Created attachment 696179 [details] [diff] [review] > part 1: Make TabChild dispatch spec-compliant compat mouse events > > Note the XXX comments. The current TabChild event code is firing contextmenu, but I don't understand how. All that logic may be unnecessary.
Assignee | ||
Comment 68•12 years ago
|
||
Aha, this is because the apzc gesture detector already detects long-tap. But nobody does it for sync-scrolled content, so we still need that logic.
Comment 69•12 years ago
|
||
Comment on attachment 696207 [details] [diff] [review] part 2: Use touch event for scrolling if available, v3 Review of attachment 696207 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementScrolling.js @@ +132,5 @@ > KineticPanning.record(new Point(0, 0), evt.timeStamp); > }, > > onTouchEnd: function cp_onTouchEnd(evt) { > + if (!this.dragging || nit: remove tailing space. @@ +198,5 @@ > + // We're going to drive synchronously scrolling an inner frame. > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > + } else { > + // Let AsyncPanZoomController handle the scrolling gesture. Due to the definition change of |this.detectingScrolling|, we'll remove the scrollCallback while we reach the end of scrollable region before receiving touchend event. However, we should be able to scroll the previous selected frame to different direction without re-tap on the touch screen. @@ +203,5 @@ > + this.scrollCallback = null; > + return; > + } > + } > + nit: remove tailing space. @@ +211,5 @@ > this.panning = true; > this._resetActive(); > } > + > + if (this.panning) { |this.panning| will remain false if delta is too small, however, these touch/mouse event were already used for scrolling. I think we also need to consider |isScroll| here for determining if events are consumed by scrolling.
Attachment #696207 -
Flags: review?(schien)
Assignee | ||
Comment 70•12 years ago
|
||
Need to hit the sack. Below are the regressions found with fairly extensive testing. The ones I think are gecko problems I've noted (GECKO), and I have a fix (I think). The rest are gaia regressions that need synthetic_mouse_event love. == Homescreen == - tap events generated when panning starting from icon X and ending on icon X == Dialer == - keypad: long-press on 0 to make + doesn't work - keypad: enter long number, long-press <=, doesn't delete all numbers - (GECKO) keypad: keys not highlighting on press - in-call keypad: tones keep playing when finger moves off key == Gallery == - drag far on image, release on same image => click (Ed: looks like same issue as homescreen) == Contacts == - contact-image expose doesn't work, can't "pull down" == SMS == - "Send" button doesn't work --- some hacky workaround? == Settings == - can't drag "Sound" sliders - (GECKO) rows not highlighted on tap == Music == - scrubber doesn't slide (Ed: works fine in video ...)
Comment 71•12 years ago
|
||
Chris, For these regressions had you applied my gaia patch with the shim and app modifications to use the shim?
Assignee | ||
Comment 72•12 years ago
|
||
That's right; working with synthetic_mouse_events branch rebased on 12b722515af8df6eabad59ac8ecd19d8b0b24734.
Assignee | ||
Comment 73•12 years ago
|
||
Applies on top of gecko-18 b8d7310e257ab133e9c7b1eb5425e1b2cf819398.
Assignee | ||
Comment 74•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #70) > Need to hit the sack. Below are the regressions found with fairly extensive > testing. The ones I think are gecko problems I've noted (GECKO), and I have > a fix (I think). > > The rest are gaia regressions that need synthetic_mouse_event love. > > == Homescreen == > - tap events generated when panning starting from icon X and ending on icon > X > > == Dialer == > - keypad: long-press on 0 to make + doesn't work > - keypad: enter long number, long-press <=, doesn't delete all numbers > - (GECKO) keypad: keys not highlighting on press > - in-call keypad: tones keep playing when finger moves off key > > == Gallery == > - drag far on image, release on same image => click (Ed: looks like same > issue as homescreen) > > == Contacts == > - contact-image expose doesn't work, can't "pull down" > > == SMS == > - "Send" button doesn't work --- some hacky workaround? I should clarify: this worked just fine when I tested on the 21st. So something in the sms app has changed.
Assignee | ||
Comment 75•12 years ago
|
||
This fixes all the gecko bugs listed above. Working on top of gecko-18 d4c41e8c1e4dd8cfa222b7a0cdda59641d25225d and djf's branch merged with gaia 85b6f11cda451d5d5e66aef344a40592d970a4b6. Updated triage Behavioral change: "long-tap", i.e. context menu, gestures no longer generate taps. (Even on buttons, which is a very difficult bug.) == Homescreen == - tap events generated when panning starting from icon X and ending on icon X == Gallery == - drag far on image, release on same image => click (same issue as above) == Dialer == - keypad: keys stay highlighted when finger moves off - in-call keypad: tones keep playing when finger moves off key (same-ish bug as above) == Contacts == - contact-image expose doesn't work, can't "pull down" == SMS == - send to existing contact: "Send" button doesn't work --- some hacky workaround? - enter number into search, write message, "Send": gives E/GeckoConsole( 3027): [JavaScript Error: "Error: Permission denied to access property 'dispatchEvent'" {file: "app://sms.gaiamobile.org/shared/js/mouse_event_shim.js" line: 191}] == Settings == - can't drag "Sound" or "Display" sliders == Music == - scrubber doesn't slide (Ed: works fine in video ...)
Attachment #696344 -
Attachment is obsolete: true
Assignee | ||
Comment 76•12 years ago
|
||
djf: the thing that worries me most is the permission error above. I'm going to take a quick look, but I think that due to the organization of the communications app, we end up trying to dispatch an event cross-domain :|.
Assignee | ||
Comment 77•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #75) > == SMS == > - send to existing contact: "Send" button doesn't work --- some hacky > workaround? > - enter number into search, write message, "Send": gives > E/GeckoConsole( 3027): [JavaScript Error: "Error: Permission denied to > access property 'dispatchEvent'" {file: > "app://sms.gaiamobile.org/shared/js/mouse_event_shim.js" line: 191}] The behavior here is strangely intermittent. I see three different behaviors, trying to sort them out - "Send" button does nothing, no security error - "Send" button does nothing, security error - "Send" button "clicks", but no sms is sent
Assignee | ||
Comment 78•12 years ago
|
||
Heh, the problem here is the event shim itself. The SMS app's usage of mouse events is legit even in a touch environment. If I remove the shim, everything works perfectly, and I scroll the thread without dismissing the keyboard etc.
Assignee | ||
Comment 79•12 years ago
|
||
So remaining work - get the gecko patches r+'d - make homescreen and gallery not get (clicks?) when panning - fix keys on dialer keypad - shim the settings sliders - make the music scrubber behave like the video scrubber
Assignee | ||
Comment 81•12 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] from comment #69) > Comment on attachment 696207 [details] [diff] [review] > part 2: Use touch event for scrolling if available, v3 > > Review of attachment 696207 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementScrolling.js > @@ +132,5 @@ > > KineticPanning.record(new Point(0, 0), evt.timeStamp); > > }, > > > > onTouchEnd: function cp_onTouchEnd(evt) { > > + if (!this.dragging || > > nit: remove tailing space. > Done. > @@ +198,5 @@ > > + // We're going to drive synchronously scrolling an inner frame. > > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > > + } else { > > + // Let AsyncPanZoomController handle the scrolling gesture. > > Due to the definition change of |this.detectingScrolling|, we'll remove the > scrollCallback while we reach the end of scrollable region before receiving > touchend event. However, we should be able to scroll the previous selected > frame to different direction without re-tap on the touch screen. OK, I understand. Nice catch! :) Let me revert this change. > > @@ +203,5 @@ > > + this.scrollCallback = null; > > + return; > > + } > > + } > > + > > nit: remove tailing space. > Done. > @@ +211,5 @@ > > this.panning = true; > > this._resetActive(); > > } > > + > > + if (this.panning) { > > |this.panning| will remain false if delta is too small, however, these > touch/mouse event were already used for scrolling. I think we also need to > consider |isScroll| here for determining if events are consumed by scrolling. The problem here is that we can't disrupt platform tap or contextmenu detection. BrowserElementScrolling only starts "consuming" touchmove events (uses them to scroll) if |this.panning| becomes true. If |this.panning| never becomes true, then the gesture was a tap and BrowserElementScrolling didn't interfere. If we preventDefault() touchmove events when |this.panning| is false, then the following bug occurs - touchstart on target X - touchmove a distance within the tap threshold * BrowserElementScrolling preventDefault()s the touchmove * Gecko blocks sending synthetic click (per spec AFAIK) - touchend * no click is generated on X
Assignee | ||
Comment 82•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #81) > (In reply to Shih-Chiang Chien [:schien] from comment #69) > > Comment on attachment 696207 [details] [diff] [review] > > @@ +198,5 @@ > > > + // We're going to drive synchronously scrolling an inner frame. > > > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > > > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > > > + } else { > > > + // Let AsyncPanZoomController handle the scrolling gesture. > > > > Due to the definition change of |this.detectingScrolling|, we'll remove the > > scrollCallback while we reach the end of scrollable region before receiving > > touchend event. However, we should be able to scroll the previous selected > > frame to different direction without re-tap on the touch screen. Actually, no, I don't see the problem here. detectingScrolling always goes from true -> false on the first touchmove. If that touchmove was below the pan threshold, we hand off to the apzc (as the previous code did). If it was above the pan threshold, then BrowserElementScrolling doesn't set the scrollCallback to null and owns panning from there one. After BrowserElementScrolling owns panning, it's perfectly fine for the scrollCallback to return false. BrowserElementScrolling still continues to own the panning. In other words, this detection code only runs once per pan series. That was something that confused me a bit in your previous patch, which is why I attempted to change it :). Let me know if I missed something.
Assignee | ||
Comment 83•12 years ago
|
||
I'm afraid v4 picks up one more level of complexity. Sorry Shih-Chiang :(. The problem with the previous patch is that it broke :active handling. If we had done things right from the beginning and taught EventStateManager about touch events, this all would have mostly Just Worked. But we didn't. So this patch duplicates yet more of that logic, but not completely correctly. The logic looks like - touchstart * save the "real" event.target as |this.pointerDownTarget| * if we have a pannable |this.target|, then set a timer off which to activate |pointerDownTarget| * otherwise, immediately activate |pointerDownTarget| - touchmove * if we detected a pan, cancel the timer and reset activation - [timer expires] * activate pointerDownTarget - touchend * cancel everything, reset activation
Attachment #696207 -
Attachment is obsolete: true
Attachment #696437 -
Flags: review?(schien)
Assignee | ||
Comment 84•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #79) > - make homescreen and gallery not get (clicks?) when panning The homescreen grid doesn't listen for clicks on elements so I'm not quite sure what's up. Looks like a problem with removing event listeners or the order of event delivery. The gallery isn't doing anything wrong either, it's just relying on the current weird BES behavior. Is mouse_events_shim even needed for the main pannable area? It looks like things should Just Work without it. We would still need it for the sliders, of course.
Comment 85•12 years ago
|
||
I'm on vacation right now, though I may be able to review after new years if no other reviewer is available sooner.
Comment 86•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #82) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #81) > > (In reply to Shih-Chiang Chien [:schien] from comment #69) > > > Comment on attachment 696207 [details] [diff] [review] > > > @@ +198,5 @@ > > > > + // We're going to drive synchronously scrolling an inner frame. > > > > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > > > > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > > > > + } else { > > > > + // Let AsyncPanZoomController handle the scrolling gesture. > > > > > > Due to the definition change of |this.detectingScrolling|, we'll remove the > > > scrollCallback while we reach the end of scrollable region before receiving > > > touchend event. However, we should be able to scroll the previous selected > > > frame to different direction without re-tap on the touch screen. > > Actually, no, I don't see the problem here. detectingScrolling always goes > from true -> false on the first touchmove. If that touchmove was below the > pan threshold, we hand off to the apzc (as the previous code did). If it > was above the pan threshold, then BrowserElementScrolling doesn't set the > scrollCallback to null and owns panning from there one. > > After BrowserElementScrolling owns panning, it's perfectly fine for the > scrollCallback to return false. BrowserElementScrolling still continues to > own the panning. > > In other words, this detection code only runs once per pan series. That was > something that confused me a bit in your previous patch, which is why I > attempted to change it :). > > Let me know if I missed something. Now I understand the usage of |this.detectingScrolling| in your patch, thanks for the explanation. :)
Comment 87•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #79) > So remaining work > - get the gecko patches r+'d > - make homescreen and gallery not get (clicks?) when panning > - fix keys on dialer keypad > - shim the settings sliders > - make the music scrubber behave like the video scrubber In an unrelated code review for the music app, I noticed that the music scrubber is using the layerX property of the mouseevent rather than pageX or clientX. The shim doesn't emulate layerX, so that is why it is breaking. It should be easy to patch. Chris: do you want to break these gaia work items out as separate bugs and assign them to me? Or should we just work on them here?
Assignee | ||
Comment 88•12 years ago
|
||
However you prefer; I think so far the gaia patching has been easy enough that we can have two patches, one for the shim and one for users. All this has to land together so we might as well do it here.
Comment 89•12 years ago
|
||
My heart sings when I see the list of bugs that this bug will positively impact. :)
Comment 90•12 years ago
|
||
Comment on attachment 696437 [details] [diff] [review] part 2: Use touch event for scrolling if available, v4 Review of attachment 696437 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementScrolling.js @@ +6,5 @@ > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > const ContentPanning = { > init: function cp_init() { > + ['mousedown', 'mouseup', 'mousemove', 'touchstart', 'touchend', 'touchmove'].forEach(function(type) { Some people do use: try { content.document.createEvent('TouchEvent'); var events = ['touchstart', 'touchmove', touchend']; } catch(e) { var events = ['mousedown', 'mousemove' 'mouveup']; } events.forEach(...); That would let you get rid of the need for _filterEvent. @@ +78,5 @@ > + if (touches[i].identifier === this.primaryPointerId) { > + return touches[i]; > + } > + } > + return undefined; return null? @@ +85,2 @@ > onTouchStart: function cp_onTouchStart(evt) { > + let target, screenX, screenY; target seems unused. @@ +96,5 @@ > + screenY = firstTouch.screenY; > + } else { > + this.pointerDownTarget = evt.target; > + screenX = evt.screenX; > + screenY = evt.screenY; You may be able to move some of this logic to the switch case directly if you pass evt.changedTouches[0] to a case 'touchstart': this.onTouchStart(evt.changedTouches[0]); break; case 'mousedown': this.onTouchStart(evt); break; I also feel that you can move your findPrimaryPointer calls to handleEvent as well by decoupling the mousemove/touchmove and the mouseup/touchend cases. @@ +109,5 @@ > // this case, and if we are using async panning and zooming on the parent > // frame, inform the pan/zoom controller that it should not attempt to > // handle any touch events it gets until the next batch (meaning the next > // time we get a touch end). > if (this.target != null && ContentPanning._asyncPanZoomForViewportFrame) { nit: ContentPanning -> this @@ +128,5 @@ > + if (this.target === null) { > + this.notify(this._activationTimer); > + } else { > + this._activationTimer.initWithCallback(this, > + /*XXXpref?*/ 50 /*ms*/, 50 seems really small on a device. @@ +219,5 @@ > + // Stop async-pan-zooming if the subframe is really scrolled. > + if (isScroll) { > + // We're going to drive synchronously scrolling an inner frame. > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); Services.os.notifyObservers(...) @@ +226,5 @@ > + this.scrollCallback = null; > + return; > + } > + } > + nit: extra spaces @@ +384,5 @@ > + get _activationTimer() { > + delete this._activationTimer; > + return this._activationTimer = Cc["@mozilla.org/timer;1"] > + .createInstance(Ci.nsITimer); > + }, I guess in this case a _activationTimer: Cc["@mozilla.org/timer;1"] .createInstance(Ci.nsITimer); would be enough @@ +390,3 @@ > _resetActive: function cp_resetActive() { > + let elt = this.target || this.pointerDownTarget; > + // assert elt Is it a leftover or do you want to assert here? ::: gfx/layers/ipc/AsyncPanZoomController.h @@ +592,5 @@ > bool mHandlingTouchQueue; > > + // Flag used to determine whether or not we should try scrolling by > + // BrowserElementScrolling first. If set, this means we should pend touch move > + // event, which not be cosumed by GestureListener. This flag will be reset nit: cosumed -> consumed
Comment 91•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #90) > Some people do use: > try { > content.document.createEvent('TouchEvent'); > var events = ['touchstart', 'touchmove', touchend']; > } catch(e) { > var events = ['mousedown', 'mousemove' 'mouveup']; > } We're really trying to discourage people from writing sites or apps that won't work with touch and mouse simultaneously right now. We're already seeing lots of bugs from it with Windows 8 Metro (where users can choose to switch between mouse and touch at any time based on which they prefer).
Assignee | ||
Comment 92•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #90) > Comment on attachment 696437 [details] [diff] [review] > part 2: Use touch event for scrolling if available, v4 > > Review of attachment 696437 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementScrolling.js > @@ +6,5 @@ > > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > > > const ContentPanning = { > > init: function cp_init() { > > + ['mousedown', 'mouseup', 'mousemove', 'touchstart', 'touchend', 'touchmove'].forEach(function(type) { > > Some people do use: > > try { > content.document.createEvent('TouchEvent'); > > var events = ['touchstart', 'touchmove', touchend']; > } catch(e) { > var events = ['mousedown', 'mousemove' 'mouveup']; > } > > events.forEach(...); > > That would let you get rid of the need for _filterEvent. > Works for me. > @@ +78,5 @@ > > + if (touches[i].identifier === this.primaryPointerId) { > > + return touches[i]; > > + } > > + } > > + return undefined; > > return null? > Sure. > @@ +85,2 @@ > > onTouchStart: function cp_onTouchStart(evt) { > > + let target, screenX, screenY; > > target seems unused. Yep, good call. > @@ +96,5 @@ > > + screenY = firstTouch.screenY; > > + } else { > > + this.pointerDownTarget = evt.target; > > + screenX = evt.screenX; > > + screenY = evt.screenY; > > You may be able to move some of this logic to the switch case directly if > you pass evt.changedTouches[0] to a > IDOMTouch doesn't have all the information we need, like timestamps e.g. > I also feel that you can move your findPrimaryPointer calls to handleEvent > as well by decoupling the mousemove/touchmove and the mouseup/touchend cases. I'm not quite sure I follow ... you're suggesting to move the pointer-detection code into handleEvent() in order to share it between touchmove and touchend? > @@ +109,5 @@ > > // this case, and if we are using async panning and zooming on the parent > > // frame, inform the pan/zoom controller that it should not attempt to > > // handle any touch events it gets until the next batch (meaning the next > > // time we get a touch end). > > if (this.target != null && ContentPanning._asyncPanZoomForViewportFrame) { > > nit: ContentPanning -> this > Changed. > @@ +128,5 @@ > > + if (this.target === null) { > > + this.notify(this._activationTimer); > > + } else { > > + this._activationTimer.initWithCallback(this, > > + /*XXXpref?*/ 50 /*ms*/, > > 50 seems really small on a device. It's about three frames. I chose it based on fairly extensive testing with the settings app. If it's too high, then we don't fix bug 805908. If it's too low, then we don't get a "click" feedback when entering a menu. This is something we can easily tune though. > @@ +219,5 @@ > > + // Stop async-pan-zooming if the subframe is really scrolled. > > + if (isScroll) { > > + // We're going to drive synchronously scrolling an inner frame. > > + var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); > > + os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > > Services.os.notifyObservers(...) > Changed. > @@ +226,5 @@ > > + this.scrollCallback = null; > > + return; > > + } > > + } > > + > > nit: extra spaces > Removed. > @@ +384,5 @@ > > + get _activationTimer() { > > + delete this._activationTimer; > > + return this._activationTimer = Cc["@mozilla.org/timer;1"] > > + .createInstance(Ci.nsITimer); > > + }, > > I guess in this case a _activationTimer: Cc["@mozilla.org/timer;1"] > .createInstance(Ci.nsITimer); > would be enough > Sorry, I don't understand this comment. > @@ +390,3 @@ > > _resetActive: function cp_resetActive() { > > + let elt = this.target || this.pointerDownTarget; > > + // assert elt > > Is it a leftover or do you want to assert here? > I want to assert; I left this in for documentation. But I guess it's pretty obvious that we'll throw below if it's null. Removed. > ::: gfx/layers/ipc/AsyncPanZoomController.h > @@ +592,5 @@ > > bool mHandlingTouchQueue; > > > > + // Flag used to determine whether or not we should try scrolling by > > + // BrowserElementScrolling first. If set, this means we should pend touch move > > + // event, which not be cosumed by GestureListener. This flag will be reset > > nit: cosumed -> consumed Fixed up this comment a bit.
Assignee | ||
Comment 93•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #91) > (In reply to Vivien Nicolas (:vingtetun) from comment #90) > > Some people do use: > > try { > > content.document.createEvent('TouchEvent'); > > var events = ['touchstart', 'touchmove', touchend']; > > } catch(e) { > > var events = ['mousedown', 'mousemove' 'mouveup']; > > } > > We're really trying to discourage people from writing sites or apps that > won't work with touch and mouse simultaneously right now. We're already > seeing lots of bugs from it with Windows 8 Metro (where users can choose to > switch between mouse and touch at any time based on which they prefer). This is code deep within the bowels of gecko that controls auto-scrolling of content. That said, I really think we should be investing in something like the pointer events spec. It's just incredibly hard to do anything nontrivial while having to support both mousemove and touchmove simultaneously.
Assignee | ||
Comment 94•12 years ago
|
||
Attachment #696437 -
Attachment is obsolete: true
Attachment #696437 -
Flags: review?(schien)
Attachment #696841 -
Flags: review?(schien)
Attachment #696841 -
Flags: review?(21)
Comment 95•12 years ago
|
||
Comment on attachment 696841 [details] [diff] [review] part 2: Use touch event for scrolling if available, v5 Review of attachment 696841 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementScrolling.js @@ +11,4 @@ > init: function cp_init() { > + var events; > + try { > + content.document.createEvent('TouchEvent'); In desktop build, we'll not be able to scroll because touch event can still be created. @@ +153,1 @@ > if (this.target && click && (this.panning || this.preventNextClick)) { For touch event, the !KineticPanning.isPan() and this.panning will always be the opposite value, therefore, the unexpected click will still be triggered while swiping on screen. @@ +200,5 @@ > + if (!this.scrollCallback) { > + return; > + } > + > + let isScroll = this.scrollCallback(delta.scale(-1)); I think we should only perform scrolling while |KinecticPanning.isPan() == true| since we're using KinecticPanning for distinguishing panning and click. @@ +206,5 @@ > + this.detectingScrolling = false; > + // Stop async-pan-zooming if the subframe is really scrolled. > + if (isScroll) { > + // We're going to drive synchronously scrolling an inner frame. > + Services.os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); typo: should be Services.obs
Attachment #696841 -
Flags: review?(schien) → review-
Chris: Mwu is on PTO until the 14th, so we need a different reviewer for part 1.
Assignee: nobody → jones.chris.g
Comment 97•12 years ago
|
||
Comment on attachment 696179 [details] [diff] [review] part 1: Make TabChild dispatch spec-compliant compat mouse events Review of attachment 696179 [details] [diff] [review]: ----------------------------------------------------------------- This all seems sane to me. The interaction of some of these things with :active is a bit strange for us, but its probably better handled somewhere else... ::: dom/ipc/TabChild.cpp @@ +1465,5 @@ > +void > +TabChild::FireContextMenuEvent() > +{ > + MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0); > + // XXX do we need to fire mousemove+mousedown here? We don't do that on Fennec and I've never seen any problems from it.
Attachment #696179 -
Flags: feedback?(wjohnston) → feedback+
Assignee | ||
Comment 98•12 years ago
|
||
(In reply to Shih-Chiang Chien [:schien] from comment #95) > Comment on attachment 696841 [details] [diff] [review] > part 2: Use touch event for scrolling if available, v5 > > Review of attachment 696841 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementScrolling.js > @@ +11,4 @@ > > init: function cp_init() { > > + var events; > > + try { > > + content.document.createEvent('TouchEvent'); > > In desktop build, we'll not be able to scroll because touch event can still > be created. A simple test gives me [13:08:11.337] NotSupportedError: Operation is not supported @ file:///home/cjones/Desktop/test.html:37 but I'll double-check in a b2g desktop build. > @@ +153,1 @@ > > if (this.target && click && (this.panning || this.preventNextClick)) { > > For touch event, the !KineticPanning.isPan() and this.panning will always be > the opposite value, therefore, the unexpected click will still be triggered > while swiping on screen. There's no synthetic click generated when KineticPanning.isPan() is true and we're using touch events. > @@ +200,5 @@ > > + if (!this.scrollCallback) { > > + return; > > + } > > + > > + let isScroll = this.scrollCallback(delta.scale(-1)); > > I think we should only perform scrolling while |KinecticPanning.isPan() == > true| since we're using KinecticPanning for distinguishing panning and click. That makes sense, good catch. > @@ +206,5 @@ > > + this.detectingScrolling = false; > > + // Stop async-pan-zooming if the subframe is really scrolled. > > + if (isScroll) { > > + // We're going to drive synchronously scrolling an inner frame. > > + Services.os.notifyObservers(docShell, 'cancel-default-pan-zoom', null); > > typo: should be Services.obs Oops!
Assignee | ||
Comment 99•12 years ago
|
||
(In reply to Wesley Johnston (:wesj) from comment #97) > Comment on attachment 696179 [details] [diff] [review] > part 1: Make TabChild dispatch spec-compliant compat mouse events > > Review of attachment 696179 [details] [diff] [review]: > ----------------------------------------------------------------- > > This all seems sane to me. The interaction of some of these things with > :active is a bit strange for us, but its probably better handled somewhere > else... > > ::: dom/ipc/TabChild.cpp > @@ +1465,5 @@ > > +void > > +TabChild::FireContextMenuEvent() > > +{ > > + MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0); > > + // XXX do we need to fire mousemove+mousedown here? > > We don't do that on Fennec and I've never seen any problems from it. Good to know, thanks.
Assignee | ||
Comment 100•12 years ago
|
||
Will try to find an alternate reviewer in the meantime.
Attachment #696179 -
Attachment is obsolete: true
Attachment #696179 -
Flags: review?(mwu)
Attachment #697281 -
Flags: review?(mwu)
Assignee | ||
Comment 101•12 years ago
|
||
Addresses Shih-Chiang's comments. We were enabling touch events in b2g desktop builds; this patch turns that off.
Attachment #696841 -
Attachment is obsolete: true
Attachment #696841 -
Flags: review?(21)
Attachment #697282 -
Flags: review?(schien)
Attachment #697282 -
Flags: review?(21)
Assignee | ||
Comment 103•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #100) > Created attachment 697281 [details] [diff] [review] > part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1 jlebar, your review queue seems to be pretty substantial, but do you think you could fill in for mwu here?
Flags: needinfo?(justin.lebar+bug)
Comment 104•12 years ago
|
||
Comment on attachment 697281 [details] [diff] [review] part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1 Sure, I can look at this.
Attachment #697281 -
Flags: review?(mwu) → review?(justin.lebar+bug)
Flags: needinfo?(justin.lebar+bug)
Comment 106•12 years ago
|
||
Comment on attachment 697282 [details] [diff] [review] part 2: Use touch event for scrolling if available, v6 Review of attachment 697282 [details] [diff] [review]: ----------------------------------------------------------------- looks good to me, except for one little problem. ::: dom/browser-element/BrowserElementScrolling.js @@ +218,5 @@ > + // Let AsyncPanZoomController handle the scrolling gesture. > + this.scrollCallback = null; > + return; > + } > + } The expressions above mean APZC will be enable if our first scrolling gesture moves only a tiny distance, i.e., we cannot scroll subframe while we scroll the web page very slowly. However, this defect is not perceptible under regular user operation IMO.
Attachment #697282 -
Flags: review?(schien) → review+
Updated•12 years ago
|
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Comment 107•12 years ago
|
||
Comment on attachment 697282 [details] [diff] [review] part 2: Use touch event for scrolling if available, v6 Review of attachment 697282 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementScrolling.js @@ +116,5 @@ > + if (this.target === null) { > + this.notify(this._activationTimer); > + } else { > + this._activationTimer.initWithCallback(this, > + /*XXXpref?*/ 50 /*ms*/, I tried it on a device and I bet this is too short. What about adding a pref right now so UX will be able to tweak it easily in Gaia? @@ +154,5 @@ > + // event. That's OK however, because we won't fire a synthetic > + // click when we're using touch events and this touch series > + // wasn't a "tap" gesture. > + let click = (this.watchedEventsType == 'mouse') ? > + evt.detail : !KineticPanning.isPan(); This code will still fire a click if you move your finger back to the original position. Also it seems like the plaform generates a click when you pan but the target is still under your finger.
Assignee | ||
Comment 108•12 years ago
|
||
Vivien points out that these patches break text selection in app processes. (It's already broken in browser processes, and it continues to work in the browser UI.) That's not fixable in the scope of v1, so we're going to have to declare bankruptcy on that regression.
Comment 109•12 years ago
|
||
Comment on attachment 697282 [details] [diff] [review] part 2: Use touch event for scrolling if available, v6 Review of attachment 697282 [details] [diff] [review]: ----------------------------------------------------------------- I will be fine to r+ this code once the click issue is resolved. Steps to reproduce: - Go to the UI tests app - Choose 'Select tests' - Choose the second select where the content is scrollable - Click on 'basecamp' and scroll it to the top keeping the finger on it Actual result: - 'basecamp' is selected even if you have pan more than what is expected for a click Expected result: - no click
Attachment #697282 -
Flags: review?(21) → review-
Assignee | ||
Comment 110•12 years ago
|
||
Is the b2g process drawing that UI? If so I think I know what the problem is.
Flags: needinfo?(21)
Assignee | ||
Comment 112•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #107) > Comment on attachment 697282 [details] [diff] [review] > part 2: Use touch event for scrolling if available, v6 > > Review of attachment 697282 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementScrolling.js > @@ +116,5 @@ > > + if (this.target === null) { > > + this.notify(this._activationTimer); > > + } else { > > + this._activationTimer.initWithCallback(this, > > + /*XXXpref?*/ 50 /*ms*/, > > I tried it on a device and I bet this is too short. What about adding a pref > right now so UX will be able to tweak it easily in Gaia? > I keep meaning to add the pref, will do. I tuned this in the settings app: when we tap a category header, we want to see it highlight when we transition into the subview. At 100ms, I didn't ever really see highlight. The second case is panning around the categories list. With 50ms I mostly don't see the activation highlight when starting a pan. At 25ms, it was the opposite, I almost always saw it. > @@ +154,5 @@ > > + // event. That's OK however, because we won't fire a synthetic > > + // click when we're using touch events and this touch series > > + // wasn't a "tap" gesture. > > + let click = (this.watchedEventsType == 'mouse') ? > > + evt.detail : !KineticPanning.isPan(); > > This code will still fire a click if you move your finger back to the > original position. > Also it seems like the plaform generates a click when you pan but the target > is still under your finger. This doesn't happen in content processes, but you're right, it occurs in the (still non-compliant) master process. My fix/hack takes care of this.
Assignee | ||
Comment 113•12 years ago
|
||
Interdiff is going to fall over. Addresses review comments and adds hack for b2g process.
Attachment #697637 -
Flags: review?(21)
Assignee | ||
Comment 114•12 years ago
|
||
Attachment #697282 -
Attachment is obsolete: true
Attachment #697283 -
Attachment is obsolete: true
Assignee | ||
Comment 115•12 years ago
|
||
Assignee | ||
Comment 116•12 years ago
|
||
Testing on http://people.mozilla.com/~cjones/events.html with a "tap" gesture, here's a samping of behaviors fennec: touchstart touchend mousemove mousedown mouseup click b2g: touchstart touchend mousemove mousedown mouseup click chrome-android-JB: touchstart touchend mousemove mousedown mouseup click android-browser-JB: mousemove touchstart touchend mousedown mouseup click mobilesafari-ios5: touchstart touchend All behaviors comply with spec. Yay for great specs! I think it's most important for us to align with fennec here, and now we do. chrome is also in our camp, and humorously, a different one from the stock android browser. Although TBH I like the behavior of android-browser ...
Comment 117•12 years ago
|
||
I think we've got most of the known gaia regressions fixed. Current status is at https://etherpad.mozilla.org/touch-regressions I've been pushing fixes to my synthetic_mouse_events branch on github: https://github.com/mozilla-b2g/gaia/pull/7120 Fixes to the shim include: a setCapture() helper, mouseenter/mouseleave events, and not sending click events if the touch moves more than 25px. Its probably time to start reviewing the gaia side of this patch. Who wants to do it? cjones? cpeterson? vingtetun?
Comment 118•12 years ago
|
||
Note that becuase of bug 825840 I have not rebuilt gecko with the latest rollup patch, so I've been working on the gaia side using a Vivien's build from this morning. We'll need to test again with the latest version of the gecko patches. Things that remain on my todo list include grepping for mousemove in all apps to make sure that we haven't overlooked any that need the shim.
Comment 119•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #117) > I think we've got most of the known gaia regressions fixed. > > Current status is at https://etherpad.mozilla.org/touch-regressions > > I've been pushing fixes to my synthetic_mouse_events branch on github: > https://github.com/mozilla-b2g/gaia/pull/7120 > > Fixes to the shim include: a setCapture() helper, mouseenter/mouseleave > events, and not sending click events if the touch moves more than 25px. > > Its probably time to start reviewing the gaia side of this patch. Who wants > to do it? cjones? cpeterson? vingtetun? I can do it.
Comment 120•12 years ago
|
||
Comment on attachment 697640 [details] [diff] [review] part 2: Use touch event for scrolling if available, v7 Review of attachment 697640 [details] [diff] [review]: ----------------------------------------------------------------- I feel like some parts of the code could be written differently to make it easier to read/maintain the code but nothing that should prevent this to land. Nice work!
Attachment #697640 -
Flags: review+
Attachment #697637 -
Flags: review?(21) → review+
Comment 121•12 years ago
|
||
There is still a small bug with UI tests: Steps to reproduce: - Go to the UI tests app - Choose 'Select tests' - Choose the second select where the content is scrollable - Click on 'basecamp' and scroll it to the top - Click on 'Gecko-2' and start to pan to the bottom Actual result: - The scrollable area jump to the top and scroll from here Expected result: - The scrollable area scroll from the current point
Comment 122•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #121) > There is still a small bug with UI tests: > > Steps to reproduce: > - Go to the UI tests app > - Choose 'Select tests' > - Choose the second select where the content is scrollable > - Click on 'basecamp' and scroll it to the top > - Click on 'Gecko-2' and start to pan to the bottom > > Actual result: > - The scrollable area jump to the top and scroll from here > > Expected result: > - The scrollable area scroll from the current point It seems like this.position.set(eventX, eventY) is not set correctly and so the first onTouchMove call generate a wrong delta.
Comment 123•12 years ago
|
||
Comment on attachment 697281 [details] [diff] [review] part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1 >+ nsIntPoint currentPoint = trackedTouch->mRefPoint; >+ int64_t time = aEvent.time; >+ switch (aEvent.message) { >+ case NS_TOUCH_MOVE: >+ if (abs(currentPoint.x - mGestureDownPoint.x) > sDragThreshold.width || >+ abs(currentPoint.y - mGestureDownPoint.y) > sDragThreshold.height) { It seems weird to use L-infnity distance instead of L2 distance here. I guess this is OK because the drag threshold is small, so one won't notice the difference? >+void >+TabChild::FireContextMenuEvent() >+{ >+ MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0); >+ RecvHandleLongTap(mGestureDownPoint); >+ CancelTapTracking(); This can fire (long) after TabChild::ActorDestroy. It looks kosher to call RecvHandleLongTap at this point, but I wanted to check. (Perhaps you should cancel the timer from ActorDestroy anyway, since the timer will keep the TabParent alive, which could, at the very least, cause false mochitest leaks.)
Attachment #697281 -
Flags: review?(justin.lebar+bug) → review+
Comment 124•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #122) > (In reply to Vivien Nicolas (:vingtetun) from comment #121) > > There is still a small bug with UI tests: > > > > Steps to reproduce: > > - Go to the UI tests app > > - Choose 'Select tests' > > - Choose the second select where the content is scrollable > > - Click on 'basecamp' and scroll it to the top > > - Click on 'Gecko-2' and start to pan to the bottom > > > > Actual result: > > - The scrollable area jump to the top and scroll from here > > > > Expected result: > > - The scrollable area scroll from the current point > > It seems like this.position.set(eventX, eventY) is not set correctly and so > the first onTouchMove call generate a wrong delta. Sorry for the spam it seems like I fail to merged all.js in my build and so the code is failing with an exception on reading the new preference for the 'active' callback. I'm rebuilding but I bet it will work now.
Comment 125•12 years ago
|
||
Comment on attachment 694591 [details]
link to patch on github
Vivien,
Thanks for agreeing to review this. The patch is still a bunch of separate commits right now, but I will of course rebase and merge into one commit before landing. I can do that right away if it makes your review easier.
I'll continue to look for gaia-side regressions and there is a chance I may add more commits, but I think it is basically ready for review now.
Attachment #694591 -
Flags: review?(21)
Comment 126•12 years ago
|
||
(In reply to David Flanagan [:djf] from comment #125) > Comment on attachment 694591 [details] > link to patch on github > > Vivien, > > Thanks for agreeing to review this. The patch is still a bunch of separate > commits right now, but I will of course rebase and merge into one commit > before landing. I can do that right away if it makes your review easier. > > I'll continue to look for gaia-side regressions and there is a chance I may > add more commits, but I think it is basically ready for review now. I have added my review comments on github already. Thanks timezone shift! (And I have spent time to look for regressions and didn't find new ones)
Assignee | ||
Comment 127•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #121) > There is still a small bug with UI tests: > > Steps to reproduce: > - Go to the UI tests app > - Choose 'Select tests' > - Choose the second select where the content is scrollable > - Click on 'basecamp' and scroll it to the top > - Click on 'Gecko-2' and start to pan to the bottom > > Actual result: > - The scrollable area jump to the top and scroll from here > > Expected result: > - The scrollable area scroll from the current point I can't reproduce this. I hope you can't in your updated build :).
Assignee | ||
Comment 128•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #123) > Comment on attachment 697281 [details] [diff] [review] > part 1: Make TabChild dispatch spec-compliant compat mouse events, v1.1 > > >+ nsIntPoint currentPoint = trackedTouch->mRefPoint; > >+ int64_t time = aEvent.time; > >+ switch (aEvent.message) { > >+ case NS_TOUCH_MOVE: > >+ if (abs(currentPoint.x - mGestureDownPoint.x) > sDragThreshold.width || > >+ abs(currentPoint.y - mGestureDownPoint.y) > sDragThreshold.height) { > > It seems weird to use L-infnity distance instead of L2 distance here. I > guess > this is OK because the drag threshold is small, so one won't notice the > difference? > This code is taken verbatim from nsEventStateManager, so consistency is the goal. This is a discussion to have elsewhere. > >+void > >+TabChild::FireContextMenuEvent() > >+{ > >+ MOZ_ASSERT(mTapHoldTimer && mActivePointerId >= 0); > >+ RecvHandleLongTap(mGestureDownPoint); > >+ CancelTapTracking(); > > This can fire (long) after TabChild::ActorDestroy. It looks kosher to call > RecvHandleLongTap at this point, but I wanted to check. We'll never see TabChild::ActorDestroy in release builds. Separately, yes it's kosher for this timer to fire afterwards. > (Perhaps you should cancel the timer from ActorDestroy anyway, since the > timer > will keep the TabParent alive, which could, at the very least, cause false > mochitest leaks.) Do we clear timer listeners on xpcom-shutdown or whichever?
Assignee | ||
Comment 129•12 years ago
|
||
I agree that we should cancel tracking taps at ActorDestroy(). I don't think it would cause any problems not to, but it's pointless to continue doing so (since we won't receive any more input events) and could just lead to badness.
Assignee | ||
Comment 130•12 years ago
|
||
In fact, we should never receive ActorDestroy if we're tracking taps except in emergencies (parent-process crash).
Assignee | ||
Comment 131•12 years ago
|
||
Attachment #697281 -
Attachment is obsolete: true
Attachment #697637 -
Attachment is obsolete: true
Attachment #697640 -
Attachment is obsolete: true
Attachment #697641 -
Attachment is obsolete: true
Attachment #698182 -
Flags: review+
Assignee | ||
Comment 133•12 years ago
|
||
Gonk widget backend remains broken.
Summary: Make mouse events synthesized from touch events comply with spec (in gonk widget backend and cross-process code) → Make mouse events synthesized from touch events comply with spec (in apps)
Comment 134•12 years ago
|
||
Comment on attachment 694591 [details]
link to patch on github
Vivien reviewed and approved this patch, but forgot to mark r+ here.
Attachment #694591 -
Flags: review?(21) → review+
Comment 135•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6172df451c1c https://hg.mozilla.org/releases/mozilla-aurora/rev/9c07ef512e29 https://hg.mozilla.org/releases/mozilla-b2g18/rev/445511ee3465 master: https://github.com/mozilla-b2g/gaia/commit/0a66aae25a92c54cc9e12c3d884feb7cd7590930 nightly: https://github.com/mozilla-b2g/gaia/commit/b506bdbd5bcdb042e3a8ca5d62856ae5d683c176
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Assignee | ||
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•