Summary: | [Model] [macOS] Add support for interaction on macOS | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||
Component: | New Bugs | Assignee: | Antoine Quint <graouts> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | dino, sam, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Antoine Quint
2021-11-14 12:40:32 PST
Created attachment 444189 [details]
Patch
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. (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. Created attachment 444212 [details]
Patch
Created attachment 444214 [details]
Patch
Created attachment 444215 [details]
Patch
(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. 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. (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. Created attachment 444520 [details]
Patch
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. Thanks for the comments Wenson, I'm addressing all of those in the patch for landing. Created attachment 444545 [details]
Patch for landing
Committed r285986 (244383@main): <https://commits.webkit.org/244383@main> |