Skip to content

Conversation

@fangel
Copy link
Collaborator

@fangel fangel commented Jan 29, 2015

Hi,

I started out by adding test-cases for the serialisation, and found a few issues once I compared to how browsers (ehh, phantomjs) does it. I found a few discrepancies that I have addressed. Additionally I ensured that elem.[attr] is equivalent to elem.getAttribute("[attr]").

So the new or fixed features are:

  • elem.[attr] is equivalent to elem.getAttribute("[attr]")
  • elem.style.backgroundColor and other CSS properties that include hyphens are now serialised correctly
  • Namespaced attributes are now serialised correctly (I implemented it wrong in the previous PR)
  • Void-elements are now serialised as void-elements - ie, they do not have an end-tag. This is only done if the documents namespace is the HTML namespace.

Known differences between min-document and browsers:

  • document.createElement('input').getAttribute('type') is null in browsers, but text in min-document. This is because in both browsers and min-document, document.createElement('input').type is text - ie, in browsers there is a difference between .type and .getAttribute('type');.
  • document.createElement('input') is serialised as <input> in browsers, and <input type="text"> in min-document. This is the same issue as the previous discrepancy.

Kind regards
Morten

Attributes can now be get and set through both elem.[attr] or elem.getAttribute("[attr]"). Added test-case that shows this behaviour
Ie elem.style.backgroundColor="xxx" now translate to style="background-color: xxx; ". Test-cases added to test this behavior
The namespace-id shouldn't be part of the attribute-name. Test-case updated
@Raynos
Copy link
Owner

Raynos commented Jan 29, 2015

elem.[attr] is equivalent to elem.getAttribute("[attr]")

that's not how the DOM works. elem.[propertyName] and elem.getAttribute('[attrName]') are completely different.

Copy link
Owner

Choose a reason for hiding this comment

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

We cannot write to this. We must write to this._attributes['default-namespace'] or something. We can even have this._attributes and this._nsAttributes

@Raynos
Copy link
Owner

Raynos commented Jan 29, 2015

Other then props vs attributes the rest looks good to me.

@fangel
Copy link
Collaborator Author

fangel commented Jan 30, 2015

Well, at least for HTML documents, and supported attributes is does.

E.g

el = document.createElement('div')
el.id = "foo"
el.getAttribute("id") === "foo" // <- true

I considered having a white-list of attributes that if the namespaceURI was the html-namespace, then have this behaviour, but I figured it would start pushing this project away from being a minimal DOM implementation.

But if you don't want this behaviour, then I'll remove that commit, add a commit that fixes serialisation of things like

el = document.createElement('div')
el.foo = "bar"
el.setAttribute("foo", "baz")

and force-push to this feature-branch to update the PR. The problem with the code above is that it would serialise to <div foo="bar" foo="bas"></div>. Do you have any input on what this should serialise to?

-M.

@Raynos
Copy link
Owner

Raynos commented Jan 31, 2015

@fangel

The problem you are running into is the need to have a property -> attribute mapping. Because x.className serializes as <x class=...>

You also need deduping, i.e. if an attribute is written once dont write it again.

I recommend:

  • change the serializer and add a deduper to not double serialize props and attributes
  • ignore the x.id = 'foo'; x.getAttribute('id') edge case. Anyone setting with props and reading with attributes deserves what they get.

However let's not simplify the problem down to "attributes and properties are the same" that is a wrong implementation. I'd rather have an incomplete or subset implementation then a wrong one.

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.

3 participants