Bug 154318 - Allow double tap to zoom in fast-click pages
Summary: Allow double tap to zoom in fast-click pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified iOS 9.3
: P2 Normal
Assignee: Dean Jackson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-02-16 17:34 PST by Jon Lee
Modified: 2016-02-16 19:27 PST (History)
3 users (show)

See Also:


Attachments
Patch (13.37 KB, patch)
2016-02-16 17:55 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (13.10 KB, patch)
2016-02-16 18:17 PST, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (13.97 KB, patch)
2016-02-16 18:45 PST, Dean Jackson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2016-02-16 17:34:23 PST
rdar://problem/24223767
Comment 1 Dean Jackson 2016-02-16 17:55:13 PST
Created attachment 271514 [details]
Patch
Comment 2 Simon Fraser (smfr) 2016-02-16 17:57:18 PST
Comment on attachment 271514 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941
> +    [_fastDoubleTapGestureRecognizer setEnabled:NO];
> +    [_fastDoubleTapGestureRecognizer setEnabled:YES];

Really?
Comment 3 Simon Fraser (smfr) 2016-02-16 18:04:39 PST
Comment on attachment 271514 [details]
Patch

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

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941
>> +    [_fastDoubleTapGestureRecognizer setEnabled:YES];
> 
> Really?

Maybe add a comment to make it clear this isn't a typo.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1342
> +- (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point

Method name doesn't obviously read as a setter (it sounds like a getter), and doesn't imply that things like data detectors will kick in. It needs to sound more imperative.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1405
> +- (void)_fastDoubleTapRecognized:(UITapGestureRecognizer *)gestureRecognizer

"fast double tap" makes it sound like there's another kind of double tap. is it more like "interruptible double tap"?
Comment 4 Benjamin Poulain 2016-02-16 18:17:47 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=271514&action=review

Some comments.
I am not sure to understand this patch. I'll have to talk to Jon.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.h:158
> +    BOOL _isDoubleTapPending;

This must be reset in [-cleanupInteraction] or [-setupInteraction]. 

Otherwise, you may keep a bogus state after a crash.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1353
> +    if (!_isDoubleTapPending)
> +        return;
> +
> +    _smartMagnificationController->handleSmartMagnificationGesture(_lastInteractionLocation);
> +    _isDoubleTapPending = NO;

Is it expected to show the data detector AND zoom?
I *think* zooming dismisses some of the pop overs.

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1403
> +- (void)_resetIsDoubleTapPending
> +{
> +    _isDoubleTapPending = NO;
> +}

Looks like adding complexity. All the lines with [self _resetIsDoubleTapPending] could be "_isDoubleTapPending = NO;".

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1407
> +    _lastInteractionLocation = gestureRecognizer.location;

This is a bit odd. As far as I know, "lastInteractionLocation" previously always referred to taps/clicks.
Comment 5 Dean Jackson 2016-02-16 18:17:58 PST
Created attachment 271515 [details]
Patch
Comment 6 Dean Jackson 2016-02-16 18:21:33 PST
Comment on attachment 271514 [details]
Patch

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

>>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:941
>>> +    [_fastDoubleTapGestureRecognizer setEnabled:YES];
>> 
>> Really?
> 
> Maybe add a comment to make it clear this isn't a typo.

Turns out I can remove this.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1342
>> +- (void)_didNotHandleTapAsClick:(const WebCore::IntPoint&)point
> 
> Method name doesn't obviously read as a setter (it sounds like a getter), and doesn't imply that things like data detectors will kick in. It needs to sound more imperative.

This is an existing name that signals an incoming message from the Web Process. It's an event handler, like _singleTapRecognized, which can also do a number of things.

>> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1405
>> +- (void)_fastDoubleTapRecognized:(UITapGestureRecognizer *)gestureRecognizer
> 
> "fast double tap" makes it sound like there's another kind of double tap. is it more like "interruptible double tap"?

I definitely agree here. It actually means the double tap that is recognized when we're in fast click mode.
Comment 7 Dean Jackson 2016-02-16 18:22:09 PST
Comment on attachment 271515 [details]
Patch

smfr gave r+ to the first patch. This one is better.
Comment 8 Jon Lee 2016-02-16 18:26:24 PST
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:1407
> > +    _lastInteractionLocation = gestureRecognizer.location;
> 
> This is a bit odd. As far as I know, "lastInteractionLocation" previously
> always referred to taps/clicks.

The other double tap recognizer does the same.
Comment 9 Dean Jackson 2016-02-16 18:45:10 PST
Created attachment 271517 [details]
Patch
Comment 10 Dean Jackson 2016-02-16 19:27:26 PST
Committed r196679: <http://trac.webkit.org/changeset/196679>