Skip to content

Conversation

@sreynen
Copy link
Contributor

@sreynen sreynen commented Apr 30, 2016

This is my test case:

const bel = require('bel');
console.log(bel`<input type="hidden" name="name" value="value" />`.toString());

Which currently outputs <input type="text" type="hidden" name="test" value="" /> when min-document is being used via global, within node.

I'm not sure where or why the type property is rendered as an attribute, but it seems like it should be set as an attribute rather than a property, which removes the duplicate rendered attribute.

This is my test case:

const bel = require('bel');
console.log(bel`<input type="hidden" name="name" value="value" />`.toString());

Which currently outputs <input type="text" type="hidden" name="test" value="" /> when min-document is being used (within node).

I'm not sure where or why the type property is rendered as an attribute, but it seems like it should be set as an attribute rather than a property, which removes the duplicate rendered attribute.
@yoshuawuyts
Copy link

I think this looks reasonable; If it's not too much trouble, could you perhaps add a test to catch future regressions? Thanks! ✨

@sreynen
Copy link
Contributor Author

sreynen commented May 2, 2016

I added a test, and also updated the test this had broken.

@mplatt
Copy link

mplatt commented Aug 2, 2016

@Raynos any chance we could get this in?

dom-element.js Outdated

if (this.tagName === 'INPUT') {
this.type = 'text'
this.setAttribute('type', 'text')
Copy link
Owner

Choose a reason for hiding this comment

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

👎 Please do both or only properties.

Properties are source of truth, attributes are just a weird bag of stuff.

@sreynen
Copy link
Contributor Author

sreynen commented Aug 3, 2016

If properties are the source of truth, then I guess bel needs to be updated (around https://github.com/shama/bel/blob/master/index.js#L92) to set type as a property rather than an attribute. Is there any documentation of which attributes rely on properties?

@sreynen
Copy link
Contributor Author

sreynen commented Aug 21, 2016

After looking at this more, I think I understand it better. Here's what I've found:

  1. What bel does works with browser DOM, so the change needs to happen in min-document.
  2. Specifically, what needs to change is the attribute type and the property type should be the same thing, on both set and get, as they are in browser DOM.
  3. And because min-document outputs both attributes and properties, only one of the two should ever get set to prevent duplicate type= output.
  4. As there are no getters and setters on properties that would allow setting attributes instead, this must go the other way, setting and getting the property any time the attribute is set or get.

Updated PR does that by changing DOMElement.prototype.setAttributeNS and DOMElement.prototype.getAttributeNS so they set and get the property rather than attribute for input type.

@Raynos
Copy link
Owner

Raynos commented Sep 21, 2016

This is great, thanks @sreynen

I like this solution.

@Raynos Raynos merged commit eb710ff into Raynos:master Sep 21, 2016
@Raynos
Copy link
Owner

Raynos commented Sep 21, 2016

Fixed in 2.19.0

@clehner
Copy link

clehner commented Oct 9, 2016

I ran into this bug and was directed to this PR. Could 2.19.0 be published to npm please?

@clehner
Copy link

clehner commented Oct 10, 2016

Never mind, my cache was stale. Carry on!

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.

5 participants