Closed
Bug 597547
Opened 14 years ago
Closed 14 years ago
Pinch to zoom does not work when fingers are close together
Categories
(Firefox for Android Graveyard :: Panning/Zooming, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: stechz, Assigned: stechz)
References
Details
Attachments
(6 files, 6 obsolete files)
16.72 KB,
patch
|
stechz
:
review+
|
Details | Diff | Splinter Review |
6.23 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
4.39 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
stechz
:
review+
|
Details | Diff | Splinter Review |
5.09 KB,
patch
|
stechz
:
review+
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
This is because of our grab model in inputhandler. As I see it, the InputHandler causes more trouble than it's worth, making it hard to understand what happens in our mouse module and our gesture code. Instead, we can have a much simpler "CancelTouchSequence" event that anything touch related can listen to. Patches coming.
Assignee | ||
Comment 1•14 years ago
|
||
Assignee | ||
Comment 2•14 years ago
|
||
Assignee | ||
Comment 3•14 years ago
|
||
Assignee | ||
Comment 4•14 years ago
|
||
Assignee | ||
Comment 5•14 years ago
|
||
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #476392 -
Flags: review?(mbrubeck)
Assignee | ||
Comment 7•14 years ago
|
||
I should note that only the very first patch is needed, but I followed through to the see the very end of InputHandler. All we really need to land is the first one.
Comment 8•14 years ago
|
||
Comment on attachment 476392 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug) This looks fine to me. Mark should probably review this too.
Attachment #476392 -
Flags: review?(mbrubeck)
Attachment #476392 -
Flags: review?(mark.finkle)
Attachment #476392 -
Flags: review+
Comment 9•14 years ago
|
||
Comment on attachment 476397 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - rename InputHandler file This is missing a change to chrome/jar.mn
Comment 10•14 years ago
|
||
(In reply to comment #9) > This is missing a change to chrome/jar.mn And to browser.xul
Comment 11•14 years ago
|
||
Comment on attachment 476392 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug) > handleEvent: function handleEvent(ev) { >+ // ignore content events we generate >+ if (ev.target.localName == "browser") >+ return; >+ > if (!this._targetIsContent(ev)) { > TapHighlightHelper.hide(); > this._dispatchMouseEvent("Browser:MouseCancel"); > return; > } These two "if" checks seem redundant, since _targetIsContent should return false whenever target.localName == "browser". Using these patches on Android, I often get a tap highlight that stays on the screen while I am pinch-zooming. I think we need another TapHighlightHelper.hide() somewhere - possibly in a CancelTouchSequence event listener?
Comment 12•14 years ago
|
||
> These two "if" checks seem redundant, since _targetIsContent should return
> false whenever target.localName == "browser".
Sorry, ignore that comment - I was not thinking clearly.
Assignee | ||
Comment 13•14 years ago
|
||
> Using these patches on Android, I often get a tap highlight that stays on the
> screen while I am pinch-zooming. I think we need another
> TapHighlightHelper.hide() somewhere - possibly in a CancelTouchSequence event
> listener?
Yes, exactly. Sometimes this doesn't help because the highlight message happens after the touch sequence begins, but I also have a patch for that. I'll update this soon.
Comment 14•14 years ago
|
||
More feedback: I think it makes sense for MouseModule and its peers to be singleton objects instead of constructors.
Assignee | ||
Comment 15•14 years ago
|
||
Comment on attachment 476392 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug) To be clear, only the *first* patch is necessary. It fixes the bug by not using InputHandler's "grab" functionality anymore, replacing it with a CancelTouchEvent. "grab" has the problem of not allowing other modules to grab an event after one has already grabbed it. Instead of rearchitecting InputHandler to support this functionality, I used Mozilla's native event system to do the job of InputHandler for me.
Attachment #476392 -
Attachment description: Pinch to zoom does not work when fingers are close together - divorce touch events from input handler → Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug)
Comment 16•14 years ago
|
||
Comment on attachment 476392 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug) >diff -r 5dd23d5d592e -r a5da4d004fed chrome/content/InputHandler.js >+ new MouseModule(); >+ new GestureModule(); nit from IRC: Use singletons >- suppressNextClick: function suppressNextClick() { >- allowClicks: function allowClicks() { * Are we sure suppressNextClick and allowClicks are handled correctly in the new code? * What kind of tests will these patches require? A change like this is a great time to build some browser-chrome (or appropriate) automated tests. >+ window.addEventListener("mousedown", this, true); >+ window.addEventListener("mouseup", this, true); >+ window.addEventListener("mousemove", this, true); >+ window.addEventListener("CancelTouchSequence", this, true); I know the end result of handling the mouse events is a touch sequence, but the name doesn't fit in very well with the impl. I can't think of a likeable name for now, so we can wait. "MouseCancel" comes to mind. >+ default: { >+ // Filter out mouse events that aren't first button >+ if (aEvent.button === 0) >+ >+ switch (aEvent.type) { >+ case "mousedown": >+ this._onMouseDown(aEvent); >+ break; >+ case "mousemove": >+ aEvent.stopPropagation(); >+ aEvent.preventDefault(); >+ this._onMouseMove(aEvent); >+ break; >+ case "mouseup": >+ this._onMouseUp(aEvent); >+ break; >+ } nit: Indent the switch and add {} for the if. I know it's a single statement, but it's too long of a block and too easy to mess up in maintenance. r+ but we need tests and fix the nits Definitely post beta 1
Attachment #476392 -
Flags: review?(mark.finkle) → review+
Updated•14 years ago
|
tracking-fennec: --- → ?
Flags: in-litmus?(mozaakash)
Comment 17•14 years ago
|
||
Comment on attachment 476397 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - rename InputHandler file Feedback on the refactor patches: * Don't like "touch.js" - it's about more than touch. It's really about input events, right? InputEvents.js or inputs.js? * XxxModule is boring too. XxxEvents? (MouseEvents, GestureEvents, ScrollwheelEvents) * I think we should move KeyEvents in here too. Do we really not want to keep the key events separated? If we do want to merge the key events into the ContentCustomKeySender we should probably rename ContentCustomKeySender, since it deals with more than content, right? KeyEvents? * Would we have the same benefits to moving MouseModule, etc into the CustomMouseSender? We could just pull the custom senders out of browser.js and keep them in inputs.js or events.js
Assignee | ||
Comment 18•14 years ago
|
||
> * Don't like "touch.js" - it's about more than touch. It's really about input > events, right? InputEvents.js or inputs.js? There's no keyboard events anymore. I suppose scrollwheel doesn't quite fit. inputs.js is fine with me though. > * XxxModule is boring too. XxxEvents? (MouseEvents, GestureEvents, > ScrollwheelEvents) Sounds good to me. > * I think we should move KeyEvents in here too. Do we really not want to keep > the key events separated? If we do want to merge the key events into the Ah, this is why you want inputs.js! This is fine with me, though I'd also probably want to move ContentTouchHandler (or whatever we call it) in there too, for consistency. > ContentCustomKeySender we should probably rename ContentCustomKeySender, since > it deals with more than content, right? KeyEvents? It does? > * Would we have the same benefits to moving MouseModule, etc into the > CustomMouseSender? We could just pull the custom senders out of browser.js and > keep them in inputs.js or events.js I'm fine with moving that in inputs.js, but MouseModule should remain independent IMO. MouseModule has some nice properties, in that it encapsulates exactly what kind of events we really want from the platform (when to pan, when to click, when to double tap, etc).
Assignee | ||
Updated•14 years ago
|
Attachment #476393 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #476394 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #476395 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•14 years ago
|
Attachment #476396 -
Flags: review?(mbrubeck)
Attachment #476396 -
Flags: review?(mark.finkle)
Updated•14 years ago
|
Attachment #476395 -
Flags: review?(mbrubeck) → review+
Comment 19•14 years ago
|
||
Comment on attachment 476396 [details] [diff] [review] Pinch to zoom does not work when fingers are close together - Kill InputHandler object! > let inputHandlerOverlay = document.getElementById("inputhandler-overlay"); >+ inputHandlerOverlay.customDragger = new Browser.MainDragger(); Do we want to rename the overlay while we're at it? I can't think of a better name off-hand, so I don't care too much.
Attachment #476396 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 20•14 years ago
|
||
(BTW, I will make a new patch that does singletons and another that moves input-related stuff into input.js)
Comment 21•14 years ago
|
||
I found a regression with these patches: 1. Open the awesomescreen 2. Press on the list and drag up and down. 3. Release your mouse button (or finger). Expected results: List stops panning, nothing happens. Actual results: Fennec begins opening one of the items from the list.
Assignee | ||
Comment 22•14 years ago
|
||
I have this fixed in a patch I haven't posted. It turns out preventDefault on mouseup does not stop the click!
Assignee | ||
Comment 23•14 years ago
|
||
Assignee | ||
Comment 24•14 years ago
|
||
Assignee | ||
Comment 25•14 years ago
|
||
Assignee | ||
Comment 26•14 years ago
|
||
Assignee | ||
Comment 27•14 years ago
|
||
Assignee | ||
Comment 28•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #476392 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #476393 -
Attachment is obsolete: true
Attachment #476393 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #476394 -
Attachment is obsolete: true
Attachment #476394 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #476395 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #476396 -
Attachment is obsolete: true
Attachment #476396 -
Flags: review?(mbrubeck)
Assignee | ||
Updated•14 years ago
|
Attachment #476397 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #477721 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #477722 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #477723 -
Flags: review?(mark.finkle)
Assignee | ||
Updated•14 years ago
|
Attachment #477724 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #477725 -
Attachment description: , part 5: Kill InputHandler object! → part 5: Kill InputHandler object!
Attachment #477725 -
Flags: review+
Assignee | ||
Updated•14 years ago
|
Attachment #477726 -
Attachment description: , part 6: rename InputHandler file → part 6: rename InputHandler file
Attachment #477726 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 29•14 years ago
|
||
> (BTW, I will make a new patch that does singletons and another that moves
> input-related stuff into input.js)
Could we move this to a new bug?
Updated•14 years ago
|
Attachment #477722 -
Flags: review?(mark.finkle) → review+
Updated•14 years ago
|
Attachment #477723 -
Flags: review?(mark.finkle) → review+
Updated•14 years ago
|
Attachment #477726 -
Flags: review?(mark.finkle) → review+
Comment 30•14 years ago
|
||
(In reply to comment #29) > > (BTW, I will make a new patch that does singletons and another that moves > > input-related stuff into input.js) > > Could we move this to a new bug? Yes. I want to see how all these refactors work together and then we should do a "cleanup based on feedback" bug
Assignee | ||
Comment 31•14 years ago
|
||
Pushed http://hg.mozilla.org/mobile-browser/rev/b76ac56f3321
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 32•14 years ago
|
||
It'd be nice to get some unit tests on this patch. verified FIXED on build: Mozilla/5.0 (Android; Linux armv71; rv:2.0b6pre) Gecko/20100923 Namoroka/4.0b7pre Fennec/2.0b1pre
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Updated•14 years ago
|
Flags: in-litmus?(mozaakash) → in-litmus-
Updated•10 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•