Bug 189114 – Long press on picture/link show menu obscured by keyboard

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[James Savage]

<rdar://problem/42446498>

[James Savage]

Created attachment 348447 [details]
Patch

[Megan Gardner]

Comment on attachment 348447 [details]
Patch

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

r=me, with comments.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:373
> +        CGRect visibleRect = CGRectNull;

See associated next comment.
I think this should be view.window.bounds (or equivalent) not CGRectNull.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:402
>  {

Probably rename this to something better if we make the associated changes.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:411
> +        visibleRect = view.window.bounds;

Could we move this logic to the calling function? I don't like having to do this compare here, if we just set the visible rect to the default before we even call into here, then we can make this cleaner.

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:414
> +

remove whitespace

> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:549
> +    CGRect visibleRect = CGRectNull;

Same as above. Should be view.window.bounds

[James Savage]

Comment on attachment 348447 [details]
Patch

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

Will come back with a v2. I think moving the free function to an instance method will address your concerns.

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:373
>> +        CGRect visibleRect = CGRectNull;
> 
> See associated next comment.
> I think this should be view.window.bounds (or equivalent) not CGRectNull.

I’m trying to keep the fallback value in one place. If you don’t like using CGRectNull as a sentinel, how about moving presentationStyleForView() to an instance method on WKActionSheetAssistant. That way it could make the delegate call and have the fallback, rather than callers providing it?

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:414
>> +
> 
> remove whitespace

This was already here (the diff got out of order), but I can remove it.

>> Source/WebKit/UIProcess/ios/WKActionSheetAssistant.mm:549
>> +    CGRect visibleRect = CGRectNull;
> 
> Same as above. Should be view.window.bounds

Ditto also :P

[James Savage]

Created attachment 348579 [details]
Patch

[Megan Gardner]

Comment on attachment 348579 [details]
Patch

I like this better

[WebKit Commit Bot]

Comment on attachment 348579 [details]
Patch

Clearing flags on attachment: 348579

Committed r235870: <https://trac.webkit.org/changeset/235870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.