Bug 129146 - WebKit2 View Gestures (Smart Magnification): Support for iOS
Summary: WebKit2 View Gestures (Smart Magnification): Support for iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-21 02:48 PST by Tim Horton
Modified: 2014-03-01 21:22 PST (History)
7 users (show)

See Also:


Attachments
patch (44.28 KB, patch)
2014-02-21 13:52 PST, Tim Horton
no flags Details | Formatted Diff | Diff
rebase (47.36 KB, patch)
2014-02-21 13:59 PST, Tim Horton
no flags Details | Formatted Diff | Diff
rebase (44.23 KB, patch)
2014-02-21 14:01 PST, Tim Horton
no flags Details | Formatted Diff | Diff
zomgrebase (44.33 KB, patch)
2014-02-21 14:03 PST, Tim Horton
no flags Details | Formatted Diff | Diff
new approach (52.84 KB, patch)
2014-02-26 12:17 PST, Tim Horton
no flags Details | Formatted Diff | Diff
fix mac build (53.77 KB, patch)
2014-02-26 14:07 PST, Tim Horton
benjamin: review-
Details | Formatted Diff | Diff
patch (56.46 KB, patch)
2014-02-26 17:26 PST, Tim Horton
no flags Details | Formatted Diff | Diff
patch (56.45 KB, patch)
2014-02-26 17:30 PST, Tim Horton
benjamin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-02-21 02:48:52 PST
<rdar://problem/16032668>
Comment 1 Tim Horton 2014-02-21 13:52:39 PST
Created attachment 224899 [details]
patch
Comment 2 Tim Horton 2014-02-21 13:59:52 PST
Created attachment 224902 [details]
rebase
Comment 3 Tim Horton 2014-02-21 14:01:07 PST
Created attachment 224903 [details]
rebase
Comment 4 Tim Horton 2014-02-21 14:03:23 PST
Created attachment 224904 [details]
zomgrebase
Comment 5 WebKit Commit Bot 2014-02-21 14:05:56 PST
Attachment 224904 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/mac/ViewGestureController.h:96:  The parameter name "scrollView" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Simon Fraser (smfr) 2014-02-24 19:54:20 PST
Comment on attachment 224904 [details]
zomgrebase

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

> Source/WebKit2/UIProcess/API/ios/WKContentView.mm:330
> +    [_interactionView _setDoubleTapGestureRecognizer:recognizer];

Nope!

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:137
> +    toImpl([_contentView _pageRef])->setShouldRecordNavigationSnapshots(allowsBackForwardNavigationGestures);

Space after [. This is now [_contentView page]->

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:262
> +    WebPageProxy *webPageProxy = toImpl([_contentView _pageRef]);

Changed.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:265
> +    UIGestureRecognizer *recognizer = _gestureController->installSmartMagnificationHandler(_contentView.get(), _scrollView.get());

Odd that we call it magnification not zoom.

> Source/WebKit2/UIProcess/mac/ViewGestureController.h:164
> +    RetainPtr<WKSmartMagnificationGestureController> m_smartMagnificationGestureController;
> +    RetainPtr<UIScrollView> m_smartMagnificationScrollView;
> +    RetainPtr<UIView> m_smartMagnificationTargetView;

Who owns the gesture controller? Are we creating ref cycles here?
Comment 7 Benjamin Poulain 2014-02-24 19:56:34 PST
Comment on attachment 224904 [details]
zomgrebase

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

I'd like to discuss if there is a way to untangle stuff if you have 5 minutes.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:175
> +#if PLATFORM(IOS)
> +    _gestureController = std::make_unique<WebKit::ViewGestureController>(*_page);
> +
> +    UIGestureRecognizer *recognizer = _gestureController->installSmartMagnificationHandler(_contentView.get(), _scrollView.get());
> +    [_contentView _setDoubleTapGestureRecognizer:recognizer];
> +#endif
> +

I don't think this is the right place. Why would the smart magnification know anything about the scrollview?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:475
> +

Blank line.

> Source/WebKit2/UIProcess/API/ios/WKContentView.mm:332
> +- (void)_setDoubleTapGestureRecognizer:(UIGestureRecognizer *)recognizer
> +{
> +    [_interactionView _setDoubleTapGestureRecognizer:recognizer];
> +}
> +

You can remove this, no _interactionView anymore :)

