Bug 238672 - Sharing services picker menu Mail presents in the wrong place when compose web view is scrolled
Summary: Sharing services picker menu Mail presents in the wrong place when compose we...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-01 11:03 PDT by Kate Cheney
Modified: 2022-04-08 23:30 PDT (History)
9 users (show)

See Also:

Attachments
Patch (3.63 KB, patch)
2022-04-01 11:05 PDT, Kate Cheney 		no flags Details | Formatted Diff | Diff
Patch (2.10 KB, patch)
2022-04-08 10:54 PDT, Kate Cheney 		no flags Details | Formatted Diff | Diff
Patch (2.04 KB, patch)
2022-04-08 12:10 PDT, Kate Cheney 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2022-04-01 11:03:44 PDT 
Sharing services picker menu Mail presents in the wrong place when compose web view is scrolled
Comment 1 Kate Cheney 2022-04-01 11:05:26 PDT 
Created attachment 456373 [details]
Patch
Comment 2 Kate Cheney 2022-04-01 11:05:44 PDT 
rdar://88966924
Comment 3 Wenson Hsieh 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)
Comment 4 Kate Cheney 2022-04-08 10:54:52 PDT 
Created attachment 457095 [details]
Patch
Comment 5 Wenson Hsieh 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?)
Comment 6 Wenson Hsieh 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!)
Comment 7 Kate Cheney 2022-04-08 12:10:55 PDT 
Created attachment 457108 [details]
Patch
Comment 8 Kate Cheney 2022-04-08 21:02:11 PDT 
thanks for the review!
Comment 9 EWS 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].