Bug 82693 – Fix some problems of text field decoration

• Component: Forms
• Status: RESOLVED FIXED

[Kent Tamura]

Fix some problems of text field decoration

[Kent Tamura]

Created attachment 134729 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 134729 [details]
Patch

Attachment 134729 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12267072

[Kent Tamura]

Created attachment 134734 [details]
Patch 2

Fix GTK build

[Hajime Morrita]

Comment on attachment 134729 [details]
Patch

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

Actually, this raises an interesting question: What should we do if decorator-like shadow want to react visual change?
I remember we discussed this a bit before. But I don't think we reached any solid conclusion.

I want to see what will actually happen in didDetach().

> Source/WebCore/ChangeLog:11
> +        change the state of detaching element. This patch change the callback to

s/change/changes

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:73
> +    ShadowRoot* newRoot = 0;

It looks we could have a function to create a necessary shadow root.

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:80
> +        newRoot->removeChild(newRoot->firstChild());

removeChildren()?

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:161
> +    m_textFieldDecorator->didDetach(hostInput());

hostInput() could be null (or assertion could fail inside it) if we use timer here.
since the host <input> could be blew out.
I hope this to be called from something like tree mutation event.

[Kent Tamura]

Comment on attachment 134729 [details]
Patch

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

> I want to see what will actually happen in didDetach().

I guess a typical task in didDetach() will be to close a popup UI corresponding to the specified input element. Changing the input state in didDetach() is not expected, but it is possible.


>> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:161
>> +    m_textFieldDecorator->didDetach(hostInput());
> 
> hostInput() could be null (or assertion could fail inside it) if we use timer here.
> since the host <input> could be blew out.
> I hope this to be called from something like tree mutation event.

Oh, you're right.
We can avoid this issue by keeping RefPtr<HTMLInputElement>. But it would make the code dirtier.

[Kent Tamura]

(In reply to comment #5)
> > hostInput() could be null (or assertion could fail inside it) if we use timer here.
> > since the host <input> could be blew out.
> > I hope this to be called from something like tree mutation event.
> 
> Oh, you're right.
> We can avoid this issue by keeping RefPtr<HTMLInputElement>. But it would make the code dirtier.

I realized using a timer was really a bad idea. RefPtr<HTMLInputElement> doesn't work because ContainerNode::removeAllChildren() apply 'delete' operator to child nodes.

I don't change the code for willDetach(), and add a warning comment.

[Kent Tamura]

Created attachment 134752 [details]
Patch 3

[Dimitri Glazkov (Google)]

Comment on attachment 134752 [details]
Patch 3

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

> Source/WebCore/GNUmakefile.list.am:2480
> +	Source/WebCore/html/shadow/HTMLShaodwElement.h \

Typo.

> Source/WebCore/html/InputType.cpp:396
>          root->removeAllChildren();
> +        root->appendChild(HTMLShadowElement::create(shadowTag, element()->document()));

Since you are the first consumer of this, I am trying to learn lessons here. It seems like we need ability to just "disable" a shadow DOM subtree in a tree stack. This way, you don't need to keep removing and re-adding DOM elements.

> Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:91
> +    getDecorationRootAndDecoratedRoot(input, decorationRoot, existingRoot);

Interesting approach. It's seems we need a way to reference specific shadow DOM subtrees from the shadowTree(), like getElementById.

> Source/WebCore/html/shadow/TextFieldDecorationElement.h:57
> +    // This function is called just before detaching the decoration. It must not
> +    // call functions which updating state of the specified HTMLInputElement
> +    // object.

I am still not happy about needing to articulate this in comments. The best kind of articulation is by design.

[Kent Tamura]

(In reply to comment #8)
> > Source/WebCore/html/InputType.cpp:396
> >          root->removeAllChildren();
> > +        root->appendChild(HTMLShadowElement::create(shadowTag, element()->document()));
> 
> Since you are the first consumer of this, I am trying to learn lessons here. It seems like we need ability to just "disable" a shadow DOM subtree in a tree stack. This way, you don't need to keep removing and re-adding DOM elements.

I think supporting ShadowRoot removal is the best way because of the following.

> > Source/WebCore/html/shadow/TextFieldDecorationElement.cpp:91
> > +    getDecorationRootAndDecoratedRoot(input, decorationRoot, existingRoot);
> 
> Interesting approach. It's seems we need a way to reference specific shadow DOM subtrees from the shadowTree(), like getElementById.

In this case, what we want is the youngest effective (not just <shadow> child) ShadowRoot. If we supported ShadowRoot removal, it would be the youngest ShadowRoot.  It's simple.
If we introduced "disabled" state or something, we would need to iterate over the ShadowRoots eventually.

> > Source/WebCore/html/shadow/TextFieldDecorationElement.h:57
> > +    // This function is called just before detaching the decoration. It must not
> > +    // call functions which updating state of the specified HTMLInputElement
> > +    // object.
> 
> I am still not happy about needing to articulate this in comments. The best kind of articulation is by design.

Yeah, I understand. However I have no good idea to resolve this by design.

[Kent Tamura]

Committed r112830: <http://trac.webkit.org/changeset/112830>