Bug 233105 - [Model] [macOS] Add support for interaction on macOS
Summary: [Model] [macOS] Add support for interaction on macOS
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: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-14 12:40 PST by Antoine Quint
Modified: 2021-11-17 23:58 PST (History)
5 users (show)

See Also:


Attachments
Patch (15.04 KB, patch)
2021-11-14 12:51 PST, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.03 KB, patch)
2021-11-14 23:52 PST, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.21 KB, patch)
2021-11-15 00:02 PST, Antoine Quint
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (15.17 KB, patch)
2021-11-15 00:13 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (23.43 KB, patch)
2021-11-17 07:32 PST, Antoine Quint
wenson_hsieh: review+
Details | Formatted Diff | Diff
Patch for landing (24.54 KB, patch)
2021-11-17 11:53 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2021-11-14 12:40:32 PST
[Model] [macOS] Add support for interaction on macOS
Comment 1 Antoine Quint 2021-11-14 12:41:21 PST
<rdar://problem/80079386>
Comment 2 Antoine Quint 2021-11-14 12:51:45 PST
Created attachment 444189 [details]
Patch
Comment 3 Sam Weinig 2021-11-14 15:59:40 PST
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.
Comment 4 Antoine Quint 2021-11-14 23:50:19 PST
(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.
Comment 5 Antoine Quint 2021-11-14 23:52:29 PST
Created attachment 444212 [details]
Patch
Comment 6 Antoine Quint 2021-11-15 00:02:09 PST
Created attachment 444214 [details]
Patch
Comment 7 Antoine Quint 2021-11-15 00:13:11 PST
Created attachment 444215 [details]
Patch
Comment 8 Wenson Hsieh 2021-11-15 14:57:01 PST
(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.
Comment 9 Antoine Quint 2021-11-16 00:01:32 PST
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.
Comment 10 Sam Weinig 2021-11-16 16:18:40 PST
(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.
Comment 11 Antoine Quint 2021-11-17 07:32:16 PST
Created attachment 444520 [details]
Patch
Comment 12 Wenson Hsieh 2021-11-17 08:00:54 PST
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/webkit-dev@lists.webkit.org/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.
Comment 13 Antoine Quint 2021-11-17 11:53:40 PST
Thanks for the comments Wenson, I'm addressing all of those in the patch for landing.
Comment 14 Antoine Quint 2021-11-17 11:53:57 PST
Created attachment 444545 [details]
Patch for landing
Comment 15 Antoine Quint 2021-11-17 23:58:36 PST
Committed r285986 (244383@main): <https://commits.webkit.org/244383@main>