Bug 107607

Summary: [Chromium, Mobile] Do not show disambiguation pop up in mobile sites
Product: WebKit Reporter: dfalcantara
Component: WebKit Misc.Assignee: dfalcantara
Status: RESOLVED FIXED    
Severity: Normal CC: aelias, dglazkov, klobag, trchen, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description dfalcantara 2013-01-22 17:56:28 PST
Corresponding Chromium bug:
http://crbug.com/168117

For mobile sites, it seems unnecessary to make a disambiguation popup since the page should be designed specifically for ease of use on a mobile device, e.g. touch targets should be big enough to click on without ambiguity.
Comment 1 dfalcantara 2013-01-22 18:18:11 PST
Created attachment 184097 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-22 19:30:32 PST
Comment on attachment 184097 [details]
Patch

Attachment 184097 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16039885

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment 3 Adam Barth 2013-01-23 11:29:55 PST
Comment on attachment 184097 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184097&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:758
> +        // Don't trigger a disambiguation popup on sites designed for mobile devices.
> +        // Instead, assume that the page has been designed with big enough buttons and links.
> +        ViewportArguments viewportArguments = mainFrameImpl()->frame()->document()->viewportArguments();
> +        bool isLikelyMobileSite = !viewportArguments.userZoom || (viewportArguments.width == ViewportArguments::ValueDeviceWidth);

Do we have other heuristics like this elsewhere?  Should we use the same one consistently?  If not, should we factor this code in a way that future code that wants a "is mobile site" heuristic can use it too?

Also, we need a test.
Comment 4 dfalcantara 2013-01-23 13:30:21 PST
> Do we have other heuristics like this elsewhere?  Should we use the same one consistently?  If not, should we factor this code in a way that future code that wants a "is mobile site" heuristic can use it too?
> 
> Also, we need a test.

I think I found the code you mentioned for the 300ms simulated click event delay Chromium-side, in ContentViewGestureHandler.java.  It seems pretty specific, though: because our double tap action is to zoom in, we simply don't enforce the trigger the delay when there is a fixed page scale.

I'll dig around some more for other current possible use-cases, but I think factoring out the code is a good idea, regardless.
Comment 5 dfalcantara 2013-01-23 16:46:46 PST
Created attachment 184346 [details]
Patch
Comment 6 dfalcantara 2013-01-24 11:43:18 PST
Created attachment 184542 [details]
Patch
Comment 7 dfalcantara 2013-01-24 11:44:07 PST
Alex: is this what you were going for?
Comment 8 Alexandre Elias 2013-01-24 11:54:12 PST
LGTM
Comment 9 dfalcantara 2013-01-28 13:04:29 PST
Ping?
Comment 10 Adam Barth 2013-01-28 15:15:26 PST
Comment on attachment 184542 [details]
Patch

Thanks.  This is much better.
Comment 11 WebKit Review Bot 2013-01-28 15:39:46 PST
Comment on attachment 184542 [details]
Patch

Clearing flags on attachment: 184542

Committed r141019: <http://trac.webkit.org/changeset/141019>
Comment 12 WebKit Review Bot 2013-01-28 15:39:50 PST
All reviewed patches have been landed.  Closing bug.