| Summary: | [Model] Allow disabling interaction | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||
| Component: | New Bugs | Assignee: | Antoine Quint <graouts> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cdumez, changseok, darin, dino, esprehn+autocc, ews-watchlist, gyuyoung.kim, kondapallykalyan, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=236493 | ||||||||
| Bug Depends on: | 235697 | ||||||||
| Bug Blocks: | |||||||||
| Attachments: |
|
||||||||
|
Description
Antoine Quint
2021-07-02 05:50:59 PDT
We need to be sure that we integrate with touch-action correctly. Bug 235697 is a dependency since we would hit a crash when interaction is disabled and dragging on macOS otherwise. Created attachment 450122 [details]
Patch
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. Created attachment 450143 [details]
Patch for landing
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. 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. (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. 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. (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. Committed r288728 (246521@trunk): <https://commits.webkit.org/246521@trunk> |