Bug 227621 – [Model] Allow disabling interaction

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Quint]

We support inline interaction of <model> elements, at least on iOS, but there should be a way to disable it, or rather opt into it. This means that there likely needs to be an HTML attribute / HTMLModelElement property and also a way to use preventDefault() on a touchstart/mousedown/pointerdown to selectively disable interaction for a given interaction.

[Radar WebKit Bug Importer]

<rdar://problem/80079703>

[Antoine Quint]

We need to be sure that we integrate with touch-action correctly.

[Antoine Quint]

Bug 235697 is a dependency since we would hit a crash when interaction is disabled and dragging on macOS otherwise.

[Antoine Quint]

Created attachment 450122 [details]
Patch

[Darin Adler]

Comment on attachment 450122 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:347
> +void HTMLModelElement::attributeChanged(const QualifiedName& attributeName, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason modificationReason)

Would give these arguments single-word names, "name" and "reason", unless they seem ambiguous in this context.

[Antoine Quint]

Created attachment 450143 [details]
Patch for landing

[Darin Adler]

Comment on attachment 450143 [details]
Patch for landing

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

I looked this over again and had a few other thoughts. Probably none urgent. Sorry I didn’t see these the first time. Good for follow-up I guess.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:344
> +    if (m_modelPlayer && wasInteractive != isInteractive)
> +        m_modelPlayer->setInteractionEnabled(isInteractive);

This code is *not* needed. It will just redo what HTMLModelElement::attributeChanged has already done.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:380
> +    if (type == eventNames().mousedownEvent && !m_isDragging && !event.defaultPrevented() && interactive())
>          dragDidStart(mouseEvent);
>      else if (type == eventNames().mousemoveEvent && m_isDragging)
>          dragDidChange(mouseEvent);
>      else if (type == eventNames().mouseupEvent && m_isDragging)
>          dragDidEnd(mouseEvent);

I don’t fully understand why we should check defaultPrevented only for start, and not for change/end.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:382
> +    else
> +        HTMLElement::defaultEventHandler(event);

I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.

> Source/WebCore/Modules/model-element/HTMLModelElement.h:74
> +    bool interactive() const;
> +    void setInteractive(bool);

We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.

> Source/WebCore/Modules/model-element/HTMLModelElement.h:125
> +    void attributeChanged(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly) final;

I don’t think we need the "= ModifiedDirectly" here.

[Darin Adler]

Comment on attachment 450143 [details]
Patch for landing

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

I looked this over again and had a few other thoughts. Probably none urgent. Sorry I didn’t see these the first time. Good for follow-up I guess.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:344
> +    if (m_modelPlayer && wasInteractive != isInteractive)
> +        m_modelPlayer->setInteractionEnabled(isInteractive);

This code is *not* needed. It will just redo what HTMLModelElement::attributeChanged has already done.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:380
> +    if (type == eventNames().mousedownEvent && !m_isDragging && !event.defaultPrevented() && interactive())
>          dragDidStart(mouseEvent);
>      else if (type == eventNames().mousemoveEvent && m_isDragging)
>          dragDidChange(mouseEvent);
>      else if (type == eventNames().mouseupEvent && m_isDragging)
>          dragDidEnd(mouseEvent);

I don’t fully understand why we should check defaultPrevented only for start, and not for change/end.

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:382
> +    else
> +        HTMLElement::defaultEventHandler(event);

I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.

> Source/WebCore/Modules/model-element/HTMLModelElement.h:74
> +    bool interactive() const;
> +    void setInteractive(bool);

We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.

> Source/WebCore/Modules/model-element/HTMLModelElement.h:125
> +    void attributeChanged(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly) final;

I don’t think we need the "= ModifiedDirectly" here.

[Antoine Quint]

