Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upstream XMLSerializer from DOM Parsing and Serialization spec #10498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Upstream XMLSerializer from DOM Parsing and Serialization spec #10498

wants to merge 1 commit into from

Conversation

lukewarlow
Copy link
Member

@lukewarlow lukewarlow commented Jul 18, 2024 •
edited by pr-preview bot
Loading

  • At least two implementers are interested (and none opposed):
    • …
    • …
  • Tests are written and can be reviewed and commented upon at:
    • …
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

/dynamic-markup-insertion.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/xhtml.html ( diff )

@domenic
Copy link
Member

domenic commented Jul 19, 2024

We should also be sure to replace https://html.spec.whatwg.org/#serialising-xhtml-fragments

@lukewarlow
Copy link
Member Author

We should also be sure to replace https://html.spec.whatwg.org/#serialising-xhtml-fragments

Yep absolutely. I'm pretty sure it's not used anywhere so I should just be able to replace it with the new one (which I planned to move to that area anyway).

lukewarlow
lukewarlow commented Jul 19, 2024
View reviewed changes
source Outdated Show resolved Hide resolved
@lukewarlow lukewarlow force-pushed the xml-serializer branch 4 times, most recently from 281ca5c to f5dcb1a Compare July 19, 2024 23:52
@lukewarlow lukewarlow force-pushed the xml-serializer branch 5 times, most recently from 5b4b320 to 5ad1670 Compare August 29, 2024 18:04
@lukewarlow lukewarlow marked this pull request as ready for review August 29, 2024 18:08
@lukewarlow lukewarlow marked this pull request as draft August 29, 2024 18:14
@lukewarlow lukewarlow marked this pull request as ready for review August 29, 2024 19:28
@lukewarlow
Copy link
Member Author

@domenic sorry for the delay on getting back to this. It should hopefully include everything the domparsing spec has with some editorial tweaks here and there. I've also included CDATA serialization as browsers do include it (was commented it out in dom parsing spec).

No rush but I think it's now ready for an initial review.

@lukewarlow lukewarlow changed the title WIP: Upstream XMLSerializer from DOM Parsing and Serialization spec Upstream XMLSerializer from DOM Parsing and Serialization spec Aug 29, 2024
domenic
domenic reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I got most of the way through serializing an element...

Overall, there's a lot of work left to fit WHATWG style. Several of my comments apply pretty broadly, so it might help if you could try to do a pass to apply those and other cases you find, and then I'll do another pass?

source
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-domimplementation"><code>DOMImplementation</code></dfn> interface</li>
<li><dfn data-x="DOM Document" data-x-href="https://dom.spec.whatwg.org/#interface-document"><code>Document</code></dfn> interface and its
<dfn data-x="dom-document-element" data-x-href="https://dom.spec.whatwg.org/#document-element"><code>documentElement</code></dfn> concept and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be "document element" and not in code.

source
<dfn data-x="dom-Document-doctype" data-x-href="https://dom.spec.whatwg.org/#dom-document-doctype"><code>doctype</code></dfn> attribute
</li>
<li><dfn data-x="DOM DocumentOrShadowRoot" data-x-href="https://dom.spec.whatwg.org/#documentorshadowroot"><code>DocumentOrShadowRoot</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-documentfragment"><code>DocumentFragment</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-documenttype"><code>DocumentType</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-documenttype"><code>DocumentType</code></dfn> interface, and its <dfn data-x="dom-documenttype-name" data-x-href="https://dom.spec.whatwg.org/#dom-documenttype-name">name</dfn>, <dfn data-x="dom-documenttype-publicid" data-x-href="https://dom.spec.whatwg.org/#dom-documenttype-publicid">publicId</dfn> and <dfn data-x="dom-documenttype-systemid" data-x-href="https://dom.spec.whatwg.org/#dom-documenttype-systemid">systemId</dfn> attributes</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should go the internal slots, e.g. "public ID" with https://dom.spec.whatwg.org/#concept-doctype-publicid , not the getters.

source
@@ -3159,15 +3163,17 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute

<ul class="brief">
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-attr"><code>Attr</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-characterdata"><code>CharacterData</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-characterdata"><code>CharacterData</code></dfn> interface, and its <dfn data-x="dom-characterdata-data" data-x-href="https://dom.spec.whatwg.org/#dom-characterdata-data">data</dfn> attribute</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to the internal slot, https://dom.spec.whatwg.org/#concept-cd-data , not the attribute.

source
@@ -3178,7 +3184,7 @@ a.setAttribute('href', 'https://example.com/'); // change the content attribute
<li>The <dfn data-x="dom-retarget" data-x-href="https://dom.spec.whatwg.org/#retarget">retargeting algorithm</dfn></li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-node"><code>Node</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-nodelist"><code>NodeList</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-processinginstruction"><code>ProcessingInstruction</code></dfn> interface</li>
<li><dfn data-x-href="https://dom.spec.whatwg.org/#interface-processinginstruction"><code>ProcessingInstruction</code></dfn> interface, and its <dfn data-x="dom-processinginstruction-target" data-x-href="https://dom.spec.whatwg.org/#dom-processinginstruction-target">target</dfn> attribute</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internal slot, not getter

source
<li><p>Return the <span data-x="xml-serialization">XML serialization</span> of <var>node</var>
given <var>require well-formed</var>.</p></li>
<li><p>Return the result of <span>XML fragment serialization algorithm</span> with
<var>node</var> and <var>require well-formed</var>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and in the previous step, we should say "result of running" instead of "result of", for better grammar.

source
other XML constraints; for example, trying to append two elements to a <code>Document</code> node
will throw a <span>"<code>HierarchyRequestError</code>"</span> <code>DOMException</code>.</p>
<ol>
<li><p>If the <var>local prefixes map</var> contains a key matching <var>prefix</var>, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<li><p>If the <var>local prefixes map</var> contains a key matching <var>prefix</var>, then
<li><p>If <var>local prefixes map</var> contains a key matching <var>prefix</var>, then

source
other XML constraints; for example, trying to append two elements to a <code>Document</code> node
will throw a <span>"<code>HierarchyRequestError</code>"</span> <code>DOMException</code>.</p>
<ol>
<li><p>If the <var>local prefixes map</var> contains a key matching <var>prefix</var>, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use https://infra.spec.whatwg.org/#map-exists

source
<ol>
<li><p>If the <var>local prefixes map</var> contains a key matching <var>prefix</var>, then
let <var>prefix</var> be the result of <span data-x="generate a prefix">generating a
prefix</span> providing as input <var>map</var>, <var>ns</var> and <var>prefix
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
prefix</span> providing as input <var>map</var>, <var>ns</var> and <var>prefix
prefix</span> given <var>map</var>, <var>ns</var> and <var>prefix

source
<li><p>The string "<code data-x="">xmlns:</code>";</p></li>
<li><p><var>prefix</var></p></li>
<li><p>"<code data-x="">="</code>" (U+003D EQUALS SIGN, U+0022 QUOTATION MARK);</p></li>
<li><p>The result of <span data-ignore="">serializing an attribute</span> given
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, fix the data-ignores.

source
<li><p>The result of <span data-ignore="">serializing an attribute</span> given
<var>ns</var> and <var>require well-formed</var>;</p></li>
<li><p>"<code data-x="">"</code>" (U+0022 QUOTATION MARK).</p></li>
<li><p>If <var>local default namespace</var> is not null, then let <var>inherited ns</var>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step doesn't seem to belong here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domenic domenic domenic left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lukewarlow @domenic