> Source/WebKit2/UIProcess/API/ios/WKContentViewInternal.h:79
> +- (void)_setDoubleTapGestureRecognizer:(UIGestureRecognizer *)recognizer;

This is the wrong header. WKContentViewInternal is/was for API used from bellow (WebPageProxy, PageClient, etc). WKContentView.h is the API for WKView/WKWebView.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:33
> -#import "WKContentView.h"
> +#import "WKContentViewInternal.h"

Nope, see above.
Comment 8 Tim Horton 2014-02-24 20:56:42 PST
(In reply to comment #7)
> (From update of attachment 224904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=224904&action=review
> 
> I'd like to discuss if there is a way to untangle stuff if you have 5 minutes.

I think we have a plan.
Comment 9 Benjamin Poulain 2014-02-25 15:12:11 PST
Something I forgot to mention, you will need to call [WKContentView willStartUserTriggeredZoom] before starting the animation. That way the future WebProcess scale update will not force the initial zoom on the page.
Comment 10 Tim Horton 2014-02-26 12:17:26 PST
Created attachment 225278 [details]
new approach
Comment 11 Tim Horton 2014-02-26 14:07:22 PST
Created attachment 225295 [details]
fix mac build
Comment 12 Benjamin Poulain 2014-02-26 15:22:16 PST
Comment on attachment 225295 [details]
fix mac build

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:325
> +- (RetainPtr<CGImageRef>)_takeViewSnapshot
> +{
> +    // FIXME: We should be able to use acquire an IOSurface directly, instead of going to CGImage here and back in ViewSnapshotStore.
> +    UIGraphicsBeginImageContextWithOptions(self.bounds.size, YES, self.window.screen.scale);
> +    [self drawViewHierarchyInRect:[self bounds] afterScreenUpdates:NO];
> +    UIImage *image = UIGraphicsGetImageFromCurrentImageContext();
> +    UIGraphicsEndImageContext();
> +    return image.CGImage;
> +}

Let's fix everything at once, otherwise it is just ugly.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:331
> +    const double maximumZoomDuration = 0.4;
> +    const double minimumZoomDuration = 0.1;
> +    const double zoomDurationFactor = 0.3;

I think Darin would prefer no const here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:341
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);

You could move this bellow, just before it is needed.
Shouldn't the type be CGSize or something?

I don't think the scrollview's bounds is ever correct when you want to show something. You should use the unobscuredRect.

Fun fact, the unobscuredRect can change in response to the zoom. This means the centering is not strictly correct. This is also wrong on WebKit1.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:343
> +    double currentScaleFactor = [_scrollView zoomScale];
> +    double relativeScale = scale / currentScaleFactor;

Shouldn't those be CGFloat?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:346
> +    WebCore::FloatRect targetRectInScrollView = [_scrollView convertRect:targetRect fromView:_contentView.get()];
> +    WebCore::FloatRect scaledTargetRectInScrollView = targetRectInScrollView;

CGRect?

I prefer the name to say in which coordinate system the rect is. For example, targetRectInScrollViewCoordinate.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:347
> +    scaledTargetRectInScrollView.scale(relativeScale, relativeScale);

targetRectInScrollViewAfterZoom or something like that?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:356
> +    if (scaledTargetRectInScrollView.width() > scrollViewSize.width())
> +        zoomCenter.setX(origin.x());
> +    if (scaledTargetRectInScrollView.height() > scrollViewSize.height())
> +        zoomCenter.setY(origin.y());

I think you should use the unobscured rect here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:365
> +    contentOffset = contentOffset.expandedTo(WebCore::FloatPoint());

???

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:373
> +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);
> +    WebCore::FloatSize contentSize([_scrollView contentSize]);
> +    WebCore::FloatPoint currentContentOffset([_scrollView contentOffset]);

