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.

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

HtmlNode.ToString(): fix self-closing tags on empty non-void elements #1413

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

stroborobo
Copy link
Contributor

Quoting from
https://www.w3.org/TR/2011/WD-html-markup-20110113/syntax.html#void-element

The following is a complete list of the void elements in HTML:
area, base, br, col, command, embed, hr, img, input, keygen, link,
meta, param, source, track, wbr

[...]

Void elements only have a start tag; end tags must not be specified
for void elements.
A non-void element must have an end tag, unless the subsection for
that element in the HTML elements section of this reference indicates
that its end tag can be omitted.

@stroborobo
Copy link
Contributor Author

Hey 👋

Noticed that ToString'd script elements aren't getting their closing tag and that broke some HTML I glued together. This fixes it according to the spec.

I wasn't sure where to put isVoidElement since it's static data, please let me know if I should move it.

@stroborobo
Copy link
Contributor Author

stroborobo commented Nov 19, 2021

Also isVoidElement is a duplicate, just noticed while trying to fix another issue. (Dropped white space in text nodes)

https://github.com/fsprojects/FSharp.Data/blob/5a5f3e0/src/Html/HtmlParser.fs#L774

Private helper module at the start of the file?

@stroborobo stroborobo force-pushed the fix-self-closing-tags branch from 6bf7e59 to d8b7e9a Compare November 19, 2021 23:38
@stroborobo
Copy link
Contributor Author

Added tests

@cartermp
Copy link
Collaborator

Looks like FSharp.Data.Tests.HtmlParser.Renderes self-closing tag for void elements() is a failure here.

@stroborobo
Copy link
Contributor Author

Oh right I fixed that locally and probably forgot to push again, will do once I'm at my desk again. Sorry, it's late 😅

@stroborobo stroborobo force-pushed the fix-self-closing-tags branch from d8b7e9a to bbf31f6 Compare November 20, 2021 14:27
Copy link
Collaborator

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Approved with these changes and pending green CI

@@ -647,6 +647,19 @@ let ``Renders textarea closing tag``() =

result |> should equal """<textarea cols="40" rows="2"></textarea>"""

[<Test>]
let ``Renderes self-closing tag for void elements``() =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let ``Renderes self-closing tag for void elements``() =
let ``Renders self-closing tag for void elements``() =

html |> should equal $"<%s{name} />")

[<Test>]
let ``Renderes no self-closing tag for non-void elements``() =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let ``Renderes no self-closing tag for non-void elements``() =
let ``Renders no self-closing tag for non-void elements``() =

Quoting from
https://www.w3.org/TR/2011/WD-html-markup-20110113/syntax.html#void-element

> The following is a complete list of the void elements in HTML:
> area, base, br, col, command, embed, hr, img, input, keygen, link,
> meta, param, source, track, wbr

[...]

> Void elements only have a start tag; end tags must not be specified
> for void elements.
> A non-void element must have an end tag, unless the subsection for
> that element in the HTML elements section of this reference indicates
> that its end tag can be omitted.
@stroborobo stroborobo force-pushed the fix-self-closing-tags branch from bbf31f6 to a3434dd Compare November 23, 2021 09:37
@stroborobo
Copy link
Contributor Author

Sorry for the late response, typos are fixed now.

@cartermp
Copy link
Collaborator

Thank you!

@cartermp cartermp merged commit 387a324 into fsprojects:main Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants