Bug 174001

Summary: Add API to get WKActivatedElementInfo
Product: WebKit Reporter: Megan Gardner <megan_gardner>
Component: New BugsAssignee: Megan Gardner <megan_gardner>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, simon.fraser, thorton, wenson_hsieh
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch thorton: review+

Description Megan Gardner 2017-06-29 18:25:05 PDT
Add API to get WKActivatedElementInfo
Comment 1 Megan Gardner 2017-06-29 18:25:42 PDT
Created attachment 314199 [details]
Patch
Comment 2 Megan Gardner 2017-06-29 18:27:38 PDT
Sorry, forgot to add descriptions to the change logs, I'll do that in the morning.
Comment 3 Wenson Hsieh 2017-06-29 18:36:20 PDT
Comment on attachment 314199 [details]
Patch

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

> Source/WebKit2/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174001

Please include a rdar link.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5256
> +    WebKit::InteractionInformationRequest infoRequest = WebKit::InteractionInformationRequest(WebCore::roundedIntPoint(position));

I don't think we need the WebKit:: namespacing here. Also, auto infoRequest would look a bit cleaner.

> Tools/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=174001

Please include a rdar link here too.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:40
> +TEST(WebKit2, WKReqestActivatedElementInfo)

These are very similar to the PositionInformationTests formerly used for drag and drop. There are a couple of scenarios there that I think we should also test here, such as calling requestActivatedElementAtPosition: from within a requestActivatedElementAtPosition: block. But I think for this patch, it should be OK (we can work on a followup that moves PositionInformationTests to this file).

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:44
> +    [webView _test_waitForDidFinishNavigation];

This can be shortened to use -synchronouslyLoadTestPageNamed:

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:52
> +        EXPECT_TRUE(elementInfo.image != nil);

Could we also (or instead) check the size of the image here?
Comment 4 Tim Horton 2017-06-29 18:36:47 PDT
Comment on attachment 314199 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5260
> +        capturedBlock([_WKActivatedElementInfo infoWithType:_WKActivatedElementTypeLink withInteractionInformationAtPosition:information]);

Now's probably time to think about what we want to do with the Type here. Maybe Wenson will have an idea :)

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:363
>  - (void)_requestDraggableElementAtPosition:(CGPoint)position completionBlock:(void (^)(_WKDraggableElementInfo *))block WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

Is this one really WK_MAC_TBA??? Isn't this TARGET_OS_IPHONE code???

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:365
> +- (void)requestActivatedElementAtPosition:(CGPoint)position completionBlock:(void (^)(_WKActivatedElementInfo *))block;

This need a leading underscore. This needs availability annotations.

> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:38
> +- (instancetype)infoWithType:(_WKActivatedElementType)type withInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;

The instance method should be initWithType, not infoWithType. They should both have leading underscores. Also, do you need both?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Time Traveller!

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:40
> +TEST(WebKit2, WKReqestActivatedElementInfo)

Request. Also, drop the WK prefix

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:43
> +    [webView loadHTMLString:@"<html><body margin: 0px; padding: 0px; ><a  href='testURL.test' style='display: block; height: 100%;' title='HitTestLinkTitle' id='testID'></a></body></html>" baseURL:nil];

