Bug 233105 – [Model] [macOS] Add support for interaction on macOS

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

[Antoine Quint]

[Model] [macOS] Add support for interaction on macOS

[Antoine Quint]

<rdar://problem/80079386>

[Antoine Quint]

Created attachment 444189 [details]
Patch

[Sam Weinig]

Comment on attachment 444189 [details]
Patch

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

> Source/WebCore/Modules/model-element/ModelPlayer.h:45
> +    virtual void didReceiveMouseEvent(const String&, const LayoutPoint&, const MonotonicTime&) = 0;

I think this would be a bit nicer if it took a PlatformMouseEvent instead of this adhoc set, though I can't remember the best way to do this (e.g. how to go from a WebCore::Event to WebCore::PlatformMouseEvent, or better yet, just bypass the WebCore::Event stage altogether).

Perhaps Tim or Wenson know, as I bet drag-and-drop has to do things like that.

[Antoine Quint]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 444189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444189&action=review
> 
> > Source/WebCore/Modules/model-element/ModelPlayer.h:45
> > +    virtual void didReceiveMouseEvent(const String&, const LayoutPoint&, const MonotonicTime&) = 0;
> 
> I think this would be a bit nicer if it took a PlatformMouseEvent instead of
> this adhoc set, though I can't remember the best way to do this (e.g. how to
> go from a WebCore::Event to WebCore::PlatformMouseEvent, or better yet, just
> bypass the WebCore::Event stage altogether).

We currently do everything through defaultEventHandler() which is using an Event. If someone can recommend a way to do this with a different approach, maybe we can forgo using Event. Note however that it's important that an Event has been dispatched through the DOM so we can check whether preventDefault() was called.

[Antoine Quint]

Created attachment 444212 [details]
Patch

[Antoine Quint]

Created attachment 444214 [details]
Patch

[Antoine Quint]

Created attachment 444215 [details]
Patch

[Wenson Hsieh]

(In reply to Sam Weinig from comment #3)
> Comment on attachment 444189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444189&action=review
> 
> > Source/WebCore/Modules/model-element/ModelPlayer.h:45
> > +    virtual void didReceiveMouseEvent(const String&, const LayoutPoint&, const MonotonicTime&) = 0;
> 
> I think this would be a bit nicer if it took a PlatformMouseEvent instead of
> this adhoc set, though I can't remember the best way to do this (e.g. how to
> go from a WebCore::Event to WebCore::PlatformMouseEvent, or better yet, just
> bypass the WebCore::Event stage altogether).
> 
> Perhaps Tim or Wenson know, as I bet drag-and-drop has to do things like
> that.

So for drag and drop, we trigger the platform drag using a PlatformMouseEvent, which we obtain from MouseEventWithHitTestResults in EventHandler. It's a bit tricky in this case though, since we're already inside `defaultEventHandler()` which just takes WebCore::Event.

[Antoine Quint]

Sam, how would you like to move forward?

I using MouseEvent is a good way to implement this feature, notably because we need to know whether the Web content has called preventDefault() to cancel the default behavior.

As for the ModelPlayer interface, we could pass in the MouseEvent directly.

I'm happy to rework this patch, but would like to get some actionable feedback to improve it as it's blocking further work I'd like to do with <model> interaction.

[Sam Weinig]

(In reply to Antoine Quint from comment #9)
> Sam, how would you like to move forward?
> 
> I using MouseEvent is a good way to implement this feature, notably because
> we need to know whether the Web content has called preventDefault() to
> cancel the default behavior.
> 
> As for the ModelPlayer interface, we could pass in the MouseEvent directly.
> 
> I'm happy to rework this patch, but would like to get some actionable
> feedback to improve it as it's blocking further work I'd like to do with
> <model> interaction.

Go with what you have for now. We can refactor to use Platform stuff in the future.

[Antoine Quint]

Created attachment 444520 [details]
Patch

[Wenson Hsieh]

Comment on attachment 444520 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:325
> +    auto frame = document().frame();

IIRC best practice™ is to wrap local variables in Ref or RefPtr unless it's only passed into trivial functions (https://www.mail-archive.com/[e-mail]/msg29670.html). Not precisely sure what constitutes "trivial" though :/

I think in this case it's technically safe because nothing below has the potential to trigger layout, but it's probably still better to make this `RefPtr frame`.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:334
> +        m_modelPlayer->handleMouseDown(event.pageLocation(), event.timeStamp());

I would double check that `pageLocation()` does the right thing in subframes.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:351
> +    auto frame = document().frame();

Ditto (`RefPtr frame`).

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:199
> +    auto iterator = m_inlinePreviews.find(uuid);
> +    if (iterator != m_inlinePreviews.end())
> +        return iterator->value;
> +    return nullptr;

Nit - I think the body of this function can simply be `return m_inlinePreviews.get(uuid);`

> Source/WebKit/UIProcess/WebPageProxy.h:598
> +    void handleMouseDownForModelElement(const String&, const WebCore::LayoutPoint&, const MonotonicTime);
> +    void handleMouseMoveForModelElement(const String&, const WebCore::LayoutPoint&, const MonotonicTime);
> +    void handleMouseUpForModelElement(const String&, const WebCore::LayoutPoint&, const MonotonicTime);

Nit - I don't think we usually go out of our way to mark pass-by-value arguments as const? Though it's fine as well, if you prefer to keep it.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:184
> +        page->send(Messages::WebPageProxy::HandleMouseDownForModelElement((String)[m_inlinePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Nit - I think you can omit the (String) here.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:190
> +        page->send(Messages::WebPageProxy::HandleMouseMoveForModelElement((String)[m_inlinePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Ditto.

> Source/WebKit/WebProcess/Model/mac/ARKitInlinePreviewModelPlayerMac.mm:196
> +        page->send(Messages::WebPageProxy::HandleMouseUpForModelElement((String)[m_inlinePreview uuid].UUIDString, locationInPageCoordinates, timestamp));

Ditto.

[Antoine Quint]

Thanks for the comments Wenson, I'm addressing all of those in the patch for landing.

[Antoine Quint]

Created attachment 444545 [details]
Patch for landing

[Antoine Quint]

Committed r285986 (244383@main): <https://commits.webkit.org/244383@main>