Closed Bug 1091425 Opened 6 years ago Closed 6 years ago

Create elements in the document on the "intended parent"

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: wchen, Assigned: wchen)

Details

Attachments

(3 files)

When creating an element in the parser, it should be created in the document of the "intended parent".

https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token

We currently create all elements in the main document and it causes us to adopt nodes where it is not necessary.

When templates and custom elements are used together, the difference in behavior is very obvious. Custom elements are supposed to be inert inside template contents because it should be created in a special document that does not have a custom element registry. However, we currently create custom elements in the main document (and then gets adopted into another document when appended to the template contents owner). This causes the custom elements to be set up and invokes the created callback.
This patch adds an intended parent argument to createElement and updates the callers to pass in the appropriate node.

One of the steps to find the parent in the foster parenting algorithm is to check the parent of a <table> node, and we can only do this during tree op execution. Thus we have to delay creating the fostered child until we have started executing tree op. So I've moved the logic of creating an element and inserting it into a foster parent into tree ops.
Attachment #8514695 - Flags: review?(hsivonen)
This patch changes the nsNodeInfoManager used to create elements, text nodes and comments so that we create it in the correct document as specified.

https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token
https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token
https://html.spec.whatwg.org/multipage/syntax.html#insert-a-comment

It also contains a new tree op to find the foster parent so that we may use it as the "intended parent" when creating elements that are going to be fostered.
Attachment #8514699 - Flags: review?(hsivonen)
Comment on attachment 8514695 [details] [diff] [review]
Java parser - Provide intended parent when creating elements in the parser.

Review of attachment 8514695 [details] [diff] [review]:
-----------------------------------------------------------------

I'm pretty unhappy that the spec has gained a complication like this, but it seems that this patch implements the complication correctly. Thanks.

::: src/nu/validator/htmlparser/impl/TreeBuilder.java
@@ +5641,5 @@
>      protected abstract void insertFosterParentedChild(T child, T table,
>              T stackParent) throws SAXException;
>  
> +    // [NOCPP[
> +

Please add a comment that explains why this is [NOCPP[. (Took me a while to poke around.)
Attachment #8514695 - Flags: review?(hsivonen) → review+
Comment on attachment 8514699 [details] [diff] [review]
Use the correct document when creating nodes in the parser.

Review of attachment 8514699 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/html/nsHtml5TreeOperation.cpp
@@ +677,5 @@
>        nsCOMPtr<nsIAtom> name = Reget(mTwo.atom);
>        nsHtml5HtmlAttributes* attributes = mThree.attributes;
> +      nsIContent* intendedParent = mFive.node ? *(mFive.node) : nullptr;
> +
> +      // intendedParent == nullptr is a special case where we the

Stray word "we"?

::: parser/html/nsHtml5TreeOperation.h
@@ +494,5 @@
>        char*                           charPtr;
>        nsHtml5TreeOperationStringPair* stringPair;
>        nsAHtml5TreeBuilderState*       state;
>        int32_t                         integer;
> +    } mOne, mTwo, mThree, mFour, mFive;

It would be interesting to know what the perf cost (if any) of growing the tree ops by the pointer size is... (I think growing the ops is what's needed, though.)

Please file a follow-up for getting rid of nsHtml5TreeOperationStringPair now that the ops themselves can hold one more item, so the indirection of pairing two strings into one item is no longer necessary.
Attachment #8514699 - Flags: review?(hsivonen) → review+
Follow up bug for removing nsHtml5TreeOperationStringPair: Bug 1096166
https://hg.mozilla.org/mozilla-central/rev/ede99651d089
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.