Bug 150693

Summary: Tapping and holding a link should have a share option
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, commit-queue, enrica, mitz, thorton
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
thorton: review+
Follow-up fix for non-ascii URLs mitz: review+

Description Beth Dakin 2015-10-29 14:36:30 PDT
Tapping and holding a link should have a share option

rdar://problem/21319702
Comment 1 Beth Dakin 2015-10-29 14:39:12 PDT
Created attachment 264347 [details]
Patch
Comment 2 WebKit Commit Bot 2015-10-29 14:40:57 PDT
Attachment 264347 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.mm:132:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Beth Dakin 2015-10-30 11:58:09 PDT
Created attachment 264406 [details]
Patch
Comment 4 WebKit Commit Bot 2015-10-30 11:59:45 PDT
Attachment 264406 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.mm:132:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Tim Horton 2015-10-30 12:02:29 PDT
Comment on attachment 264406 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/_WKElementAction.mm:133
> +            NSLog(@"actionInfo._interactionLocation for share item is %@", NSStringFromPoint(actionInfo._interactionLocation));

Please no.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:726
> +    if ([_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)])

Wonder if we still need all these respondsToSelector checks these days.

> Source/WebKit2/UIProcess/ios/WKPDFView.mm:727
> +        [_webSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];

url.absoluteString (and above too)
Comment 6 Enrica Casucci 2015-10-30 12:31:11 PDT
Comment on attachment 264406 [details]
Patch

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

>> Source/WebKit2/UIProcess/ios/WKPDFView.mm:726
>> +    if ([_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)])
> 
> Wonder if we still need all these respondsToSelector checks these days.

I don't think we do need the responds to selector here.
Comment 7 Beth Dakin 2015-10-30 12:40:54 PDT
(In reply to comment #6)
> Comment on attachment 264406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264406&action=review
> 
> >> Source/WebKit2/UIProcess/ios/WKPDFView.mm:726
> >> +    if ([_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)])
> > 
> > Wonder if we still need all these respondsToSelector checks these days.
> 
> I don't think we do need the responds to selector here.

Thank you Enrica and Tim! 

Checked in http://trac.webkit.org/changeset/191805
Comment 8 mitz 2015-10-30 18:02:14 PDT
Comment on attachment 264406 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3388
> +        [_textSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];
> +    else if (_webSelectionAssistant && [_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)])
> +        [_webSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];

Do these strings get interpreted as URLs by some code on the other side of this API, or do they end up being presented to the user? If it’s the latter, then we shouldn’t use -absoluteString, because it doesn’t give a good representation of certain URLs, such as ones with non-ASCII characters in the host or the path. Instead, -_web_userVisibleString should work. If these strings do get turned back into URLs, and we can’t use an alternative API that takes NSURLs, then we should consider using -_web_originalDataAsString here and the reverse transformation on the receiving end.

>> Source/WebKit2/UIProcess/ios/WKPDFView.mm:727
>> +        [_webSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];
> 
> url.absoluteString (and above too)

Same comment here.
Comment 9 Beth Dakin 2015-11-02 08:53:00 PST
(In reply to comment #8)
> Comment on attachment 264406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264406&action=review
> 
> > Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3388
> > +        [_textSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];
> > +    else if (_webSelectionAssistant && [_webSelectionAssistant respondsToSelector:@selector(showShareSheetFor:fromRect:)])
> > +        [_webSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];
> 
> Do these strings get interpreted as URLs by some code on the other side of
> this API, or do they end up being presented to the user? If it’s the latter,
> then we shouldn’t use -absoluteString, because it doesn’t give a good
> representation of certain URLs, such as ones with non-ASCII characters in
> the host or the path. Instead, -_web_userVisibleString should work. If these
> strings do get turned back into URLs, and we can’t use an alternative API
> that takes NSURLs, then we should consider using -_web_originalDataAsString
> here and the reverse transformation on the receiving end.
> 
> >> Source/WebKit2/UIProcess/ios/WKPDFView.mm:727
> >> +        [_webSelectionAssistant showShareSheetFor:[url absoluteString] fromRect:boundingRect];
> > 
> > url.absoluteString (and above too)
> 
> Same comment here.

Thank you Dan! This is presented to the user, and I was able to see the bug you referred to with non-ascii URLs. I will attach a follow-up patch.
Comment 10 Beth Dakin 2015-11-02 10:04:53 PST
Created attachment 264597 [details]
Follow-up fix for non-ascii URLs
Comment 11 Beth Dakin 2015-11-02 10:26:21 PST
Thanks, Dan! 

http://trac.webkit.org/changeset/191894