Bug 236747 - Make input element UA shadow tree creation lazy
Summary: Make input element UA shadow tree creation lazy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on: 237131 237224 238429
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-16 18:04 PST by Cameron McCormack (:heycam)
Modified: 2022-03-27 19:02 PDT (History)
20 users (show)

See Also:


Attachments
WIP (23.04 KB, patch)
2022-02-16 18:58 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
WIP (37.15 KB, patch)
2022-02-16 19:00 PST, Cameron McCormack (:heycam)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP (37.28 KB, patch)
2022-02-16 22:23 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
WIP (36.86 KB, patch)
2022-02-16 22:30 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
WIP (42.71 KB, patch)
2022-02-17 13:56 PST, Cameron McCormack (:heycam)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (38.86 KB, patch)
2022-02-17 16:22 PST, Cameron McCormack (:heycam)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch v1.1 (42.87 KB, patch)
2022-02-17 20:03 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch v1.2 (42.83 KB, patch)
2022-02-18 13:59 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch v1.3 (42.89 KB, patch)
2022-02-18 14:33 PST, Cameron McCormack (:heycam)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (42.89 KB, patch)
2022-02-21 12:31 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch with dependencies for EWS (56.33 KB, patch)
2022-02-26 14:07 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch with dependencies for EWS (58.34 KB, patch)
2022-02-26 16:32 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch with dependencies for EWS (59.44 KB, patch)
2022-02-26 20:42 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch for landing (49.20 KB, patch)
2022-02-27 17:25 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch for landing (49.20 KB, patch)
2022-02-28 13:01 PST, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2022-02-16 18:04:47 PST
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.
Comment 1 Cameron McCormack (:heycam) 2022-02-16 18:58:38 PST
Created attachment 452286 [details]
WIP
Comment 2 Cameron McCormack (:heycam) 2022-02-16 19:00:17 PST
Created attachment 452287 [details]
WIP
Comment 3 Cameron McCormack (:heycam) 2022-02-16 22:23:53 PST
Created attachment 452318 [details]
WIP
Comment 4 Cameron McCormack (:heycam) 2022-02-16 22:30:10 PST
Created attachment 452319 [details]
WIP
Comment 5 Cameron McCormack (:heycam) 2022-02-17 13:56:15 PST
Created attachment 452429 [details]
WIP
Comment 6 Cameron McCormack (:heycam) 2022-02-17 16:22:15 PST
Created attachment 452443 [details]
Patch
Comment 7 Aditya Keerthi 2022-02-17 17:57:59 PST
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.
Comment 8 Cameron McCormack (:heycam) 2022-02-17 18:49:46 PST
(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.
Comment 9 Aditya Keerthi 2022-02-17 19:10:27 PST
(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.
Comment 10 Cameron McCormack (:heycam) 2022-02-17 20:03:24 PST
Created attachment 452474 [details]
Patch v1.1

Addressed comments and added a test.
Comment 11 Antti Koivisto 2022-02-18 00:43:24 PST
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?
Comment 12 Aakash Jain 2022-02-18 07:14:37 PST
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.
Comment 13 Aakash Jain 2022-02-18 07:16:26 PST
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.
Comment 14 Cameron McCormack (:heycam) 2022-02-18 13:02:33 PST
Sorry I should have cancelled those earlier patches once I got the information from the runs that I needed.
Comment 15 Cameron McCormack (:heycam) 2022-02-18 13:45:42 PST
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.
Comment 16 Cameron McCormack (:heycam) 2022-02-18 13:59:34 PST
Created attachment 452578 [details]
Patch v1.2
Comment 17 Cameron McCormack (:heycam) 2022-02-18 14:00:32 PST
(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.
Comment 18 Aditya Keerthi 2022-02-18 14:19:50 PST
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).
Comment 19 Chris Dumez 2022-02-18 14:22:31 PST
(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.
Comment 20 Chris Dumez 2022-02-18 14:24:02 PST
(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.
Comment 21 Cameron McCormack (:heycam) 2022-02-18 14:33:53 PST
Created attachment 452583 [details]
Patch v1.3
Comment 22 Cameron McCormack (:heycam) 2022-02-21 12:31:11 PST
Created attachment 452757 [details]
Patch for landing
Comment 23 EWS 2022-02-21 19:38:07 PST
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].
Comment 24 Radar WebKit Bug Importer 2022-02-21 19:39:18 PST
<rdar://problem/89271227>
Comment 25 WebKit Commit Bot 2022-02-23 23:25:41 PST
Re-opened since this is blocked by bug 237131
Comment 26 Cameron McCormack (:heycam) 2022-02-26 14:07:00 PST
Created attachment 453310 [details]
Patch with dependencies for EWS
Comment 27 Cameron McCormack (:heycam) 2022-02-26 16:32:16 PST
Created attachment 453314 [details]
Patch with dependencies for EWS
Comment 28 Cameron McCormack (:heycam) 2022-02-26 20:42:34 PST
Created attachment 453321 [details]
Patch with dependencies for EWS
Comment 29 Cameron McCormack (:heycam) 2022-02-27 17:25:44 PST
Created attachment 453362 [details]
Patch for landing
Comment 30 Cameron McCormack (:heycam) 2022-02-28 13:01:46 PST
Created attachment 453418 [details]
Patch for landing
Comment 31 EWS 2022-03-01 02:29:46 PST
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].