Add API to get WKActivatedElementInfo
Created attachment 314199 [details] Patch
Sorry, forgot to add descriptions to the change logs, I'll do that in the morning.
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 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 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?
(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 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 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 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)
Created attachment 314339 [details] Patch
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.
(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 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 :)
Created attachment 314340 [details] Patch
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.
Created attachment 314346 [details] Patch
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!
Created attachment 314347 [details] Patch
Created attachment 314348 [details] Patch
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?
https://trac.webkit.org/r219034
https://trac.webkit.org/r219036 to fix Mac build