Bug 236747 – Make input element UA shadow tree creation lazy

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Cameron McCormack (:heycam)]

We currently delay InputType creation for parser inserted elements until just after the attributes have been set, so that we don't wastefully create an InputType and the UA shadow tree creation if a non-text type="" was specified on the tag.  We don't do anything similar for script inserted input elements.  We could make the InputType creation lazy, but most of the wasted time is due to the shadow tree creation.  I think we can make the shadow tree creation lazy.

[Cameron McCormack (:heycam)]

Created attachment 452286 [details]
WIP

[Cameron McCormack (:heycam)]

Created attachment 452287 [details]
WIP

[Cameron McCormack (:heycam)]

Created attachment 452318 [details]
WIP

[Cameron McCormack (:heycam)]

Created attachment 452319 [details]
WIP

[Cameron McCormack (:heycam)]

Created attachment 452429 [details]
WIP

[Cameron McCormack (:heycam)]

Created attachment 452443 [details]
Patch

[Aditya Keerthi]

Comment on attachment 452443 [details]
Patch

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

> Source/WebCore/html/FileInputType.cpp:273
>  void FileInputType::disabledStateChanged()

What ensures the disabled state below is set correctly, if the disabled attribute is set before insertion?

Same comment for the other overrides of this method.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:374
> +    ASSERT(innerTextElement());

I was going to say we might not have an inner text element in WebKit 1 if this is created through `DOMHTMLInputElement` and call `startPosition` before inserting in the document.