(In reply to Darin Adler from comment #8)
> Comment on attachment 450143 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450143&action=review
> 
> I looked this over again and had a few other thoughts. Probably none urgent.
> Sorry I didn’t see these the first time. Good for follow-up I guess.

No, let's fix them now :)

> > Source/WebCore/Modules/model-element/HTMLModelElement.cpp:344
> > +    if (m_modelPlayer && wasInteractive != isInteractive)
> > +        m_modelPlayer->setInteractionEnabled(isInteractive);
> 
> This code is *not* needed. It will just redo what
> HTMLModelElement::attributeChanged has already done.

Right, will remove it.

> > Source/WebCore/Modules/model-element/HTMLModelElement.cpp:380
> > +    if (type == eventNames().mousedownEvent && !m_isDragging && !event.defaultPrevented() && interactive())
> >          dragDidStart(mouseEvent);
> >      else if (type == eventNames().mousemoveEvent && m_isDragging)
> >          dragDidChange(mouseEvent);
> >      else if (type == eventNames().mouseupEvent && m_isDragging)
> >          dragDidEnd(mouseEvent);
> 
> I don’t fully understand why we should check defaultPrevented only for
> start, and not for change/end.

We've decided that preventDefault() could only interrupt an interaction as it's initiated, not afterwards, for the built-in support to drag to set the camera for <model>. This may be revisited as the <model> proposal goes through the various steps to becoming a Web standard, but I think this is outside of the scope of this particular code review.

> > Source/WebCore/Modules/model-element/HTMLModelElement.cpp:382
> > +    else
> > +        HTMLElement::defaultEventHandler(event);
> 
> I think it would be better to always call through to the base class, rather
> than doing it only in the else. Unless there is some correctness or
> significant performance benefit to not calling through in those other cases.

It seems a lot of classes, maybe all, don't call super by default but only if it elects not to take over handling of the event in question. That makes sense to me, and I followed that pattern.

> > Source/WebCore/Modules/model-element/HTMLModelElement.h:74
> > +    bool interactive() const;
> > +    void setInteractive(bool);
> 
> We don’t need these functions at all. We can just use [Reflect] in the IDL
> file. I suggest we delete them.

Wow! Did not know about that. I'll probably keep interactive() as a private method to avoid calling hasAttributeWithoutSynchronization(HTMLNames::interactiveAttr) instead.

> > Source/WebCore/Modules/model-element/HTMLModelElement.h:125
> > +    void attributeChanged(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly) final;
> 
> I don’t think we need the "= ModifiedDirectly" here.

Will remove.

[Darin Adler]

Comment on attachment 450143 [details]
Patch for landing

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

>>>> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:382
>>>> +        HTMLElement::defaultEventHandler(event);
>>> 
>>> I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.
>> 
>> I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.
> 
> It seems a lot of classes, maybe all, don't call super by default but only if it elects not to take over handling of the event in question. That makes sense to me, and I followed that pattern.

This is kind of a deep subject.

It really depends on what code is in the base class’s defaultEventHandler function. Whether this class does *something* or not is not the whole story. If the base class has some additional handling to do, we’d possibly want to call through to it. Unless it’s something that we should prevent since this class has "fully handled" the event.

I personally think calling the base class unconditionally the more maintainable pattern. After all, to know it’s safe to not call through, you need to know that nothing in the base class function needs to run. In some rare case, it may be that the derived class version really does need to actively prevent what the base class would have done. In that case it’s necessary not to call through.

>>>> Source/WebCore/Modules/model-element/HTMLModelElement.h:74
>>>> +    void setInteractive(bool);
>>> 
>>> We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.
>> 
>> We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.
> 
> Wow! Did not know about that. I'll probably keep interactive() as a private method to avoid calling hasAttributeWithoutSynchronization(HTMLNames::interactiveAttr) instead.

I’d name the private one isInteractive(), I think.

[Antoine Quint]

(In reply to Darin Adler from comment #10)
> Comment on attachment 450143 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450143&action=review
> 
> >>>> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:382
> >>>> +        HTMLElement::defaultEventHandler(event);
> >>> 
> >>> I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.
> >> 
> >> I think it would be better to always call through to the base class, rather than doing it only in the else. Unless there is some correctness or significant performance benefit to not calling through in those other cases.
> > 
> > It seems a lot of classes, maybe all, don't call super by default but only if it elects not to take over handling of the event in question. That makes sense to me, and I followed that pattern.
> 
> This is kind of a deep subject.
> 
> It really depends on what code is in the base class’s defaultEventHandler
> function. Whether this class does *something* or not is not the whole story.
> If the base class has some additional handling to do, we’d possibly want to
> call through to it. Unless it’s something that we should prevent since this
> class has "fully handled" the event.
> 
> I personally think calling the base class unconditionally the more
> maintainable pattern. After all, to know it’s safe to not call through, you
> need to know that nothing in the base class function needs to run. In some
> rare case, it may be that the derived class version really does need to
> actively prevent what the base class would have done. In that case it’s
> necessary not to call through.

OK, I'll move the call to HTMLElement::defaultEventHandler to the top of the function.

> >>>> Source/WebCore/Modules/model-element/HTMLModelElement.h:74
> >>>> +    void setInteractive(bool);
> >>> 
> >>> We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.
> >> 
> >> We don’t need these functions at all. We can just use [Reflect] in the IDL file. I suggest we delete them.
> > 
> > Wow! Did not know about that. I'll probably keep interactive() as a private method to avoid calling hasAttributeWithoutSynchronization(HTMLNames::interactiveAttr) instead.
> 
> I’d name the private one isInteractive(), I think.

Sure, will do that.

[Antoine Quint]

Committed r288728 (246521@trunk): <https://commits.webkit.org/246521@trunk>