Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18850005:
Disable double tap zoom on mobile sites, to remove 300ms click delay (Closed)
|
Created:
7 years, 5 months ago by johnme Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Peter Beverloo Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisable double tap zoom on mobile sites, to remove 300ms click delay
This patch disables double tap zoom on pages that have a
width=device-width or narrower viewport (indicating that this is a
mobile-optimized or responsive website).
Double tap zoom continues to be disabled for pages that disallow the
user from zooming (even if they don't have a device-width or narrower
viewport).
This causes very noticeable performance benefits: links, buttons,
checkboxes, etc respond within around 10ms instead of 300ms.
Tested this on: http://jsbin.com/ixibol/6
Users shouldn't miss the double-tap gesture in the affected
situations, as double-tap means "zoom until tapped content is legible",
and in all the cases above the content was already legible (already
displayed at 1 or more DIPs per CSS pixel), so double tap zoom didn't
make sense and didn't do much.
BUG=169642
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232245
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address klobag's comment #Patch Set 3 : Rewrite, taking into account crrev.com/27043004 #
Total comments: 5
Patch Set 4 : Always disable, rather than enabling iff zoomed in by more than 10% #
Total comments: 3
Patch Set 5 : Fix typo #
Total comments: 4
Patch Set 6 : Addressed Ted's nits #Patch Set 7 : Rebase #
Messages
Total messages: 25 (0 generated)
On 2013/07/08 17:42:54, johnme wrote: I don't know the Clank Java code so I'll avoid commenting on the specific implementation details, but I want to say that I'm very supportive of this change in general. width=device-width seems like a good signal that double-tap to zoom is much less important than good response time.
This seems alright. Grace, what do you think?
I am fine with the direction. But do we really want to add/remove listener? https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:89: private boolean mDoubleTapZoomEnabled = true; This should be set by "mListener instanceof GestureDetector.OnDoubleTapListener" in initGestureDetectors.
klobag wrote: > I am fine with the direction. But do we really want to add/remove listener? The reason I did this is that if you click 2 or 3 times in a row, and double-tap zoom is disabled, it makes more sense to send 2 or 3 click events, rather than eating the 2nd click event (and hence sending 1 or 2 click events respectively). The main disadvantage of adding/removing the listener is that I'm currently calling resetGestureHandlers() to ensure a consistent state, but that might interrupt touch gestures that were in progress when the zoom change occurred and caused us to toggle double tap. Pinch zoom seems to work fine, but there may some edge cases I didn't notice. The alternative to adding/removing the listener would be to always use an OnDoubleTapListener, hence GestureDetector would always listen for double taps, but use the onDoubleTapEvent handler to receive touch up events that would otherwise get eaten, and translate those back into onSingleTapConfirmed events. This is probably do-able, but it feels a little more hacky/brittle, and there may remain weird quirks as a result of running the GestureDetector in double tap mode. A third option, might be to add/remove the listener as in the current patch, but rather than calling resetGestureHandlers, postpone changing the listener until the current gesture (if any) is complete. https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:89: private boolean mDoubleTapZoomEnabled = true; On 2013/07/10 18:26:21, klobag.chromium wrote: > This should be set by "mListener instanceof GestureDetector.OnDoubleTapListener" > in initGestureDetectors. Done (though that should always be true, as mListener is always initialized to be a GestureDetector.SimpleOnGestureListener). I also made setTestDependencies update this accordingly.
On 2013/07/10 19:19:16, johnme wrote: > klobag wrote: > > I am fine with the direction. But do we really want to add/remove listener? > > The reason I did this is that if you click 2 or 3 times in a row, and double-tap > zoom is disabled, it makes more sense to send 2 or 3 click events, rather than > eating the 2nd click event (and hence sending 1 or 2 click events respectively). > > The main disadvantage of adding/removing the listener is that I'm currently > calling resetGestureHandlers() to ensure a consistent state, but that might > interrupt touch gestures that were in progress when the zoom change occurred and > caused us to toggle double tap. Pinch zoom seems to work fine, but there may > some edge cases I didn't notice. > > The alternative to adding/removing the listener would be to always use an > OnDoubleTapListener, hence GestureDetector would always listen for double taps, > but use the onDoubleTapEvent handler to receive touch up events that would > otherwise get eaten, and translate those back into onSingleTapConfirmed events. > This is probably do-able, but it feels a little more hacky/brittle, and there > may remain weird quirks as a result of running the GestureDetector in double tap > mode. > When in the disable double tap mode, can we response to onSingleTapUp() for the first tap, and onDoubleTap() for the second tap? Also we should ignore onSingleTapConfirmed() in this mode. > A third option, might be to add/remove the listener as in the current patch, but > rather than calling resetGestureHandlers, postpone changing the listener until > the current gesture (if any) is complete. > > https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java > (right): > > https://codereview.chromium.org/18850005/diff/1/content/public/android/java/s... > content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:89: > private boolean mDoubleTapZoomEnabled = true; > On 2013/07/10 18:26:21, klobag.chromium wrote: > > This should be set by "mListener instanceof > GestureDetector.OnDoubleTapListener" > > in initGestureDetectors. > > Done (though that should always be true, as mListener is always initialized to > be a GestureDetector.SimpleOnGestureListener). > > I also made setTestDependencies update this accordingly.
Going through old patches in my review list. Should this be closed?
aelias wrote: > Going through old patches in my review list. Should this be closed? Nope, unfortunately I just didn't get enough 20% days to work on this. I've now updated the patch, which ended up being a near-total rewrite due to the changes made in crrev.com/27043004, but on the plus side the patch is now notably simpler (as it just has to change the HasFixedPageScale logic). PTAL :)
https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1479: mContentViewGestureHandler.updateShouldDisableDoubleTap(disable); So the user can double-tap to zoom out, but then double-tap is disabled again? I wonder if some might find that less than intuitive. I think my main issue here is with sites where text auto-sizing is active but we also have a scalable 'width=device-width' (particulary forums, Reddit, etc...) I find that there is a LOT of variance in the text sizes for different posts (even at the same nesting level), making double-tap drag very convenient... I realize the benefit of tap responsiveness is huge for such sites, but it's a shame to lose the feature. I assume that 'user-scalable-no' results in min=max for page scale, which is how the current optimization works when they don't explicitly set the min/max?
https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1479: mContentViewGestureHandler.updateShouldDisableDoubleTap(disable); On 2013/10/22 14:15:08, jdduke wrote: > > So the user can double-tap to zoom out, but then double-tap is disabled again? I > wonder if some might find that less than intuitive. Double-tap toggles between "overview zoom level" and "make the text I tapped on legible, or zoom to fit if it's e.g. an image". On pages with a width=device-width (or narrower) viewport, the size of a CSS px is always >= 1dp, hence the text is already legible (specifically, it's at least as legible as on desktop, which is our goal), and so double-tap zoom should have no effect. Furthermore, users generally don't expect double-tap zoom to have any effect on width=device-width sites, since the majority of them disable user zooming. (Of course it's never quite so simple, and in cases where double-tapping wouldn't otherwise do anything, Chrome for Android currently makes double-tap zoom in to 1.2x the overview zoom level. But this isn't particularly useful, so it should be fine to lose this behavior and have double-tap consistently do nothing on pages with mobile viewports.) The key point is, that double-tap means "zoom in until this text is legible", where legible means "scaled such that 1 CSS px >= 1 dp if the text hasn't been autosized, else zoom-to-fit the enclosing Text Autosizing cluster (i.e. you'll zoom in less, but that's compensated for by the font size having been made correspondingly larger)". > > I think my main issue here is with sites where text auto-sizing is active but we > also have a scalable 'width=device-width' (particulary forums, Reddit, etc...) I > find that there is a LOT of variance in the text sizes for different posts (even > at the same nesting level), making double-tap drag very convenient... I realize > the benefit of tap responsiveness is huge for such sites, but it's a shame to > lose the feature. Text Autosizing should never be active on width=device-width pages (since, as mentioned above, the text is already legible). (Currently, a nasty fudge factor causes us to apply between a *small* amount of autosizing on width=device-width pages -- http://crbug.com/252828 -- but skobes and pdr are actively working to remove that.) > > I assume that 'user-scalable-no' results in min=max for page scale, which is how > the current optimization works when they don't explicitly set the min/max? Yup.
https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1477: boolean disable = (contentWidthCss <= windowWidthDp && pageScale < 1.1f * minScale) I don't like the "pageScale < 1.1f * minScale" condition either. It seems complex. Without Blink changes, double-tap won't exclusively go back to overview mode, it might also zoom in on page elements. The 10% value also seems arbitrary. It seems bound to cause user and developer confusion, I'm not sure even I'll remember in a month when I can or cannot double-tap. I'd rather just have the 100% disable on these sites.
https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1477: boolean disable = (contentWidthCss <= windowWidthDp && pageScale < 1.1f * minScale) On 2013/10/22 23:43:26, aelias wrote: > I don't like the "pageScale < 1.1f * minScale" condition either. It seems > complex. Without Blink changes, double-tap won't exclusively go back to > overview mode, it might also zoom in on page elements. The 10% value also seems > arbitrary. It seems bound to cause user and developer confusion, I'm not sure > even I'll remember in a month when I can or cannot double-tap. I'd rather just > have the 100% disable on these sites. I'd have to agree on this, all or nothing will save a lot of headache here...
Removed the "pageScale < 1.1f * minScale" condition at aelias & jdduke's suggestion. https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/16001/content/public/android/ja... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1477: boolean disable = (contentWidthCss <= windowWidthDp && pageScale < 1.1f * minScale) On 2013/10/22 23:57:49, jdduke wrote: > On 2013/10/22 23:43:26, aelias wrote: > > I don't like the "pageScale < 1.1f * minScale" condition either. It seems > > complex. Without Blink changes, double-tap won't exclusively go back to > > overview mode, it might also zoom in on page elements. The 10% value also > seems > > arbitrary. It seems bound to cause user and developer confusion, I'm not sure > > even I'll remember in a month when I can or cannot double-tap. I'd rather > just > > have the 100% disable on these sites. > > I'd have to agree on this, all or nothing will save a lot of headache here... After thinking about this for a little more, I agree. Done.
lgtm
https://codereview.chromium.org/18850005/diff/106001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/18850005/diff/106001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:278: return mContentWidthCss <= windowWidthDp; is it a typo? windowWidthDip vs. windowWidthDp. https://codereview.chromium.org/18850005/diff/106001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:278: return mContentWidthCss <= windowWidthDp; Is it a typo? windowWidthDip vs windowWidthDp
Fixed typo. https://codereview.chromium.org/18850005/diff/106001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java (right): https://codereview.chromium.org/18850005/diff/106001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/RenderCoordinates.java:278: return mContentWidthCss <= windowWidthDp; On 2013/10/23 21:02:36, klobag.chromium wrote: > is it a typo? windowWidthDip vs. windowWidthDp. Done (oops!)
lgtm
Can we add a test somewhere that ensures double tap is disabled for narrow pages? https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1471: mRenderCoordinates.hasMobileViewport() || mRenderCoordinates.hasFixedPageScale()); +4 indent (and it looks like it will require a line wrap) https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1242: mShouldDisableDoubleTap; this looks like it should fit on the previous line.
also lgtm :-)
On 2013/10/24 19:52:56, Ted C wrote: > also lgtm :-) Are we still moving forward on this?
> Are we still moving forward on this? Yep, a weekend and sheriffing stint meant I wasn't able to look further into Ted's comment about adding tests until today. I've addressed Ted's nits. As for the test, I chatted to dominikg and he's working on a framework that sounds like the perfect way of testing this - it lets you write telemetry-based tests that inject events in a platform-independent way, and then they get converted into platform-specific events (e.g. sent through the top level Android GestureRecognizer etc). Only problem is, it'll take another 2 weeks or so till all the pieces of that test framework finish landing, which is after the m32 branch point. So it sounds like the best strategy might be to land the current patch as is, with the existing tests that cover the gesture recognizer behaviour but not the (relatively straightforward) details of how we detect a mobile viewport. Then once dominikg's framework is ready I'll add two higher level tests that track the single tap delay for mobile and non-mobile viewports, tracked by GASP alerts (like the other performance metrics). https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1471: mRenderCoordinates.hasMobileViewport() || mRenderCoordinates.hasFixedPageScale()); On 2013/10/24 19:52:44, Ted C wrote: > +4 indent (and it looks like it will require a line wrap) Done (still fit within 100 chars without line wrap). https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java (right): https://codereview.chromium.org/18850005/diff/146001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewGestureHandler.java:1242: mShouldDisableDoubleTap; On 2013/10/24 19:52:44, Ted C wrote: > this looks like it should fit on the previous line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/johnme@chromium.org/18850005/236001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/johnme@chromium.org/18850005/446001
Message was sent while issue was closed.
Change committed as 232245 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
