#63824 closed defect (bug) (invalid)
button is not a self-closing HTML element and needs a closing tag
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | Priority: | normal | |
| Severity: | trivial | Version: | 4.8 |
| Component: | Media | Keywords: | |
| Focuses: | Cc: |
Description
The browser's parser will fix it, however it's better to provide valid HTML right away
https://github.com/WordPress/wordpress-develop/blob/trunk/src/js/media/views/uploader/window.js#L30
Change History (9)
This ticket was mentioned in PR #9458 on WordPress/wordpress-develop by @kkmuffme.
3 months ago
#1
- Keywords has-patch added
#2
follow-up:
↓ 4
@
3 months ago
- Keywords close added
This is a jQuery created element, and the value passed into jQuery to instantiate the element is created with closing tags. See this codepen for a very simplified example:
https://codepen.io/joedolson/pen/GgpOqar
The actual button element in this case doesn't have any text, because it's hidden and just used as a placeholder. But the created element is complete. jQuery is creating a DOM object, not raw HTML. This would only be self-closing if it was being inserted as HTML directly.
So there is no actual issue here.
#3
@
3 months ago
- Milestone Awaiting Review deleted
- Resolution set to maybelater
- Status changed from new to closed
- Version set to 4.8
Practically, the way this is authored works fine and doesn't cause problems today. I recommend not accepting the change.
I'd like to see movement on #59883 to remove support for XHTML before considering this. I agree with the idea of this change, but this is a single isolated case of a common pattern. If this moves ahead it establishes a precedent that the HTML used in similar jQuery snippets should be normative HTML, but the question "what is normative HTML?" is surprisingly complicated.
I agree with the assessment that there's no issue right now with the code as authored using <button type="button" class="browser" />.
However, it's also true that BUTTON is not a self-closing element, and the self-closing flag /> has no impact in HTML5. It's the same as passing <button type="button" class="browser"> to jQuery.
I agree with the change provided. It's normative HTML5 and unlikely to be confused with JSX where self-closing elements do exist.
<button /> was added in [40021] along with another example that I haven't found in the codebase today.
There are many examples of this pattern:
- Several in
src/js/_enqueues/admin/user-profile.js - Several in
src/js/_enqueues/wp/customize/controls.js - Several in
src/js/media/views/embed/url.js - etc.
Should all of these be updated to use normative HTML5? Making this change would set a precedent to update many of those JavaScript examples to use normative HTML5.
This is an interesting question and is related to HTML4 and XHTML support (see #59883). The self-closing flag is completely redundant in HTML5 and void elements are established by the standard. This means that normative HTML5 would use <input> and <br>, both self-closing elements. Unfortunately, those are a problem in XHTML where the self-closing flag /> or a close tag is required.
Some notes:
- Correctly closed elements are important in XHTML, unlike HTML5.
<button />and<button></button>are acceptable in both XHTML and HTML5.<button>is the same as<button />in HTML5 but invalid in XHTML (note that this is only true because the HTML ends after<button>). - I believe jQuery relies on
innerHTML. That will throw aSyntaxErroron XHTML pages in examples likeel.innerHTML = '<button>'. - It's not easy to serve XHTML today, it requires some combination of the correct HTTP headers, DOCTYPE declaration, and
xmlnson theHTMLelement. Data suggests that the vast majority of pages are not XHTML.
#4
in reply to:
↑ 2
@
3 months ago
Replying to joedolson:
This is a jQuery created element, and the value passed into jQuery to instantiate the element is created with closing tags. See this codepen for a very simplified example:
https://codepen.io/joedolson/pen/GgpOqar
The actual
buttonelement in this case doesn't have any text, because it's hidden and just used as a placeholder. But the created element is complete. jQuery is creating a DOM object, not raw HTML. This would only be self-closing if it was being inserted as HTML directly.
So there is no actual issue here.
This has nothing to do with jQuery, but this is how the browser's DOM parser works - it fixes invalid HTML. And jQuery just (ab)uses it.
Run this and you'll see you get a valid HTML too, just like with jQuery:
document.getElementById("demo").innerHTML = '<button type="button" />';
You'll also see that the browser will always remove the XHTML closing slash too.
So yeah, you can pass incomplete HTML, but it's always better to directly pass valid HTML to reduce the parser work.
Furthermore, with some tags (not this one), this is actually unsafe and allows for XSS attacks. Therefore it only makes sense to consistently enforce valid, complete HTML to be used.
XHTML is deprecated:
https://html.spec.whatwg.org/#the-xhtml-syntax
Using the XML syntax is not recommended, for reasons which include the fact that there is no specification which defines the rules for how an XML parser must map a string of bytes or characters into a Document object, as well as the fact that the XML syntax is essentially unmaintained
And it's removed by the browser anyway in all regular HTML cases. But even in XML button isn't a self-closing tag is it?
I'd like to see movement on #59883 to remove support for XHTML before considering this
Yeah me too, but this has nothing to do with it. The XHTML slash is removed by the browser automatically anyway
Bottom-line: keeping invalid, but working HTML is a bad practice.
I can't believe that this, minimal change that ensures compliance to the HTML standard even requires a discussion... If php-src contributors worked like that, we'd still be at PHP 5.6
#6
@
3 months ago
- Keywords changes-requested added; close removed
@kkmuffme I agree with you. This is HTML compliant and you even marked as trivial, because you know it's a minimal change. But as @jonsurrell has suggested, it is not going to cause problems as-is, 90% of WP is based on the philosophy "leave it, if it's not causing problems".
My question is, why not make a real effort and fix many (all) of them at once?
I always found it a pain in the back to create dozens of reports and commits for this kind of micro fixes that extend all over the code. It involves too many people for a minimal to no improvement (and the resources nowadays are under minimums, so spending people's time in refactorings is not best, while there are a massive number of high-severity tickets on the todo list).
#7
follow-up:
↓ 9
@
3 months ago
- Keywords has-patch changes-requested removed
- Resolution set to invalid
- Status changed from reopened to closed
In jQuery, $( '<element />' ) is just a shorthand for $( '<element></element>' ). jQuery will not add <element /> to the DOM, instead it uses createElement( element ) under the hood.
See https://github.com/jquery/jquery/blob/9c84195b9445645ba22f1c88d464d0b7d5ba22dd/src/core/parseHTML.js#L34 and https://github.com/jquery/jquery/blob/9c84195b9445645ba22f1c88d464d0b7d5ba22dd/src/core/var/rsingleTag.js#L3C25-L3C90.
#8
@
3 months ago
@ocean90 I think that this is exactly what @joedolson already suggested in the first comment, but the reason of why this was reopened is here.
According to @kkmuffme:
So yeah, you can pass incomplete HTML, but it's always better to directly pass valid HTML to reduce the parser work. Furthermore, with some tags (not this one), this is actually unsafe and allows for XSS attacks. Therefore it only makes sense to consistently enforce valid, complete HTML to be used.
I going to leave this closed for you to reconsider if it's worthy of reopening or not based on this premise.
#9
in reply to:
↑ 7
@
3 months ago
Replying to ocean90:
In jQuery,
$( '<element />' )is just a shorthand for$( '<element></element>' ). jQuery will not add<element />to the DOM, instead it usescreateElement( element )under the hood.
See https://github.com/jquery/jquery/blob/9c84195b9445645ba22f1c88d464d0b7d5ba22dd/src/core/parseHTML.js#L34 and https://github.com/jquery/jquery/blob/9c84195b9445645ba22f1c88d464d0b7d5ba22dd/src/core/var/rsingleTag.js#L3C25-L3C90.
I think that is true only if the element does not have any attributes.
If the element has attributes (as in <button type="button" class="browser" />), then jQuery calls buildFragment(), which then creates a div element and uses innerHTML:
Trac ticket: https://core.trac.wordpress.org/ticket/63824