Skip to content

Serialize the children of void elements as the empty string#4238

Merged
domenic merged 4 commits intowhatwg:masterfrom
Zirro:void-innerhtml-empty-string
Dec 19, 2018
Merged

Serialize the children of void elements as the empty string#4238
domenic merged 4 commits intowhatwg:masterfrom
Zirro:void-innerhtml-empty-string

Conversation

@Zirro
Copy link
Copy Markdown
Contributor

@Zirro Zirro commented Dec 13, 2018 •
edited by pr-preview Bot
Loading

Proposed solution to #4220, which makes the algorithm consistent with the current implementations in Chrome and Safari. The intention is for the innerHTML of void elements to return the empty string even in cases where child nodes are present. I changed the steps to explicitly return a value in two locations as it made most sense for this change.

There are tests for this in https://wpt.fyi/results/html/syntax/serializing-html-fragments/serializing.html which previously disagreed with the specification. This change would align the specification with the tests.

/parsing.html ( diff )

@Zirro
Serialize the children of void elements as the empty string
9fa5157 
This is consistent with the current implementations in Chrome and Safari.
annevk
annevk reviewed Dec 13, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

How about we put VOIDLIST in a variable, so we only have to enumerate it once in this algorithm? (FWIW, I checked, only this algorithm makes use of it within HTML and I suspect that means everywhere.) Looks good otherwise, appreciate the algorithm prose cleanup too.

@annevk
Copy link
Copy Markdown
Member

annevk commented Dec 13, 2018

Do you also want to file a bug against Firefox?

@annevk annevk added topic: parser interop Implementations are not interoperable with each other labels Dec 13, 2018
@Zirro
Copy link
Copy Markdown
Contributor Author

Zirro commented Dec 15, 2018

@annevk Variable added. Here's the Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1514499

@domenic
Copy link
Copy Markdown
Member

domenic commented Dec 18, 2018

I pushed some editorial tweaks. @Zirro and/or @annevk, can you double-check my work? This seems good to merge to me now, but we should get a second set of eyes.

Zirro
Zirro commented Dec 18, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@Zirro Zirro left a comment

Choose a reason for hiding this comment

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

LGTM, with one suggested change.

Comment thread source
@domenic
Copy link
Copy Markdown
Member

domenic commented Dec 19, 2018

I wrote this commit message:

Fixes #4220, by making the algorithm consistent with the current
implementations in Blink and WebKit, as well as the existing web
platform tests. Now, innerHTML of void elements returns the empty string
even in cases where child nodes are present.

Tests (preexisting): https://wpt.fyi/results/html/syntax/serializing-html-fragments/serializing.html

But realized that you don't seem to be in the acknowledgments, @Zirro. Would you like to add yourself?

@Zirro
Copy link
Copy Markdown
Contributor Author

Zirro commented Dec 19, 2018

Thank you for asking, but I'm actually listed there under my real name after a previous contribution.

@domenic
Copy link
Copy Markdown
Member

domenic commented Dec 19, 2018

Ah thanks! I think my Ctrl+F was malfunctioning...

@domenic domenic merged commit d36d3eb into whatwg:master Dec 19, 2018
@jdm jdm mentioned this pull request Dec 19, 2018
Closed
@annevk
Copy link
Copy Markdown
Member

annevk commented Jan 2, 2019

Beautiful!

@Zirro Zirro mentioned this pull request Jan 28, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@annevk annevk annevk left review comments

@domenic domenic domenic approved these changes

Assignees

No one assigned

Labels

interop Implementations are not interoperable with each other topic: parser

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Zirro @annevk @domenic