Bug 233970 – Show correct content menu for images services chevron.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Show correct content menu for images services chevron.

[Megan Gardner]

Created attachment 446284 [details]
Patch

[Tim Horton]

Comment on attachment 446284 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Add support for showing the correct content menu for image service.

"context" "services"

> Source/WebCore/ChangeLog:11
> +        Since this is internal only and can never be affected by web content, we bypass
> +        the web content round trip and directly message the UIProcess about showing 
> +        a context menu for images.

Also because it would be bizarre if the web content could prevent a menu from popping up on a button they have no control over. (instead perhaps a way to prevent the button, but not *just* the menu)

> Source/WebCore/dom/mac/ImageControlsMac.cpp:141
> +        auto result = frame->eventHandler().hitTestResultAtPoint(mouseEvent.absoluteLocation(), hitType);

The event was already routed to the button by a hit test so it seems odd that you're doing *another* (and just to escape the shadow?). Wouldn't it be much simpler and more efficient to just check if your passed-in <button> is inside the image controls shadow DOM (I assume that is doable somehow by walking upwards?) and popping up to the image element that way?

Mostly I'm just saying... this seems a bit roundabout.

ALSO, you're doing this hit test for every click on every <button>; surely if the thing I said above is too hard, there is some other way to short-cut it in the 99(.999999)% case where it's not an image controls button?)

> Source/WebKit/Shared/ContextMenuContextData.cpp:54
> +    , m_webHitTestResultData(WebHitTestResultData(context.hitTestResult(), true))

Likely could also have just been `m_webHitTestResultData({ context.hitTestResult(), true })`

> Source/WebKit/Shared/ContextMenuContextData.cpp:78
> +bool ContextMenuContextData::setImage(WebCore::Image* image)

Everybody ignores this return value, what's it for?

> Source/WebKit/Shared/ContextMenuContextData.h:96
> +    
> +    bool setImage(WebCore::Image*);

This is clearly misplaced, put it before the members.

> Source/WebKit/UIProcess/WebContextMenuProxy.cpp:58
> +    auto hitTestDataOptional = m_context.webHitTestResultData();
> +    if (!hitTestDataOptional)
> +        return;

Perhaps this should just be an assertion? I think we expect it's always engaged if you get here?

> Source/WebKit/UIProcess/WebPageProxy.cpp:6906
> +    auto hitTestDataOptional = m_activeContextMenuContextData.webHitTestResultData();

We don't usually "optional" suffix optionals. hitTestData is fine.

Also I think again this could just be an assertion, and you could stash the optional in a reference at the top of the function and keep using that

> Source/WebKit/UIProcess/mac/WebContextMenuProxyMac.mm:312
> +    auto hitTestDataOptional = m_context.webHitTestResultData();

Apply previous comments to the rest of these files

[Megan Gardner]

Created attachment 446427 [details]
Patch

[Wenson Hsieh]

Comment on attachment 446427 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:72
> +    return downcast<HTMLElement>(node).getIdAttribute() == imageControlsButtonIdentifier();

I might be missing something, but are we guaranteed that `node` is an `HTMLElement` here? If not, this could cause type confusion.
I think it would be safer to do:

```
bool isImageControlsButtonElement(const Node& node)
{
    return is<Element>(node) && downcast<Element>(node).getIdAttribute() == imageControlsButtonIdentifier();
}
```

[Wenson Hsieh]

Comment on attachment 446427 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:120
> +            return nullptr;

(Also, this should be `return false;`)

[Chris Dumez]

Comment on attachment 446427 [details]
Patch

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

> Source/WebCore/dom/mac/ImageControlsMac.cpp:109
> +    if (event.type() == eventNames().clickEvent) {

I'd suggest using != and doing an early return to reduce scoping in this function.

> Source/WebCore/dom/mac/ImageControlsMac.cpp:110
> +        RefPtr<Frame> frame = element.document().frame();

RefPtr frame = element.document().frame();

> Source/WebCore/dom/mac/ImageControlsMac.cpp:118
> +        auto& mouseEvent = downcast<MouseEvent>(event);

This doesn't look safe. What guarantees that this is a MouseEvent? The JS can construct events with any name (e.g. new CustomEvent("click")).

[Megan Gardner]

Created attachment 446450 [details]
Patch

[Megan Gardner]

Created attachment 446478 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Megan Gardner]

Created attachment 446479 [details]
Patch for landing

[Darin Adler]

Comment on attachment 446450 [details]
Patch

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

> Source/WebKit/Shared/ContextMenuContextData.h:55
> +    std::optional<WebHitTestResultData> webHitTestResultData() { return m_webHitTestResultData; }
> +    const std::optional<WebHitTestResultData> webHitTestResultData() const { return m_webHitTestResultData; }

This doesn’t make sense. What’s const here is the std::optional. Consider this analogous case:

    int size() { ... }
    const int size() const { ... }

All the second overload does is return a value you can’t overwrite with assignment, almost no effect at all. Since this returns a copy, we only need one function:

    std::optional<WebHitTestResultData> webHitTestResultData() const { return m_webHitTestResultData; }

Or if we don’t want to do as much copying:

    const std::optional<WebHitTestResultData>& webHitTestResultData() const { return m_webHitTestResultData; }

Or we could return a reference like the code did before, and have two functions.

    std::optional<WebHitTestResultData>& webHitTestResultData() { return m_webHitTestResultData; }
    const std::optional<WebHitTestResultData>& webHitTestResultData() const { return m_webHitTestResultData; }

The latter is worth doing if we want to be able to modify the webHitTestResultData().

[Megan Gardner]

Created attachment 446495 [details]
Patch for landing

[Megan Gardner]

Created attachment 446497 [details]
Patch for landing

[Megan Gardner]

Created attachment 446499 [details]
Patch for landing

[EWS]

Committed r286762 (245003@main): <https://commits.webkit.org/245003@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446499 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86252061>