Closed Bug 1258205 Opened 4 years ago Closed 4 years ago

setAttribute doesn't throw InvalidCharacterError if the attribute already exists

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

()

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 3 obsolete files)

Attached file testcase
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160319030558

Steps to reproduce:

Create an element with an attribute which does not match the Name production, e.g.
> <div class="test" ~="123"></div>

Use setAttribute on that element with that attribute
> el.setAttribute('~', 'bad');


Actual results:

The value of the attribute changes.


Expected results:

An InvalidCharacterError should be thrown.

(https://www.w3.org/TR/dom/#dom-element-setattribute)
Attachment #8740491 - Flags: review?(amarchesini)
Attachment #8740491 - Attachment description: Make setAttribute, check if attribute name is valid even if it already exists → Make setAttribute check if attribute name is valid even if it already exists
Comment on attachment 8740491 [details] [diff] [review]
Make setAttribute check if attribute name is valid even if it already exists

Review of attachment 8740491 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +1180,5 @@
> +  aError = nsContentUtils::CheckQName(aName, false);
> +  if (aError.Failed()) {
> +    return;
> +  }
> +  

remove this extra space.
Attachment #8740491 - Flags: review?(amarchesini) → review+
NI to make sure this doesn't get lost :)
Assignee: nobody → oriol-bugzilla
Flags: needinfo?(oriol-bugzilla)
Removing space.
Attachment #8740491 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
Attachment #8740910 - Flags: review?(amarchesini)
Comment on attachment 8740910 [details] [diff] [review]
Make setAttribute check if attribute name is valid even if it already exists

Review of attachment 8740910 [details] [diff] [review]:
-----------------------------------------------------------------

Carrying forward r=baku
Attachment #8740910 - Flags: review?(amarchesini) → review+
does not apply cleanly:

patching file dom/base/Element.cpp
Hunk #1 FAILED at 1171
1 out of 1 hunks FAILED -- saving rejects to file dom/base/Element.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh setAttribute.patch

could you take a look, thanks!
Flags: needinfo?(oriol-bugzilla)
Keywords: checkin-needed
Sorry, my local code was reasonably recent so I didn't think it was necessary to update it. But it was, because the code just below my change was modified.
Attachment #8740910 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
Attachment #8740990 - Flags: review?(amarchesini)
Attachment #8740990 - Flags: review?(amarchesini) → review+
backedout for issues in w-p-t 2 tests like https://treeherder.mozilla.org/logviewer.html#?job_id=25874835&repo=mozilla-inbound
Flags: needinfo?(oriol-bugzilla)
If I understand correctly, the problem was that I didn't tell the test that now the expected result is pass instead of fail.
Attachment #8740990 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
Attachment #8741843 - Flags: review?(amarchesini)
Attachment #8741843 - Flags: review?(amarchesini) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f21863863d0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.