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.

Sign up for GitHub

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

Jump to bottom

Attribute lowercasing does not appear to match browsers or web-platform-tests #31

Closed
domenic opened this issue May 3, 2015 · 2 comments
Closed

Attribute lowercasing does not appear to match browsers or web-platform-tests #31

domenic opened this issue May 3, 2015 · 2 comments

Comments

@domenic
Copy link
Member

domenic commented May 3, 2015

I implemented the current attribute spec in jsdom pretty exactly. However, it failed the following web-platform-test (source):

test(function() {
  var el = document.createElement("div")
  el.setAttribute("ALIGN", "left")
  assert_equals(el.getAttributeNS("", "ALIGN"), null)
  assert_equals(el.getAttributeNS("", "align"), "left")
  assert_equals(el.getAttribute("align"), "left")
}, "setAttribute should lowercase its name argument (upper case attribute)")

In my implementation null is returned for all three calls, because per spec setAttribute doesn't actually do any lowercasing.

If this is intentional we can just update web-platform-tests. I was unclear.
@domenic
Copy link
Member Author

domenic commented May 3, 2015

Thinking about this further it seems very unlikely the current spec is correct, since it's very natural that el.setAttribute("ALIGN", "left") + el.getAttribute("align") would work.

domenic added a commit to jsdom/jsdom that referenced this issue May 3, 2015
@domenic
Overhaul attribute handling (but not Attr itself)
881baef 
This re-does everything related to attributes in jsdom, except for the Attr class itself. It is now much more spec-compliant, as it follows the spec algorithms almost exactly.

The new living/attributes.js file contains line-by-line translations of the spec algorithms, including the abstract operations referenced from elsewhere in the spec. The new living/element.js adds the appropriate Element.prototype methods, which mostly delegate to these operations.

This allows us to remove a lot of crufty and often non-compliant code spread throughout jsdom. Notable highlights include:
- Our NamedNodeMap and AttributeList implementations in level1/core (and some in level2/core); see #922
- Our dual buildup of attribute-related methods in level1/core and level2/core
- The DOMAttrModified mutation event code in level2/events, including its AttributeList monkeypatches; it was simplified and moved to Element.prototype._attrModified in level1/core
- The StyleAttr class and many monkeypatches in level2/style; instead some tweaks were made to HTMLElement in level2/html

Fixes #922. Fixes #1109.

Almost passes web-platform-tests at https://github.com/w3c/web-platform-tests/blob/master/dom/nodes/attributes.html, but that includes tests for Attr being a non-Node, and also has the issue referenced at whatwg/dom#31.
@domenic domenic mentioned this issue May 3, 2015
Merged
domenic added a commit to jsdom/jsdom that referenced this issue May 3, 2015
@domenic
Overhaul attribute handling (but not Attr itself)
87641e9 
This re-does everything related to attributes in jsdom, except for the Attr class itself. It is now much more spec-compliant, as it follows the spec algorithms almost exactly.

The new living/attributes.js file contains line-by-line translations of the spec algorithms, including the abstract operations referenced from elsewhere in the spec. The new living/element.js adds the appropriate Element.prototype methods, which mostly delegate to these operations.

This allows us to remove a lot of crufty and often non-compliant code spread throughout jsdom. Notable highlights include:
- Our NamedNodeMap and AttributeList implementations in level1/core (and some in level2/core); see #922
- Our dual buildup of attribute-related methods in level1/core and level2/core
- The DOMAttrModified mutation event code in level2/events, including its AttributeList monkeypatches; it was simplified and moved to Element.prototype._attrModified in level1/core
- The StyleAttr class and many monkeypatches in level2/style; instead some tweaks were made to HTMLElement in level2/html

Fixes #922. Fixes #1109.

Almost passes web-platform-tests at https://github.com/w3c/web-platform-tests/blob/master/dom/nodes/attributes.html, but that includes tests for Attr being a non-Node, and also has the issue referenced at whatwg/dom#31.
@bzbarsky
Copy link

bzbarsky commented May 4, 2015

Yeah, as far as I can see the spec is wrong here. It should be lowercasing the name at least in some cases.

@annevk annevk closed this as completed in afe5b5e May 4, 2015
domenic added a commit to jsdom/jsdom that referenced this issue May 10, 2015
@domenic
Overhaul attribute handling (but not Attr itself)
47cef36 
This re-does everything related to attributes in jsdom, except for the Attr class itself. It is now much more spec-compliant, as it follows the spec algorithms almost exactly.

The new living/attributes.js file contains line-by-line translations of the spec algorithms, including the abstract operations referenced from elsewhere in the spec. The new living/element.js adds the appropriate Element.prototype methods, which mostly delegate to these operations.

This allows us to remove a lot of crufty and often non-compliant code spread throughout jsdom. Notable highlights include:
- Our NamedNodeMap and AttributeList implementations in level1/core (and some in level2/core); see #922
- Our dual buildup of attribute-related methods in level1/core and level2/core
- The DOMAttrModified mutation event code in level2/events, including its AttributeList monkeypatches; it was simplified and moved to Element.prototype._attrModified in level1/core
- The StyleAttr class and many monkeypatches in level2/style; instead some tweaks were made to HTMLElement in level2/html

Fixes #922. Fixes #1109.

Almost passes web-platform-tests at https://github.com/w3c/web-platform-tests/blob/master/dom/nodes/attributes.html, but that includes tests for Attr being a non-Node, and also has the issue referenced at whatwg/dom#31.
domenic added a commit to jsdom/jsdom that referenced this issue May 10, 2015
@domenic
Update attribute lowercasing after whatwg/dom#31
2399cc6
akhaku pushed a commit to akhaku/jsdom that referenced this issue May 12, 2015
@domenic @akhaku
Overhaul attribute handling (but not Attr itself)
0ad367b 
This re-does everything related to attributes in jsdom, except for the Attr class itself. It is now much more spec-compliant, as it follows the spec algorithms almost exactly.

The new living/attributes.js file contains line-by-line translations of the spec algorithms, including the abstract operations referenced from elsewhere in the spec. The new living/element.js adds the appropriate Element.prototype methods, which mostly delegate to these operations.

This allows us to remove a lot of crufty and often non-compliant code spread throughout jsdom. Notable highlights include:
- Our NamedNodeMap and AttributeList implementations in level1/core (and some in level2/core); see jsdom#922
- Our dual buildup of attribute-related methods in level1/core and level2/core
- The DOMAttrModified mutation event code in level2/events, including its AttributeList monkeypatches; it was simplified and moved to Element.prototype._attrModified in level1/core
- The StyleAttr class and many monkeypatches in level2/style; instead some tweaks were made to HTMLElement in level2/html

Fixes jsdom#922. Fixes jsdom#1109.

Almost passes web-platform-tests at https://github.com/w3c/web-platform-tests/blob/master/dom/nodes/attributes.html, but that includes tests for Attr being a non-Node, and also has the issue referenced at whatwg/dom#31.
akhaku pushed a commit to akhaku/jsdom that referenced this issue May 12, 2015
@domenic @akhaku
Update attribute lowercasing after whatwg/dom#31
74ca107
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@domenic @bzbarsky