Bug 238672

Summary: Sharing services picker menu Mail presents in the wrong place when compose web view is scrolled
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: New BugsAssignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, cdumez, esprehn+autocc, ews-watchlist, hi, kangil.han, megan_gardner, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kate Cheney
Reported 2022-04-01 11:03:44 PDT
Sharing services picker menu Mail presents in the wrong place when compose web view is scrolled
Attachments
Patch (3.63 KB, patch)
2022-04-01 11:05 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (2.10 KB, patch)
2022-04-08 10:54 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (2.04 KB, patch)
2022-04-08 12:10 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kate Cheney
Comment 1 2022-04-01 11:05:26 PDT
Created attachment 456373 [details] Patch
Kate Cheney
Comment 2 2022-04-01 11:05:44 PDT
rdar://88966924
Wenson Hsieh
Comment 3 2022-04-01 11:33:45 PDT
Comment on attachment 456373 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456373&action=review > Source/WebCore/dom/mac/ImageControlsMac.cpp:149 > + ContextMenuController& controller = page->contextMenuController(); Minor nit - I think we prefer `auto&` here. > Source/WebCore/dom/mac/ImageControlsMac.cpp:150 > + Frame* frame = controller.hitTestResult().innerNodeFrame(); I think it would be safer to use RefPtr here, since it's possible for `WebPage::handleImageServiceClick` to trigger a style update. > Source/WebCore/dom/mac/ImageControlsMac.cpp:154 > + FrameView* view = frame->view(); (Ditto)
Kate Cheney
Comment 4 2022-04-08 10:54:52 PDT
Created attachment 457095 [details] Patch
Wenson Hsieh
Comment 5 2022-04-08 12:03:47 PDT
Comment on attachment 457095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457095&action=review > Source/WebCore/dom/mac/ImageControlsMac.cpp:149 > + auto& element = downcast<Element>(node); Let's use RefPtr here. > Source/WebCore/dom/mac/ImageControlsMac.cpp:159 > + auto rect = view->convertToContainingWindow(renderer->absoluteBoundingBoxRect()); > + auto point = view->contentsToWindow(rect).minXMaxYCorner(); This seems kind of strange to me (I'm pretty sure the return value of `convertToContainingWindow` isn't also in absolute/content coordinates?)
Wenson Hsieh
Comment 6 2022-04-08 12:06:48 PDT
Comment on attachment 457095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457095&action=review >> Source/WebCore/dom/mac/ImageControlsMac.cpp:149 >> + auto& element = downcast<Element>(node); > > Let's use RefPtr here. (Ref, sorry!)
Kate Cheney
Comment 7 2022-04-08 12:10:55 PDT
Created attachment 457108 [details] Patch
Kate Cheney
Comment 8 2022-04-08 21:02:11 PDT
thanks for the review!
EWS
Comment 9 2022-04-08 23:30:33 PDT
Committed r292676 (249468@main): <https://commits.webkit.org/249468@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 457108 [details].
Note You need to log in before you can comment on or make changes to this bug.