I think you should use unobscuredRect for the scrollViewSize and currentContentOffset, and use WKContentView's size instead of [_scrollView contentSize] instead.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:376
> +    WebCore::FloatPoint originInScrollView([_scrollView convertPoint:origin fromView:_contentView.get()]);
> +    WebCore::FloatRect targetRectInScrollView([_scrollView convertRect:targetRect fromView:_contentView.get()]);

inScrollViewCoordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> +    float scrollDistance = (currentContentOffset - newContentOffset).diagonalLength();

float? :)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:420
> +    CGSize scrollViewSize = [_scrollView bounds].size;

[_scrollView bounds] is probably incorrect.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:452
> +    if (!layerTreeTransaction.scaleWasSetByUIProcess() && ![_scrollView isZooming] && ![_scrollView isZoomBouncing] && ![_scrollView _isAnimatingZoom])

Sorry!

> Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:50
> +static const CGFloat smartMagnificationPanScrollThresholdZoomedOut = 60;
> +static const CGFloat smartMagnificationPanScrollThresholdIPhone = 100;
> +static const CGFloat smartMagnificationPanScrollThresholdIPad = 150;
> +static const CGFloat smartMagnificationElementPadding = 0.05;
> +
> +static const double smartMagnificationMaximumScale = 1.6;
> +static const double smartMagnificationMinimumScale = 0;

CGFloat and double?
Comment 13 Tim Horton 2014-02-26 16:11:43 PST
(In reply to comment #12)
> (From update of attachment 225295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225295&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:365
> > +    contentOffset = contentOffset.expandedTo(WebCore::FloatPoint());
> 
> ???

This is basically point clamping. We do it in a bunch of places.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:373
> > +    WebCore::FloatSize scrollViewSize([_scrollView bounds].size);
> > +    WebCore::FloatSize contentSize([_scrollView contentSize]);
> > +    WebCore::FloatPoint currentContentOffset([_scrollView contentOffset]);
> 
> I think you should use unobscuredRect for the scrollViewSize and currentContentOffset, and use WKContentView's size instead of [_scrollView contentSize] instead.

Is the offset I send to setContentOffset the same as the origin of the unobscuredRect? I think not, I think it's the actual scroll view content offset. So I'll have to move the new content offset by the difference between unobscuredRect and scrollView contentOffset?

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> > +    float scrollDistance = (currentContentOffset - newContentOffset).diagonalLength();
> 
> float? :)

Is the return type of that function :)
Comment 14 Tim Horton 2014-02-26 17:26:30 PST
Created attachment 225328 [details]
patch
Comment 15 Tim Horton 2014-02-26 17:30:19 PST
Created attachment 225329 [details]
patch
Comment 16 Benjamin Poulain 2014-02-27 18:20:28 PST
Comment on attachment 225329 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:359
> +    [_contentView willStartUserTriggeredZoom];
> +    [_scrollView _zoomToCenter:point scale:scale duration:duration];

Should this check if "scale != [_scrollview zoomScale]"? If not at runtime, an assertion would be useful. 
Otherwise, if you call [WKContentView willStartUserTriggeredZoom] and stay at the initialScale, the scale will be wrong on load or rotation.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:370
> +    CGSize targetRectSizeInScrollViewCoordinatesAfterZoom = [_scrollView convertRect:targetRect fromView:_contentView.get()].size;
> +    targetRectSizeInScrollViewCoordinatesAfterZoom.width *= relativeScale;
> +    targetRectSizeInScrollViewCoordinatesAfterZoom.height *= relativeScale;

