922896 - Optimizations to remove 300ms touch > mouse events delay

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Patrick H. Lauke]

Attachment: Comparison screenshot of Firefox Android vs FirefoxOS handling non-scalable pages and optimizations to remove 300ms delay

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36

Steps to reproduce:

1) go to http://patrickhlauke.github.io/touch/tests/event-listener_user-scalable-no.html and tap the button
2) go to http://patrickhlauke.github.io/touch/tests/event-listener_minimum-maximum-scale.html and tap the button


Actual results:

in both cases, the ~300ms delay between the touch events and the simulated/fallback mouse events is present (generally present in touch-devices to wait for a potential second tap to initiate a double-tap-to-zoom)


Expected results:

as is already the case in Firefox for Android, the delay should not be necessary in situations where a page has been explicitly set not to scale/zoom - classic examples are having the viewport meta set to user-scalable=no or having the minimum and maximum scale set to the same value.

Comment 1

[Patrick H. Lauke]

worth noting that i'm filing this bug indirectly - don't have a real FirefoxOS device, but screenshots were kindly provided for me by a friend running 1.0.1.0 on a ZTE (which may not be the latest version)

Comment 2

[David Burns :automatedtester]

Confirmed with FxOS 1.3 so assuming that it is there for previous versions.

Gecko git sha is dbb784a

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Seems like this is probably a gecko issue, not gaia.  Kind of guessing at the component here.

Comment 4

[Matt Brubeck (:mbrubeck)]

This is mostly in the gfx/layers async pan/zoom code:
http://mxr.mozilla.org/mozilla-central/ident?i=MAX_TAP_TIME

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Yeah, it's in the APZC code. In Fennec the JavaPanZoomController handles this by firing the tap event early if it knows it doesn't need to wait for a double-tap because the page is not zoomable.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/JavaPanZoomController.java#1367

We should put similar code into the APZC.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Dale, would you mind taking this one since you're working on that code anyway?

Comment 7

[Dale Harvey (:daleharvey)]

Sure thing, cheers

Comment 8

[Dale Harvey (:daleharvey)]

Attachment: Fire single tap immediately if zooming disabled

Based on top of https://bugzilla.mozilla.org/show_bug.cgi?id=915645

Comment 9

[Dale Harvey (:daleharvey)]

This fixes the delay on the mouseover event being fired, I am still seeing a 300ms delay on mousemove only in http://patrickhlauke.github.io/touch/tests/event-listener_minimum-maximum-scale.html with this patch

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 814370 [details] [diff] [review]
Fire single tap immediately if zooming disabled

Review of attachment 814370 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like the approach of changing the gesture detector itself based on whether the APZC is zoomable. I would prefer mirroring the approach we use in Fennec, where APZC::OnSingleTapUp fires controller->HandleSingleTap if the page is not zoomable and APZC::OnSingleTapConfirmed fires it if it is zoomable.

Comment 11

[Dale Harvey (:daleharvey)]

Attachment: Fire tap immediately if content is not zoomable

That is cleaner

I wasnt sure about the code duplication / whether its worth splitting into a new function or not, can do that if needed

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 815595 [details] [diff] [review]
Fire tap immediately if content is not zoomable

Review of attachment 815595 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Fire tap immediately if content is not zoomable

Rebased, carrying r+, inbound is closed so will push when its reopened

Comment 14

[Kevin Grandon :kgrandon]

Vivien - Just wanted to bring this bug to your attention as I believe you were thinking of relying on the mouse delay for styling selected elements with :hover in FirefoxOS. I'm not sure how or if this will impact your plans though.

Comment 15

[Dale Harvey (:daleharvey)]

This only affects the delay after touchup, seems like :hover should apply on touchdown

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e5777ad39d

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d4e5777ad39d

Comment 17

[henryfhchan]

Shouldn't
line 717	  if (controller && mAllowZoom) {
read
line 717	  if (controller && !mAllowZoom) {
?

Comment 18

[Dale Harvey (:daleharvey)]

Apologies, screwed that up during the rebase, will fix now

Comment 19

[Dale Harvey (:daleharvey)]

Attachment: 922896-followup.patch

Follow up to fix error in rebase

Comment 20

[Dale Harvey (:daleharvey)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad7ca67b369b

Comment 21

[Mike Lee [:mlee]]

Dale, any reason we wouldn't want this for koi (1.2)?

Comment 22

[Dale Harvey (:daleharvey)]

Its not a bug fix and there is going to be a lot of changes and improvements in this area, I would prefer we only keep uplifts for 'must fixes' and try to complete old release asap, couldnt really make a call on this bug over the others though, https://bugzilla.mozilla.org/show_bug.cgi?id=915645 has leo nom and so will likely conflict if this isnt uploaded

Comment 23

[Kartikaya Gupta (email:[e-mail])]

The follow-up landed with the wrong bug number.

https://hg.mozilla.org/mozilla-central/rev/ad7ca67b369b

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Dale Harvey (:daleharvey) PTO - 28th Oct from comment #15)
> This only affects the delay after touchup, seems like :hover should apply on
> touchdown
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e5777ad39d

:hover per spec applies on mousemove iirc. I have introduced a delay to have :active working though but this stuff affects BrowserElementPanning.js which is a code path that is not taken by this code so it should be fine. But I bet that at some point we will have to introduce an artificial delay in this code as well in order to have :active working properly.