Bug 82693

Created attachment 134729 [details] Patch

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

Created attachment 134734 [details] Patch 2 Fix GTK build

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.

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.

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

Created attachment 134752 [details] Patch 3

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.

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

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