Optimizations to remove 300ms touch > mouse events delay

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: redux, Assigned: daleharvey)

Tracking

({perf})

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c= p= s= u=])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 812889 [details]
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.

Updated

5 years ago
Keywords: perf
(Reporter)

Comment 1

5 years ago
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)
Confirmed with FxOS 1.3 so assuming that it is there for previous versions.

Gecko git sha is dbb784a
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [c= p= s= u=]
Seems like this is probably a gecko issue, not gaia.  Kind of guessing at the component here.
Component: General → DOM: Events
Product: Boot2Gecko → Core
This is mostly in the gfx/layers async pan/zoom code:
http://mxr.mozilla.org/mozilla-central/ident?i=MAX_TAP_TIME
Component: DOM: Events → Graphics: Layers
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.
Dale, would you mind taking this one since you're working on that code anyway?
Flags: needinfo?(dale)
(Assignee)

Comment 7

5 years ago
Sure thing, cheers
Assignee: nobody → dale
Flags: needinfo?(dale)
(Assignee)

Comment 8

5 years ago
Created attachment 814370 [details] [diff] [review]
Fire single tap immediately if zooming disabled

Based on top of https://bugzilla.mozilla.org/show_bug.cgi?id=915645
Attachment #814370 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 9

5 years ago
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 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.
Attachment #814370 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 11

5 years ago
Created attachment 815595 [details] [diff] [review]
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
Attachment #814370 - Attachment is obsolete: true
Attachment #815595 - Flags: review?(bugmail.mozilla)
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!
Attachment #815595 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 816405 [details] [diff] [review]
Fire tap immediately if content is not zoomable

Rebased, carrying r+, inbound is closed so will push when its reopened
Attachment #815595 - Attachment is obsolete: true
Attachment #816405 - Flags: review+
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.
Flags: needinfo?(21)
(Assignee)

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4e5777ad39d
https://hg.mozilla.org/mozilla-central/rev/d4e5777ad39d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 17

5 years ago
Shouldn't
line 717	  if (controller && mAllowZoom) {
read
line 717	  if (controller && !mAllowZoom) {
?
(Assignee)

Comment 18

5 years ago
Apologies, screwed that up during the rebase, will fix now
(Assignee)

Comment 19

5 years ago
Created attachment 816679 [details] [diff] [review]
922896-followup.patch

Follow up to fix error in rebase

Comment 21

5 years ago
Dale, any reason we wouldn't want this for koi (1.2)?
Flags: needinfo?(dale)
Whiteboard: [c= p= s= u=] → [c= p= s=2013.10.18 u=]
(Assignee)

Comment 22

5 years ago
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
Flags: needinfo?(dale)

Updated

5 years ago
Whiteboard: [c= p= s=2013.10.18 u=] → [c= p= s= u=]
(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.
Flags: needinfo?(21)
Blocks: 944082
You need to log in before you can comment on or make changes to this bug.