You have two spaces before the href

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:47
> +    [webView requestActivatedElementAtPosition:CGPointMake(50, 50) completionBlock: ^(_WKActivatedElementInfo * elementInfo) {

no space after the star
Comment 5 Wenson Hsieh 2017-06-29 18:42:40 PDT
Comment on attachment 314199 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5260
>> +        capturedBlock([_WKActivatedElementInfo infoWithType:_WKActivatedElementTypeLink withInteractionInformationAtPosition:information]);
> 
> Now's probably time to think about what we want to do with the Type here. Maybe Wenson will have an idea :)

I think always passing in _WKActivatedElementTypeLink would be misleading. It seems possible to derive the _WKActivatedElementType from position information though, unless there are corner cases I might be missing?
Comment 6 Tim Horton 2017-06-29 18:43:53 PDT
(In reply to Wenson Hsieh from comment #5)
> Comment on attachment 314199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314199&action=review
> 
> >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5260
> >> +        capturedBlock([_WKActivatedElementInfo infoWithType:_WKActivatedElementTypeLink withInteractionInformationAtPosition:information]);
> > 
> > Now's probably time to think about what we want to do with the Type here. Maybe Wenson will have an idea :)
> 
> I think always passing in _WKActivatedElementTypeLink would be misleading.
> It seems possible to derive the _WKActivatedElementType from position
> information though, unless there are corner cases I might be missing?

Probably, we just need to think about what we want to say (esp. in the case of an image-that-is-a-link and whatnot)
Comment 7 Wenson Hsieh 2017-06-29 18:59:26 PDT
Comment on attachment 314199 [details]
Patch

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

>>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5260
>>>> +        capturedBlock([_WKActivatedElementInfo infoWithType:_WKActivatedElementTypeLink withInteractionInformationAtPosition:information]);
>>> 
>>> Now's probably time to think about what we want to do with the Type here. Maybe Wenson will have an idea :)
>> 
>> I think always passing in _WKActivatedElementTypeLink would be misleading. It seems possible to derive the _WKActivatedElementType from position information though, unless there are corner cases I might be missing?
> 
> Probably, we just need to think about what we want to say (esp. in the case of an image-that-is-a-link and whatnot)

I think we should match the order of precedence for determining the _WKActivatedElementType for position information that already exists in -_dataForPreviewItemController:atPosition:type:. And refactor that out into a static helper function like activatedElementTypeForPositionInformation that we can use from both places.

As you mentioned on IRC, this would ideally go in a +infoWithInteractionInformationAtPosition: initializer, and we wouldn't be passing in the type when creating these things anymore, but that might require a bit of late-stage refactoring of -_dataForPreviewItemController:atPosition:type:.
Comment 8 Wenson Hsieh 2017-06-29 19:01:11 PDT
Comment on attachment 314199 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5256
>> +    WebKit::InteractionInformationRequest infoRequest = WebKit::InteractionInformationRequest(WebCore::roundedIntPoint(position));
> 
> I don't think we need the WebKit:: namespacing here. Also, auto infoRequest would look a bit cleaner.

Also, we don't need WebCore:: before roundedIntPoint
Comment 9 Wenson Hsieh 2017-06-29 19:06:14 PDT
Comment on attachment 314199 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:363
>>  - (void)_requestDraggableElementAtPosition:(CGPoint)position completionBlock:(void (^)(_WKDraggableElementInfo *))block WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Is this one really WK_MAC_TBA??? Isn't this TARGET_OS_IPHONE code???

We could conceivably make this work on macOS too. Though, I think I'm going to just remove this entirely in the near future 🙃 (and fold PositionInformationTests into the new WKRequestActivatedElementInfo tests)
Comment 10 Megan Gardner 2017-06-30 17:51:55 PDT
Created attachment 314339 [details]
Patch
Comment 11 Simon Fraser (smfr) 2017-06-30 17:56:28 PDT
Comment on attachment 314339 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:37
> ++ (instancetype)_initWithInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;

Class methods should not start with 'init'. This should be +ActivatedElementInfoAtPosition:

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:47
> +    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:CGRectMake(0, 0, 320, 500)]);
> +    [webView loadHTMLString:@"<html><head><meta name='viewport' content='initial-scale=1'></head><body style = 'margin: 0px;'><a href='testURL.test' style='display: block; height: 100%;' title='HitTestLinkTitle' id='testID'></a></body></html>" baseURL:nil];
> +    [webView _test_waitForDidFinishNavigation];
> +    
> +    __block bool finished = false;
> +    [webView _requestActivatedElementAtPosition:CGPointMake(50, 50) completionBlock: ^(_WKActivatedElementInfo *elementInfo) {

This test would be more thorough if the content were scaled and zoomed.
Comment 12 Simon Fraser (smfr) 2017-06-30 17:57:35 PDT
(In reply to Simon Fraser (smfr) from comment #11)
> Comment on attachment 314339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314339&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:37
> > ++ (instancetype)_initWithInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;
> 
> Class methods should not start with 'init'. This should be
> +ActivatedElementInfoAtPosition:

+activatedElementInfoAtPosition:
Comment 13 Tim Horton 2017-06-30 17:58:03 PDT
Comment on attachment 314339 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfoInternal.h:37
>> ++ (instancetype)_initWithInteractionInformationAtPosition:(const WebKit::InteractionInformationAtPosition&)information;
> 
> Class methods should not start with 'init'. This should be +ActivatedElementInfoAtPosition:

Indeed, the previous review comment was explicit about which one to change but was applied overzealously :)
Comment 14 Megan Gardner 2017-06-30 18:07:14 PDT
Created attachment 314340 [details]
Patch
Comment 15 Tim Horton 2017-06-30 18:15:16 PDT
Comment on attachment 314340 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Add API to get WKActivatedElementInfo

SPI

> Source/WebKit2/ChangeLog:7
> +        Adding a way to get a WKActivatedElementInfo for a point on a WebView.

WKWebView or "web view". WebView is the WebKitLegacy API.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:5257
> +    infoRequest.includeSnapshot = true;

In some more advanced version of the API the client would be able to specify whether they want this snapshot. Since this is SPI and we know the client wants the snapshot, this is fine for now, but keep it in mind!

> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:57
> ++ (instancetype)activatedElementInfoAtPosition:(const WebKit::InteractionInformationAtPosition&)information

I don't know what you've done with this name (I blame smfr, and me for not reading his suggestion better), it sounds like it would take a position, but it takes an InteractionInformationAtPosition.

Really, InteractionInformationAtPosition is a terribly named thing, but that's a separate problem.

I propose

+ (instancetype)activatedElementInfoWithInteractionInformationAtPosition:(...)

> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfo.mm:77
> +        _type = _WKActivatedElementTypeLink;

What if it's not a link??? What happens if you just have a <div>? Better add more tests :) And probably a new value to the enum.
Comment 16 Megan Gardner 2017-06-30 18:40:02 PDT
Created attachment 314346 [details]
Patch
Comment 17 Tim Horton 2017-06-30 18:43:26 PDT
Comment on attachment 314346 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKActivatedElementInfo.h:37
> +    _WKActivatedElementTypeUnspecified WK_API_AVAILABLE(macosx(10.13), ios(11.0)),

This has to go at the end.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:67
> +TEST(WebKit2, ReqestActivatedElementInfoForNothing)

It's not nothing :) (it wouldn't have a bounding rect in that case) It's just not one of those other things!
Comment 18 Megan Gardner 2017-06-30 18:43:34 PDT
Created attachment 314347 [details]
Patch
Comment 19 Megan Gardner 2017-06-30 18:46:11 PDT
Created attachment 314348 [details]
Patch
Comment 20 Tim Horton 2017-06-30 19:04:17 PDT
Comment on attachment 314348 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:46
> +    webView.get().scrollView.contentOffset = CGPointMake(0, 50);
> +    webView.get().scrollView.zoomScale = 2.0;

I think smfr meant to make it so that the test would only pass if your hit testing was taking zoom/scrolling into account, but since your element is 100%x100%, it will pass regardless. That could be a separate test (and move these setters there). Doesn't necessarily need to happen with this patch, but should happen.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:52
> +        EXPECT_TRUE([elementInfo.URL.absoluteString isEqual:@"testURL.test"]);

Whatever happened to EXPECT_WK_STREQ?

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/WKRequestActivatedElementInfo.mm:55
> +        EXPECT_TRUE(elementInfo.image != nil);

EXPECT_NOT_NULL?
Comment 21 Megan Gardner 2017-06-30 19:26:37 PDT
https://trac.webkit.org/r219034
Comment 22 Megan Gardner 2017-06-30 20:09:07 PDT
https://trac.webkit.org/r219036
to fix Mac build