Bug 150693 – Tapping and holding a link should have a share option

• Component: WebKit2
• Status: RESOLVED FIXED

[Beth Dakin]

Tapping and holding a link should have a share option

rdar://problem/21319702

[Beth Dakin]

Created attachment 264347 [details]
Patch

[WebKit Commit Bot]

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.

[Beth Dakin]

Created attachment 264406 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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)

[Enrica Casucci]

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.

[Beth Dakin]

(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 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.

[Beth Dakin]

(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.

[Beth Dakin]

Created attachment 264597 [details]
Follow-up fix for non-ascii URLs

[Beth Dakin]

Thanks, Dan! 

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