Pinch to zoom does not work when fingers are close together

VERIFIED FIXED

Status

Firefox for Android Graveyard
Panning/Zooming
VERIFIED FIXED
8 years ago
4 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

Trunk
x86
Mac OS X
Dependency tree / graph
Bug Flags:
in-testsuite ?
in-litmus -

Details

Attachments

(6 attachments, 6 obsolete attachments)

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
(Assignee)

Description

8 years ago
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

8 years ago
Created attachment 476392 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - divorce touch events from input handler (fixes bug)
(Assignee)

Comment 2

8 years ago
Created attachment 476393 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - Kill grab
(Assignee)

Comment 3

8 years ago
Created attachment 476394 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - Divorce keyinput from InputHandler
(Assignee)

Comment 4

8 years ago
Created attachment 476395 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - Divorce scrollwhell from InputHandler
(Assignee)

Comment 5

8 years ago
Created attachment 476396 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - Kill InputHandler object!
(Assignee)

Comment 6

8 years ago
Created attachment 476397 [details] [diff] [review]
Pinch to zoom does not work when fingers are close together - rename InputHandler file
(Assignee)

Updated

8 years ago
Attachment #476392 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
Depends on: 597286
(Assignee)

Comment 7

8 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 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.
(Assignee)

Comment 13

8 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.
More feedback: I think it makes sense for MouseModule and its peers to be singleton objects instead of constructors.
(Assignee)

Comment 15

8 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 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
(Assignee)

Comment 18

8 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

8 years ago
Attachment #476393 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #476394 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #476395 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 20

8 years ago
(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.
(Assignee)

Comment 22

8 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

8 years ago
Created attachment 477721 [details] [diff] [review]
part 1: divorce mousemodule and gesturemodule from inputhandler
(Assignee)

Comment 24

8 years ago
Created attachment 477722 [details] [diff] [review]
part 2: Kill grab
(Assignee)

Comment 25

8 years ago
Created attachment 477723 [details] [diff] [review]
part 3: Divorce keyinput from InputHandler
(Assignee)

Comment 26

8 years ago
Created attachment 477724 [details] [diff] [review]
part 4:  Divorce scrollwhell from InputHandler
(Assignee)

Comment 27

8 years ago
Created attachment 477725 [details] [diff] [review]
part 5: Kill InputHandler object!
(Assignee)

Comment 28

8 years ago
Created attachment 477726 [details] [diff] [review]
part 6: rename InputHandler file
(Assignee)

Updated

8 years ago
Attachment #476392 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #476393 - Attachment is obsolete: true
Attachment #476393 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #476394 - Attachment is obsolete: true
Attachment #476394 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #476395 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #476396 - Attachment is obsolete: true
Attachment #476396 - Flags: review?(mbrubeck)
(Assignee)

Updated

8 years ago
Attachment #476397 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #477721 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #477722 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #477723 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Attachment #477724 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #477725 - Attachment description: , part 5: Kill InputHandler object! → part 5: Kill InputHandler object!
Attachment #477725 - Flags: review+
(Assignee)

Updated

8 years ago
Attachment #477726 - Attachment description: , part 6: rename InputHandler file → part 6: rename InputHandler file
Attachment #477726 - Flags: review?(mark.finkle)
(Assignee)

Updated

8 years ago
Blocks: 597600
(Assignee)

Comment 29

8 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?
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
(Assignee)

Comment 31

8 years ago
Pushed http://hg.mozilla.org/mobile-browser/rev/b76ac56f3321
Status: NEW → RESOLVED
Last Resolved: 8 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.