| Summary: | Long press on picture/link show menu obscured by keyboard | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | James Savage <james.savage> | ||||||
| Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, james.savage, megan_gardner, webkit-bug-importer, wenson_hsieh | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
James Savage
2018-08-29 16:23:44 PDT
Created attachment 348447 [details]
Patch
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 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 Created attachment 348579 [details]
Patch
Comment on attachment 348579 [details]
Patch
I like this better
Comment on attachment 348579 [details] Patch Clearing flags on attachment: 348579 Committed r235870: <https://trac.webkit.org/changeset/235870> All reviewed patches have been landed. Closing bug. |