Bug 174001

Summary:

Add API to get WKActivatedElementInfo

Product:

WebKit

Reporter:

Megan Gardner <megan_gardner>

Component:

New Bugs

Assignee:

Megan Gardner <megan_gardner>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bdakin, simon.fraser, thorton, wenson_hsieh

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	thorton: review+

Megan Gardner

Reported 2017-06-29 18:25:05 PDT

Add API to get WKActivatedElementInfo

Attachments
Patch (13.41 KB, patch) 2017-06-29 18:25 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (13.94 KB, patch) 2017-06-30 17:51 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (14.02 KB, patch) 2017-06-30 18:07 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (15.79 KB, patch) 2017-06-30 18:40 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (15.86 KB, patch) 2017-06-30 18:43 PDT, Megan Gardner	no flags	Details Formatted Diff Diff
Patch (15.86 KB, patch) 2017-06-30 18:46 PDT, Megan Gardner	thorton: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Megan Gardner

Comment 1 2017-06-29 18:25:42 PDT

Created attachment 314199 [details] Patch

Megan Gardner

Comment 2 2017-06-29 18:27:38 PDT

Sorry, forgot to add descriptions to the change logs, I'll do that in the morning.

Wenson Hsieh

Comment 3 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?

Tim Horton

Comment 4 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

Wenson Hsieh

Comment 5 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?

Tim Horton

Comment 6 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)

Wenson Hsieh

Comment 7 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:.

Wenson Hsieh

Comment 8 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

Wenson Hsieh

Comment 9 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)

Megan Gardner

Comment 10 2017-06-30 17:51:55 PDT

Created attachment 314339 [details] Patch

Simon Fraser (smfr)

Comment 11 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.

Simon Fraser (smfr)

Comment 12 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:

Tim Horton

Comment 13 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 :)

Megan Gardner

Comment 14 2017-06-30 18:07:14 PDT

Created attachment 314340 [details] Patch

Tim Horton

Comment 15 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.

Megan Gardner

Comment 16 2017-06-30 18:40:02 PDT

Created attachment 314346 [details] Patch

Tim Horton

Comment 17 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!

Megan Gardner

Comment 18 2017-06-30 18:43:34 PDT

Created attachment 314347 [details] Patch

Megan Gardner

Comment 19 2017-06-30 18:46:11 PDT

Created attachment 314348 [details] Patch

Tim Horton

Comment 20 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?

Megan Gardner

Comment 21 2017-06-30 19:26:37 PDT

https://trac.webkit.org/r219034

Megan Gardner

Comment 22 2017-06-30 20:09:07 PDT

https://trac.webkit.org/r219036 to fix Mac build

Note You need to log in before you can comment on or make changes to this bug.