Closed Bug 597547 Opened 10 years ago Closed 10 years ago

Pinch to zoom does not work when fingers are close together

Categories

(Firefox for Android Graveyard :: Panning/Zooming, defect)

x86
macOS
defect
Not set
normal

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.
Attachment #476392 - Flags: review?(mbrubeck)
Depends on: 597286
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 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 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
(In reply to comment #9)
> This is missing a change to chrome/jar.mn

And to browser.xul
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?
> These two "if" checks seem redundant, since _targetIsContent should return
> false whenever target.localName == "browser".

Sorry, ignore that comment - I was not thinking clearly.
> 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.
More feedback: I think it makes sense for MouseModule and its peers to be singleton objects instead of constructors.
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 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+
tracking-fennec: --- → ?
Flags: in-litmus?(mozaakash)
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
> * 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).
Attachment #476393 - Flags: review?(mark.finkle)
Attachment #476394 - Flags: review?(mark.finkle)
Attachment #476395 - Flags: review?(mbrubeck)
Attachment #476396 - Flags: review?(mbrubeck)
Attachment #476396 - Flags: review?(mark.finkle)
Attachment #476395 - Flags: review?(mbrubeck) → review+
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+
(BTW, I will make a new patch that does singletons and another that moves input-related stuff into input.js)
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.
I have this fixed in a patch I haven't posted. It turns out preventDefault on mouseup does not stop the click!
Attachment #476392 - Attachment is obsolete: true
Attachment #476393 - Attachment is obsolete: true
Attachment #476393 - Flags: review?(mark.finkle)
Attachment #476394 - Attachment is obsolete: true
Attachment #476394 - Flags: review?(mark.finkle)
Attachment #476395 - Attachment is obsolete: true
Attachment #476396 - Attachment is obsolete: true
Attachment #476396 - Flags: review?(mbrubeck)
Attachment #476397 - Attachment is obsolete: true
Attachment #477721 - Flags: review+
Attachment #477722 - Flags: review?(mark.finkle)
Attachment #477723 - Flags: review?(mark.finkle)
Attachment #477724 - Flags: review+
Attachment #477725 - Attachment description: , part 5: Kill InputHandler object! → part 5: Kill InputHandler object!
Attachment #477725 - Flags: review+
Attachment #477726 - Attachment description: , part 6: rename InputHandler file → part 6: rename InputHandler file
Attachment #477726 - Flags: review?(mark.finkle)
Blocks: 597600
> (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?
Attachment #477722 - Flags: review?(mark.finkle) → review+
Attachment #477723 - Flags: review?(mark.finkle) → review+
Attachment #477726 - Flags: review?(mark.finkle) → review+
(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
Pushed http://hg.mozilla.org/mobile-browser/rev/b76ac56f3321
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Flags: in-litmus?(mozaakash) → in-litmus-
bugspam
Assignee: nobody → ben
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.