Bug 62116

Created attachment 96072 [details] Patch

Comment on attachment 96072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96072&action=review I think this approach is wrong. We should not be special-casing id handling inside shadow DOM. Or maybe I just don't understand why we would want to do that? > Source/WebCore/dom/Element.cpp:961 > + if (hasID() && m_attributeMap && treeScope() == document()) { Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right.

Comment on attachment 96072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96072&action=review > Source/WebCore/ChangeLog:11 > + - insertedIntoDocument() for HTMLInputElement and shadow nodes were This seems like the problem we need to be fixing.

Comment on attachment 96072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96072&action=review >> Source/WebCore/ChangeLog:11 >> + - insertedIntoDocument() for HTMLInputElement and shadow nodes were > > This seems like the problem we need to be fixing. Yeah, probably this behavior is not intended. I'll try to fix it today. >> Source/WebCore/dom/Element.cpp:961 >> + if (hasID() && m_attributeMap && treeScope() == document()) { > > Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right. I think this is a right change though it might be a good way to fix this bug. We had a similar problem in https://bugs.webkit.org/show_bug.cgi?id=61986#c25 . ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document. Now ShadowRoot also manges subordinate IDs. So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot. I think it's very straightforward behavior.

(In reply to comment #5) > it might be a good way to fix this bug. it might not be a ...

(In reply to comment #5) > (From update of attachment 96072 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=96072&action=review > > >> Source/WebCore/ChangeLog:11 > >> + - insertedIntoDocument() for HTMLInputElement and shadow nodes were > > > > This seems like the problem we need to be fixing. > > Yeah, probably this behavior is not intended. I'll try to fix it today. It seems to be hard. My simple change to fix the attach-before-insertedIntoDocument issue made another 7 layout test regressions.

(In reply to comment #5) > >> Source/WebCore/dom/Element.cpp:961 > >> + if (hasID() && m_attributeMap && treeScope() == document()) { > > > > Why do we have to have a special way of handling shadow DOM nodes here? Other than working around the bug you're fixing? I don't think this is right. > > I think this is a right change though it might be a good way to fix this bug. We had a similar problem in https://bugs.webkit.org/show_bug.cgi?id=61986#c25 . > > ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document. Now ShadowRoot also manges subordinate IDs. So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot. I think it's very straightforward behavior. Roland, Morita-san, do you have any comments on ID in ShadowRoot?

*** Bug 62109 has been marked as a duplicate of this bug. ***

*** Bug 62030 has been marked as a duplicate of this bug. ***

It's not good to keep the crash. So I have decided to revert a part of r87980. Let's discuss the ID issue in a separate bug.

Renaming the bug so that this bug shows up when people search for ieForbidsInsertHTML.

Landed the revert: http://trac.webkit.org/changeset/88216

(In reply to comment #8) > (In reply to comment #5) > > ID registration/unregistration is handled in insertedIntoDocument()/removedFromDocument() because IDs are managed by a document. Now ShadowRoot also manges subordinate IDs. So ID registration/unregistration for ShadowRoot should be handled when an element is inserted into or removed from a ShadowRoot. I think it's very straightforward behavior. I also think the above is correct. The cleanest solution IMHO would be that a node ALWAYS registers its ID with its TreeScope when it's inserted into the tree. To this end, we would need another TreeScope sub-class to contain the nodes that are associated with the Document, but !inDocument(). (This specific sub-class could even just do a no-op on ID registration). However, this is a larger refactoring, probably outside the immediate scope of this bug.