Bug 227448 - [Model] [iOS] Add support for manipulating <model> inline
Summary: [Model] [iOS] Add support for manipulating <model> inline
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-06-28 07:39 PDT by Antoine Quint
Modified: 2021-06-30 00:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (18.04 KB, patch)
2021-06-28 07:46 PDT, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch (17.04 KB, patch)
2021-06-29 07:11 PDT, Antoine Quint
thorton: review+
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-06-28 07:39:02 PDT
[Model] [iOS] Add support for manipulating <model> inline
Comment 1 Radar WebKit Bug Importer 2021-06-28 07:39:38 PDT
<rdar://problem/79863579>
Comment 2 Antoine Quint 2021-06-28 07:46:43 PDT
Created attachment 432392 [details]
Patch
Comment 3 Antoine Quint 2021-06-28 07:46:48 PDT
<rdar://problem/79863579>
Comment 4 Tim Horton 2021-06-28 10:27:08 PDT
Comment on attachment 432392 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:74
> +            if ([view isKindOfClass:[WKModelView class]])
> +                return true;

I would expect this to be a WKNativelyInteractible check too?

> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:293
> +        if ([view isKindOfClass:[WKModelView class]])
> +            return view;

Why does WKNativelyInteractible not do the trick?

> Source/WebKit/UIProcess/ios/WKModelInteractionGestureRecognizer.mm:60
> +    auto finalTouchesEnded = [touches isEqualToSet:[event touchesForGestureRecognizer:self]];
> +    [self setState:finalTouchesEnded ? UIGestureRecognizerStateEnded : UIGestureRecognizerStateChanged];

What's this weirdness about? (why not just pass the event type along? Are you sure this never means you don't get Ended?
Comment 5 Antoine Quint 2021-06-28 10:45:46 PDT
(In reply to Tim Horton from comment #4)
> Comment on attachment 432392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432392&action=review
> 
> > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:74
> > +            if ([view isKindOfClass:[WKModelView class]])
> > +                return true;
> 
> I would expect this to be a WKNativelyInteractible check too?
> 
> > Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:293
> > +        if ([view isKindOfClass:[WKModelView class]])
> > +            return view;
> 
> Why does WKNativelyInteractible not do the trick?

So you suggestI only check for WKNativelyInteractible and not WKModelView?

> > Source/WebKit/UIProcess/ios/WKModelInteractionGestureRecognizer.mm:60
> > +    auto finalTouchesEnded = [touches isEqualToSet:[event touchesForGestureRecognizer:self]];
> > +    [self setState:finalTouchesEnded ? UIGestureRecognizerStateEnded : UIGestureRecognizerStateChanged];
> 
> What's this weirdness about? (why not just pass the event type along? Are
> you sure this never means you don't get Ended?

This is so the gesture recognizer doesn't enter the Ended state until the final touches for this gestures are released. Maybe it's not actually necessary to enter the Ended state and just leave it as Changed?
Comment 6 Tim Horton 2021-06-28 10:55:08 PDT
Comment on attachment 432392 [details]
Patch

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

>>> Source/WebKit/UIProcess/RemoteLayerTree/ios/RemoteLayerTreeViews.mm:74
>>> +                return true;
>> 
>> I would expect this to be a WKNativelyInteractible check too?
> 
> So you suggestI only check for WKNativelyInteractible and not WKModelView?

Maybe, yes. Certainly better than a WKModelView check.

On the other hand, does this mean that touch-action doesn't work on these? Maybe this is just entirely wrong, and instead you need to get them in the event regions by default?

>>> Source/WebKit/UIProcess/ios/WKModelInteractionGestureRecognizer.mm:60
>>> +    [self setState:finalTouchesEnded ? UIGestureRecognizerStateEnded : UIGestureRecognizerStateChanged];
>> 
>> What's this weirdness about? (why not just pass the event type along? Are you sure this never means you don't get Ended?
> 
> This is so the gesture recognizer doesn't enter the Ended state until the final touches for this gestures are released. Maybe it's not actually necessary to enter the Ended state and just leave it as Changed?

Oh oh oh, I was misreading it. OK, I get it now.
Comment 7 Antoine Quint 2021-06-29 07:11:53 PDT
Created attachment 432478 [details]
Patch
Comment 8 Tim Horton 2021-06-29 14:38:25 PDT
Comment on attachment 432478 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKModelInteractionGestureRecognizer.mm:38
> +- (void)touchesBegan:(NSSet<UITouch *> *)touches withEvent:(UIEvent *)event

Likely you could avoid the GR and just use the UIResponder versions of these (on WKModelView directly), but this is also perfectly fine.
Comment 9 Antoine Quint 2021-06-30 00:30:14 PDT
Committed r279402 (239267@main): <https://commits.webkit.org/239267@main>