Skip shadow-root creation for input element if it is not necessary
Created attachment 431810 [details] Patch
Created attachment 431811 [details] Patch
Comment on attachment 431811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431811&action=review Many nits but generally seems ok. r=me but please consider the individual comments. > Source/WebCore/html/BaseDateAndTimeInputType.cpp:309 > + ASSERT(needsShadowSubtree()); There seem to be a lot of asserts of needsShadowSubtree() all over the place and I'm not sure what they are there for. needsShadowSubtree() is fixed based on input type, so there can't be an inconsistency at runtime unless someone forgot to add a type to the list of types. I could see having a compile-time assert somewhere maybe, but why have this in every implementation of createShadowSubtreeAndUpdateInnerTextElementEditability and every input type constructor? > Source/WebCore/html/HTMLInputElement.cpp:87 > -class ListAttributeTargetObserver : IdTargetObserver { > +class ListAttributeTargetObserver final : public IdTargetObserver { This seems unrelated to the rest of the change and isn't documented in the ChangeLog. > Source/WebCore/html/HTMLInputElement.cpp:138 > + ASSERT(m_inputType->needsShadowSubtree()); Extra weird to assert here considering we would have asserted in the constructor (and nothing seems to depend on this condition). > Source/WebCore/html/HTMLInputElement.cpp:164 > +void HTMLInputElement::createShadowSubtree() Why this rename? Other elements have didAddUserAgentShadowRoot and not createShadowSubtree. didAddUserAgentShadowRoot is called automatically from addShadowRoot, so with this method at the original name, the explicit calls to createShadowSubtree should be mostly not necessary (except in cases where it's known there is already a shadow root; is the goal here to simplicy that logic? > Source/WebCore/html/HTMLInputElement.cpp:586 > - m_inputType->createShadowSubtreeAndUpdateInnerTextElementEditability(m_parsingInProgress ? ChildChange::Source::Parser : ChildChange::Source::API, isInnerTextElementEditable()); > + if (m_inputType->needsShadowSubtree()) { > + ensureUserAgentShadowRoot(); > + createShadowSubtree(); > + } I guess if tests pass it must be ok, but the old function name suggest there's a need to do something related to inner text element editability, but I don't think the new code will do that. > Source/WebCore/html/HTMLInputElement.cpp:1279 > + // Some input types only need shadow roots to hide any children that may > + // have been appended by script. For such types, shadow roots are lazily > + // created when children are added for the first time. > + ensureUserAgentShadowRoot(); I'm not sure it's necessary to have a shadow root to hide children and it might not be the most efficient way to do so. form controls managed to hide their children before there was such a thing as shadow DOM. I wonder how we did it before. I guess its of if it actually works. > Source/WebCore/html/HTMLInputElement.h:139 > - RenderStyle createInnerTextStyle(const RenderStyle&) override; > + RenderStyle createInnerTextStyle(const RenderStyle&) final; Seems unrelated to the change and isn't documented in ChangeLog. > Source/WebCore/html/HTMLInputElement.h:272 > - bool willRespondToMouseClickEvents() override; > + bool willRespondToMouseClickEvents() final; Seems unrelated to the change and isn't documented in ChangeLog. > Source/WebCore/html/HTMLInputElement.h:351 > - void defaultEventHandler(Event&) override; > + void defaultEventHandler(Event&) final; Seems unrelated to the change and isn't documented in ChangeLog. > LayoutTests/fast/forms/checkbox-child-hidden-expected.html:5 > +<input type="checkbox" id="cb" /> The / at the end of the <input> element is unnecessary and is invalid in HTML. (It would have been both necessary and valid in XHTML).
Comment on attachment 431811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431811&action=review Thanks! And based on HTMLTextFormControlElement::childShouldCreateRenderer, I've added another test which uses pseudo-element. >> Source/WebCore/html/BaseDateAndTimeInputType.cpp:309 >> + ASSERT(needsShadowSubtree()); > > There seem to be a lot of asserts of needsShadowSubtree() all over the place and I'm not sure what they are there for. needsShadowSubtree() is fixed based on input type, so there can't be an inconsistency at runtime unless someone forgot to add a type to the list of types. I could see having a compile-time assert somewhere maybe, but why have this in every implementation of createShadowSubtreeAndUpdateInnerTextElementEditability and every input type constructor? I think this assertion is good to make needsShadowSubtree() implementation in sync with the actual implementation. We mechanically have this assertion for each createShadowSubtreeAndUpdateInnerTextElementEditability, so that any new InputType will follow this style, and do not forget to modify needsShadowSubtree. >> Source/WebCore/html/HTMLInputElement.cpp:87 >> +class ListAttributeTargetObserver final : public IdTargetObserver { > > This seems unrelated to the rest of the change and isn't documented in the ChangeLog. Added documentation in ChangeLog. >> Source/WebCore/html/HTMLInputElement.cpp:138 >> + ASSERT(m_inputType->needsShadowSubtree()); > > Extra weird to assert here considering we would have asserted in the constructor (and nothing seems to depend on this condition). This is corresponding to `HTMLInputElement::create`'s `if (!shouldCreateShadowRootLazily)` path, and we ensure that the default InputType requires shadow-subtree. >> Source/WebCore/html/HTMLInputElement.cpp:164 >> +void HTMLInputElement::createShadowSubtree() > > Why this rename? Other elements have didAddUserAgentShadowRoot and not createShadowSubtree. didAddUserAgentShadowRoot is called automatically from addShadowRoot, so with this method at the original name, the explicit calls to createShadowSubtree should be mostly not necessary (except in cases where it's known there is already a shadow root; is the goal here to simplicy that logic? Yes, I would like to simplify the logic instead of using the callback since we know when we should create shadow subtree for this element. Since HTMLInputElement's InputType can be changed (e.g. from "button" => "checkbox"), then there is a case like, Non-ShadowRoot-Type => ShadowRoot-Type ShadowRoot-Type => ShadowRoot-Type ShadowRoot-Type => Non-ShadowRoot-Type Non-ShadowRoot-Type => Non-ShadowRoot-Type and this callback is called only when we first create a shadow-root. So, we were explicitly calling `m_inputType->createShadowSubtreeAndUpdateInnerTextElementEditability(m_parsingInProgress ? ChildChange::Source::Parser : ChildChange::Source::API, isInnerTextElementEditable());` in updateType function. But this is unnecessarily complicated. By removing didAddUserAgentShadowRoot, each site calls ensureUserAgentShadowRoot / createUserAgentShadowRoot / createShadowSubtree and the logic looks much simpler. >> Source/WebCore/html/HTMLInputElement.cpp:586 >> + } > > I guess if tests pass it must be ok, but the old function name suggest there's a need to do something related to inner text element editability, but I don't think the new code will do that. Renamed it to createShadowSubtreeAndUpdateInnerTextElementEditability >> Source/WebCore/html/HTMLInputElement.cpp:1279 >> + ensureUserAgentShadowRoot(); > > I'm not sure it's necessary to have a shadow root to hide children and it might not be the most efficient way to do so. form controls managed to hide their children before there was such a thing as shadow DOM. I wonder how we did it before. I guess its of if it actually works. Yup. Found that we do not need to have this. HTMLTextFormControlElement::childShouldCreateRenderer handles things well: children only get renderers when it is under shadow-root. Removed this overriding entirely. >> Source/WebCore/html/HTMLInputElement.h:139 >> + RenderStyle createInnerTextStyle(const RenderStyle&) final; > > Seems unrelated to the change and isn't documented in ChangeLog. Added documentation in ChangeLog. >> Source/WebCore/html/HTMLInputElement.h:272 >> + bool willRespondToMouseClickEvents() final; > > Seems unrelated to the change and isn't documented in ChangeLog. Added documentation in ChangeLog. >> Source/WebCore/html/HTMLInputElement.h:351 >> + void defaultEventHandler(Event&) final; > > Seems unrelated to the change and isn't documented in ChangeLog. Added documentation in ChangeLog. >> LayoutTests/fast/forms/checkbox-child-hidden-expected.html:5 >> +<input type="checkbox" id="cb" /> > > The / at the end of the <input> element is unnecessary and is invalid in HTML. (It would have been both necessary and valid in XHTML). Fixed.
Comment on attachment 431811 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431811&action=review >>> Source/WebCore/html/HTMLInputElement.cpp:138 >>> + ASSERT(m_inputType->needsShadowSubtree()); >> >> Extra weird to assert here considering we would have asserted in the constructor (and nothing seems to depend on this condition). > > This is corresponding to `HTMLInputElement::create`'s `if (!shouldCreateShadowRootLazily)` path, and we ensure that the default InputType requires shadow-subtree. But since we already did ASSERT in HTMLInputElement::create, this is redundant. Removed.
Created attachment 431814 [details] Patch
(In reply to Yusuke Suzuki from comment #6) > Created attachment 431814 [details] > Patch This is collecting render-tree-dump for each platform.
rdar://79473176
Committed r279054 (238974@main): <https://commits.webkit.org/238974@main>