Bug 236747

Created attachment 452286 [details]
WIP

Created attachment 452287 [details]
WIP

Created attachment 452318 [details]
WIP

Created attachment 452319 [details]
WIP

Created attachment 452429 [details]
WIP

Created attachment 452443 [details]
Patch

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.

(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.

(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.

Created attachment 452474 [details]
Patch v1.1

Addressed comments and added a test.

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?

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.

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.

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

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.

Created attachment 452578 [details]
Patch v1.2

(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 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).

(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.

(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.

Created attachment 452583 [details]
Patch v1.3

Created attachment 452757 [details]
Patch for landing

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].

<rdar://problem/89271227>

Re-opened since this is blocked by bug 237131

Created attachment 453310 [details]
Patch with dependencies for EWS

Created attachment 453314 [details]
Patch with dependencies for EWS

Created attachment 453321 [details]
Patch with dependencies for EWS

Created attachment 453362 [details]
Patch for landing

Created attachment 453418 [details]
Patch for landing

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].