I think this is no longer good. Now all the other computation are done in content coordinates. I think this rect is in the wrong coordinate system.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:384
> +static WebCore::FloatPoint constrainContentOffset(WebCore::FloatPoint contentOffset, WebCore::FloatSize contentSize, WebCore::FloatSize scrollViewSize)

Maybe rename scrollViewSize to unobscuredSomethingSomething?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:399
> +    WebCore::FloatPoint originInScrollViewCoordinates([_scrollView convertPoint:origin fromView:_contentView.get()]);
> +    WebCore::FloatRect targetRectInScrollViewCoordinates([_scrollView convertRect:targetRect fromView:_contentView.get()]);

I think this may be incorrect too now. Previously you were comparing to scrollview size, which is why you needed this conversion.
Now you are comparing to unobscuredContentRect, which is content coordinate.

You can either get unobscuredRect in WKWebView coordinates and compare to that, or do the centering in content coordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:442
> +    if (![_scrollView isScrollEnabled] || ![_scrollView isZoomEnabled])
> +        return false;

Not sure we need this, we don't support it anywhere else.

We can add them everywhere if that becomes needed.

> Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:231
> +    _contentView = adoptNS([[WKContentView alloc] initWithFrame:bounds context:*toImpl(contextRef) configuration:std::move(webPageConfiguration) webView:nullptr]);

ahahahahah

> Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:79
> +    if (frameHandlesMagnificationGesture)
> +        return;

Why does the WebProcess call us back if frameHandlesMagnificationGesture is false? Shouldn't it just ignore the handleSmartMagnificationGesture()?

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:83
> +    HitTestResult hitTestResult = HitTestResult(originInContentsSpace);

Do we need the fancy iOS hit testing here? No clue what WebKit1 does.

> Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:87
> +    if (Node *node = hitTestResult.innerNode()) {

Node*
Maybe const too?
Comment 17 Tim Horton 2014-02-27 18:25:54 PST
Thanks, Ben! I'll fix things + do more testing.

(In reply to comment #16)
> (From update of attachment 225329 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225329&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:359
> > +    [_contentView willStartUserTriggeredZoom];
> > +    [_scrollView _zoomToCenter:point scale:scale duration:duration];
> 
> Should this check if "scale != [_scrollview zoomScale]"? If not at runtime, an assertion would be useful. 
> Otherwise, if you call [WKContentView willStartUserTriggeredZoom] and stay at the initialScale, the scale will be wrong on load or rotation.

Oh! Interesting. I'll do something like that.

> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:442
> > +    if (![_scrollView isScrollEnabled] || ![_scrollView isZoomEnabled])
> > +        return false;
> 
> Not sure we need this, we don't support it anywhere else.
> 
> We can add them everywhere if that becomes needed.

OK, that's fine. I was just matching WK1 (ish).

> > Source/WebKit2/UIProcess/API/ios/WKViewIOS.mm:231
> > +    _contentView = adoptNS([[WKContentView alloc] initWithFrame:bounds context:*toImpl(contextRef) configuration:std::move(webPageConfiguration) webView:nullptr]);
> 
> ahahahahah

Yeah :|

> > Source/WebKit2/UIProcess/ios/SmartMagnificationController.mm:79
> > +    if (frameHandlesMagnificationGesture)
> > +        return;
> 
> Why does the WebProcess call us back if frameHandlesMagnificationGesture is false? Shouldn't it just ignore the handleSmartMagnificationGesture()?

That's a pretty good idea :D

> > Source/WebKit2/WebProcess/WebPage/ViewGestureGeometryCollector.cpp:83
> > +    HitTestResult hitTestResult = HitTestResult(originInContentsSpace);
> 
> Do we need the fancy iOS hit testing here? No clue what WebKit1 does.

I believe this is equivalent to what WebKit1 does.
Comment 18 Tim Horton 2014-03-01 21:22:42 PST
http://trac.webkit.org/changeset/164937