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

Handle attribute values that are boolean or blank #31

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@hassox

hassox commented Jan 3, 2016

When trying to serialize a DOMElement that has an attribute set to a boolean or is blank, the current behaviour doesn't handle this well.

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 5, 2016

Collaborator

The existing behavior seems more in line with how browsers generally behave—attribute values are coerced into strings, and you need removeAttribute to make them disappear. Outputting the attribute name alone for true also seems wrong for things that aren't actual boolean attribute.

Collaborator

mwiencek commented Jan 5, 2016

The existing behavior seems more in line with how browsers generally behave—attribute values are coerced into strings, and you need removeAttribute to make them disappear. Outputting the attribute name alone for true also seems wrong for things that aren't actual boolean attribute.

@hassox

This comment has been minimized.

Show comment
Hide comment
@hassox

hassox Jan 5, 2016

Unfortunately that isn't how the spec says they should behave (or any browser I've used).

Here's the relevant info:
http://www.w3.org/TR/2008/WD-html5-20080610/semantics.html#boolean

hassox commented Jan 5, 2016

Unfortunately that isn't how the spec says they should behave (or any browser I've used).

Here's the relevant info:
http://www.w3.org/TR/2008/WD-html5-20080610/semantics.html#boolean

@mwiencek

This comment has been minimized.

Show comment
Hide comment
@mwiencek

mwiencek Jan 5, 2016

Collaborator

I think we're confusing boolean properties with boolean attributes, since this code applies to both. Here's an example of where your patch behaves incorrectly:

var document = require('min-document');

var div = document.createElement('div');
div.setAttribute('data-foo', true);
div.toString();
// WRONG: got '<div data-foo></div>', expected '<div data-foo="true"></div>'

var input = document.createElement('input');
// The spec says if a boolean attribute is present, it should be set to either "" or the name of the attribute ("checked"), but browsers will let you set it to anything.
input.setAttribute('checked', true);
input.toString();
// WRONG: got '<input type="text" checked />', expected '<input type="text" checked="true" />'

Here's where your patch probably behaves correctly:

var document = require('min-document');

var input = document.createElement('input');
input.checked = true;
input.toString();
// OK?: got '<input type="text" checked />'

Note that browsers won't even serialize the checked property as an attribute like that, but it might make sense here.

Collaborator

mwiencek commented Jan 5, 2016

I think we're confusing boolean properties with boolean attributes, since this code applies to both. Here's an example of where your patch behaves incorrectly:

var document = require('min-document');

var div = document.createElement('div');
div.setAttribute('data-foo', true);
div.toString();
// WRONG: got '<div data-foo></div>', expected '<div data-foo="true"></div>'

var input = document.createElement('input');
// The spec says if a boolean attribute is present, it should be set to either "" or the name of the attribute ("checked"), but browsers will let you set it to anything.
input.setAttribute('checked', true);
input.toString();
// WRONG: got '<input type="text" checked />', expected '<input type="text" checked="true" />'

Here's where your patch probably behaves correctly:

var document = require('min-document');

var input = document.createElement('input');
input.checked = true;
input.toString();
// OK?: got '<input type="text" checked />'

Note that browsers won't even serialize the checked property as an attribute like that, but it might make sense here.

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