Skip to content

fix: getAttribute and getAttributeNS should return null#477

Merged
karfau merged 5 commits into
xmldom:masterfrom
xiaopanshi:patch-1
Feb 25, 2023
Merged

fix: getAttribute and getAttributeNS should return null#477
karfau merged 5 commits into
xmldom:masterfrom
xiaopanshi:patch-1

Conversation

@xiaopanshi

@xiaopanshi xiaopanshi commented Feb 17, 2023

Copy link
Copy Markdown
Contributor

BREAKING CHANGE: Element.getAttribute and Element.getAttributeNS return null if the attribute doesn't exist

closes #46
https://dom.spec.whatwg.org/#dom-element-getattribute

base on https://dom.spec.whatwg.org/#dom-element-getattribute, getAttribute() and related methods should return null for not existing attributes. speediness  plea.
@karfau

karfau commented Feb 21, 2023

Copy link
Copy Markdown
Member

Thank you for the contribution.
I think your fix will also return null if an attribute is set to an empty string.
Can you verify my assumption by adding a test for that scenario and in case I'm right, fix it?

@xiaopanshi

Copy link
Copy Markdown
Contributor Author

Thank you for the contribution. I think your fix will also return null if an attribute is set to an empty string. Can you verify my assumption by adding a test for that scenario and in case I'm right, fix it?

yes, you're right, I'll try

@karfau karfau left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will have a closer look at the implementation of the related API like set and remove, to see if they are aligned with the spec, to make sure my suggested simplification doesn't break anything.

Comment thread lib/dom.js Outdated
@karfau karfau added spec:DOM-Level-2 https://www.w3.org/TR/DOM-Level-2-Core/ breaking change Some thing that requires a version bump due to breaking changes labels Feb 25, 2023
@karfau karfau added this to the 0.9.0 milestone Feb 25, 2023
@karfau

karfau commented Feb 25, 2023

Copy link
Copy Markdown
Member

I had a look at it and fond some more issues, but those will be covered in a follow up PR by me, after which I will create the next beta release.

Thank you for your contribution.

@karfau karfau changed the title fix: getAttribute does not return null fix: getAttribute and getAttributeNS should return null Feb 25, 2023
@karfau karfau merged commit 24108d4 into xmldom:master Feb 25, 2023
xiaopanshi added a commit to xiaopanshi/xmldom that referenced this pull request Feb 27, 2023
@xiaopanshi

Copy link
Copy Markdown
Contributor Author

My great pleasure.

xiaopanshi added a commit to xiaopanshi/xmldom that referenced this pull request Feb 27, 2023
karfau added a commit that referenced this pull request Mar 19, 2023
karfau added a commit that referenced this pull request Apr 2, 2023
karfau added a commit that referenced this pull request Apr 2, 2023
karfau added a commit that referenced this pull request Jun 9, 2023
- proper order in finding attributes (only affects duplicate attributes,
which can only be created manually and anyways produce invalid XML)
- properly remove item from previous last index (was not iterated over
but the reference was kept)
- set `ownerElement` to `null` even when `ownerElement.ownerDocument` is
not set (which is an edge case which doesn't happen when parsing XML)
- throw `DOMException` when there is no matching attribute
- add test coverage
- add doc comments regarding differences to the specification that still
exist:
  - no named property indices support
- add validation of qualified names in `Element` API which can throw
`DOMException` as declared in the specs
- always initialize required properties with `null` in `Attr`
constructor

BREAKING CHANGES: Mainly not throwing when removing an attribute that
doesn't exist and other behavior that was different from the
specification.

closes #46
see #477
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Some thing that requires a version bump due to breaking changes spec:DOM-Level-2 https://www.w3.org/TR/DOM-Level-2-Core/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

getAttribute() doesn't return null for not existing attributes

2 participants