But I think this is fine because that method checks that `renderer` is non-null (see `WebVisiblePosition.mm` if you're curious).

> Source/WebCore/html/InputType.h:412
> +    bool createdShadowSubtreeIfNeeded() const { return m_createdShadowSubtreeIfNeeded; }

Can this just be called `createdShadowSubtree`?

> Source/WebCore/html/TextFieldInputType.cpp:111
> +        // isEmptyValue is only called by updatePlaceholderVisibility() to

I wonder if we should rename this method to make it clear that it returns true if the shadow tree hasn't been created.

[Cameron McCormack (:heycam)]

(In reply to Aditya Keerthi from comment #7)
> > Source/WebCore/html/FileInputType.cpp:273
> >  void FileInputType::disabledStateChanged()
> 
> What ensures the disabled state below is set correctly, if the disabled
> attribute is set before insertion?

I overlooked that one.  It needs to be done in createShadowTree.  Other classes that override disabledStateChanged seem to be OK.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:374
> > +    ASSERT(innerTextElement());
> 
> I was going to say we might not have an inner text element in WebKit 1 if
> this is created through `DOMHTMLInputElement` and call `startPosition`
> before inserting in the document.
>
> But I think this is fine because that method checks that `renderer` is
> non-null (see `WebVisiblePosition.mm` if you're curious).

Ah, so these VisiblePosition-related functions can be called for non-editing reasons too?  I was thinking that if they're only used for editing, then they'd need to be in the document, with a renderer, to be called.

> > Source/WebCore/html/InputType.h:412
> > +    bool createdShadowSubtreeIfNeeded() const { return m_createdShadowSubtreeIfNeeded; }
> 
> Can this just be called `createdShadowSubtree`?

Yeah, I guess so.  Then I'll only set m_createdShadowSubtree if we did actually do that (and not if createShadowSubtreeIfNeeded is called but it's for an InputType that doesn't need one).

> > Source/WebCore/html/TextFieldInputType.cpp:111
> > +        // isEmptyValue is only called by updatePlaceholderVisibility() to
> 
> I wonder if we should rename this method to make it clear that it returns
> true if the shadow tree hasn't been created.

Oh, I think I've left an out of date comment there.  In a previous version of the patch, I wasn't doing the lazy construction of the shadow tree when updateInnerTextValue was called.  But now it is, so if there is a non-empty value, then we'll have a shadow tree.  I'll update the comment to say that if there's no subtree, it means a non-empty value hasn't been set yet.

[Aditya Keerthi]

(In reply to Cameron McCormack (:heycam) from comment #8)
> (In reply to Aditya Keerthi from comment #7)
> > > Source/WebCore/html/FileInputType.cpp:273
> > >  void FileInputType::disabledStateChanged()
> > 
> > What ensures the disabled state below is set correctly, if the disabled
> > attribute is set before insertion?
> 
> I overlooked that one.  It needs to be done in createShadowTree.  Other
> classes that override disabledStateChanged seem to be OK.
> 
> > > Source/WebCore/html/HTMLTextFormControlElement.cpp:374
> > > +    ASSERT(innerTextElement());
> > 
> > I was going to say we might not have an inner text element in WebKit 1 if
> > this is created through `DOMHTMLInputElement` and call `startPosition`
> > before inserting in the document.
> >
> > But I think this is fine because that method checks that `renderer` is
> > non-null (see `WebVisiblePosition.mm` if you're curious).
> 
> Ah, so these VisiblePosition-related functions can be called for non-editing
> reasons too?  I was thinking that if they're only used for editing, then
> they'd need to be in the document, with a renderer, to be called.

I was just thinking out loud given `startPosition` is SPI. In any case, we should be fine since it looks like all call sites either check for a renderer or have the node already in the document.

Perhaps Wenson can confirm this assertion is okay?
 
> > > Source/WebCore/html/InputType.h:412
> > > +    bool createdShadowSubtreeIfNeeded() const { return m_createdShadowSubtreeIfNeeded; }
> > 
> > Can this just be called `createdShadowSubtree`?
> 
> Yeah, I guess so.  Then I'll only set m_createdShadowSubtree if we did
> actually do that (and not if createShadowSubtreeIfNeeded is called but it's
> for an InputType that doesn't need one).
> 
> > > Source/WebCore/html/TextFieldInputType.cpp:111
> > > +        // isEmptyValue is only called by updatePlaceholderVisibility() to
> > 
> > I wonder if we should rename this method to make it clear that it returns
> > true if the shadow tree hasn't been created.
> 
> Oh, I think I've left an out of date comment there.  In a previous version
> of the patch, I wasn't doing the lazy construction of the shadow tree when
> updateInnerTextValue was called.  But now it is, so if there is a non-empty
> value, then we'll have a shadow tree.  I'll update the comment to say that
> if there's no subtree, it means a non-empty value hasn't been set yet.

[Cameron McCormack (:heycam)]

Created attachment 452474 [details]
Patch v1.1

Addressed comments and added a test.

[Antti Koivisto]

Comment on attachment 452443 [details]
Patch

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

> Source/WebCore/html/InputType.cpp:1142
> +void InputType::createShadowSubtreeIfNeeded()
> +{
> +    if (m_createdShadowSubtreeIfNeeded || !needsShadowSubtree())
> +        return;
> +    Ref protectedThis { *this };
> +    element()->ensureUserAgentShadowRoot();
> +    m_createdShadowSubtreeIfNeeded = true;
> +    createShadowSubtree();
> +}

Should this test isConnected()? Or does that get tested on some other level?

[Aakash Jain]

Cancelled https://ews-build.webkit.org/#/builders/68/builds/8521 (for WIP patch 452429) to speed up ios-wk2 queue. It already finished running layout-tests and the failures seems to be pre-existing.

[Aakash Jain]

Separately, if theses patches are iterations (and you don't intend to land all of these patches), it would be nice if you can mark older patches as 'obsolete' while uploading new patch (webkit-patch upload automatically does that). If a patch is marked as 'obsolete' EWS stop processing it.

[Cameron McCormack (:heycam)]

Sorry I should have cancelled those earlier patches once I got the information from the runs that I needed.

[Cameron McCormack (:heycam)]

The one remaining test failure is due to something like:

e = document.createElement("input");
e.value = "x";
e.type = "number";

Since the patch is calling updateInnerTextValue inside createShadowTree, we end up trying to localize the "x" string as a number, and asserting.  We need to sanitize the value before updateInnerTextValue, which is what HTMLInputElement::updateType does.  So I think I will pull out that updateInnerTextValue call from createShadowTree, and explicitly call it in the one place I need it (in HTMLInputElement::didFinishInsertingNode), instead of always calling it during shadow tree creation.

[Cameron McCormack (:heycam)]

Created attachment 452578 [details]
Patch v1.2

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #15)
> So I think I will pull out that
> updateInnerTextValue call from createShadowTree, and explicitly call it in
> the one place I need it (in HTMLInputElement::didFinishInsertingNode),
> instead of always calling it during shadow tree creation.

Actually I think it's not even necessary to call updateInnerTextValue in didFinishInsertingNode, since the only time we'll be actually creating the shadow tree there is if we never set a value on the input element.

[Aditya Keerthi]

Comment on attachment 452578 [details]
Patch v1.2

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

> Source/WebCore/html/InputType.h:209
> +    bool createdShadowSubtree() const { return m_createdShadowSubtree; }

I know I suggested renaming this from `createdShadowSubtreeIfNeeded` to `createdShadowSubtree`, but I think an even better name (matching WebKit style) is `hasShadowSubtree` (and m_hasShadowSubtree).

[Chris Dumez]

(In reply to Aditya Keerthi from comment #18)
> Comment on attachment 452578 [details]
> Patch v1.2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452578&action=review
> 
> > Source/WebCore/html/InputType.h:209
> > +    bool createdShadowSubtree() const { return m_createdShadowSubtree; }
> 
> I know I suggested renaming this from `createdShadowSubtreeIfNeeded` to
> `createdShadowSubtree`, but I think an even better name (matching WebKit
> style) is `hasShadowSubtree` (and m_hasShadowSubtree).

Per WebKit coding style (https://webkit.org/code-style-guidelines/#names-bool), Boolean variables (and thus their getters) should have a prefix (e.g. is / has).

In this case, I think "hasCreatedShadowSubtree" would be appropriate.

[Chris Dumez]

(In reply to Chris Dumez from comment #19)
> (In reply to Aditya Keerthi from comment #18)
> > Comment on attachment 452578 [details]
> > Patch v1.2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=452578&action=review
> > 
> > > Source/WebCore/html/InputType.h:209
> > > +    bool createdShadowSubtree() const { return m_createdShadowSubtree; }
> > 
> > I know I suggested renaming this from `createdShadowSubtreeIfNeeded` to
> > `createdShadowSubtree`, but I think an even better name (matching WebKit
> > style) is `hasShadowSubtree` (and m_hasShadowSubtree).
> 
> Per WebKit coding style
> (https://webkit.org/code-style-guidelines/#names-bool), Boolean variables
> (and thus their getters) should have a prefix (e.g. is / has).
> 
> In this case, I think "hasCreatedShadowSubtree" would be appropriate.

I am also fine with "hasShadowSubtree" (like Aditya suggested), I just think it needs a prefix.

[Cameron McCormack (:heycam)]

Created attachment 452583 [details]
Patch v1.3

[Cameron McCormack (:heycam)]

Created attachment 452757 [details]
Patch for landing

[EWS]

Committed r290284 (247608@main): <https://commits.webkit.org/247608@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452757 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89271227>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 237131

[Cameron McCormack (:heycam)]

Created attachment 453310 [details]
Patch with dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 453314 [details]
Patch with dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 453321 [details]
Patch with dependencies for EWS

[Cameron McCormack (:heycam)]

Created attachment 453362 [details]
Patch for landing

[Cameron McCormack (:heycam)]

Created attachment 453418 [details]
Patch for landing

[EWS]

Committed r290639 (247912@main): <https://commits.webkit.org/247912@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453418 [details].