Closed
Bug 1176313
Opened 10 years ago
Closed 10 years ago
Make Attr follow the spec again
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: dev-doc-complete, site-compat)
Attachments
(1 file)
This is basically about relanding bug 1075702. What needs to be done is a reversion of https://hg.mozilla.org/mozilla-central/rev/03682dcd82bb plus a resolution for bug 1165851, however we decide to resolve it. That decision is going to need the telemetry we're adding in bug 1175031.
Comment 1•10 years ago
|
||
The corresponding Blink bug, perhaps with a slightly wider scope, is https://code.google.com/p/chromium/issues/detail?id=502301
Comment 2•10 years ago
|
||
The use counter data from Blink is now reliable as it has reached stable:
https://www.chromestatus.com/metrics/feature/timeline/popularity/844
https://www.chromestatus.com/metrics/feature/timeline/popularity/845
Counter 845 is a proxy for the most problematic cases and its usage doesn't rise to the 0.0001% required to be visibily non-zero in the graphs. Per counter 844, only around 0.0015% of page views should be affected at all, so this has a good chance of working out, I think.
Comment 3•10 years ago
|
||
Spec bug: https://github.com/whatwg/dom/issues/72
![]() |
Assignee | |
Comment 4•10 years ago
|
||
Yeah, the telemetry from bug 1175031 is not on the release channel yet, but for the nightly/aurora/beta channels it's showing absolutely no instances of non-HTML elements in an HTML document having setAttributeNode called on them with a non-lowercase Attr that was created with createAttribute.
Olli, are you ok with the "make createAttribute lowercase" approach given that?
Flags: needinfo?(bugs)
Comment 5•10 years ago
|
||
"make createAttribute lowercase in HTML document"? Based on telemetry that should be fine.
51/8020000 ~= 0.00064%
Flags: needinfo?(bugs)
![]() |
Assignee | |
Comment 6•10 years ago
|
||
Attachment #8663244 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•10 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
Comment on attachment 8663244 [details] [diff] [review]
Reland the changes to make Attr handling follow the spec again. This is a relanding of bug 1075702, effectively
> Attr::GetValue(nsAString& aValue)
> {
> Element* element = GetElement();
> if (element) {
>- nsCOMPtr<nsIAtom> nameAtom = GetNameAtom(element);
>+ nsCOMPtr<nsIAtom> nameAtom = mNodeInfo->NameAtom();
> element->GetAttr(mNodeInfo->NamespaceID(), nameAtom, aValue);
> }
> else {
> aValue = mValue;
> }
>
> return NS_OK;
> }
>@@ -198,17 +198,17 @@ void
> Attr::SetValue(const nsAString& aValue, ErrorResult& aRv)
> {
> Element* element = GetElement();
> if (!element) {
> mValue = aValue;
> return;
> }
>
>- nsCOMPtr<nsIAtom> nameAtom = GetNameAtom(element);
>+ nsCOMPtr<nsIAtom> nameAtom = mNodeInfo->NameAtom();
So we don't have any callers for GetNameAtom anymore, want to remove it?
>- return Attributes()->RemoveNamedItem(aAttribute.NodeName(), aError);
>+ nsAutoString nameSpaceURI;
>+ aAttribute.NodeInfo()->GetNamespaceURI(nameSpaceURI);
>+ return Attributes()->RemoveNamedItemNS(nameSpaceURI, aAttribute.NodeInfo()->LocalName(), aError);
A bit annoying that we don't have nsIAtom* taking methods for this
>+++ b/dom/base/nsDOMAttributeMap.cpp
>@@ -295,72 +295,86 @@ nsDOMAttributeMap::SetNamedItemInternal(
> if (aError.Failed()) {
> return nullptr;
> }
>
> NS_ASSERTION(adoptedNode == &aAttr, "Uh, adopt node changed nodes?");
> }
>
> // Get nodeinfo and preexisting attribute (if it exists)
>- nsAutoString name;
>- nsRefPtr<mozilla::dom::NodeInfo> ni;
>+ nsRefPtr<NodeInfo> oldNi;
>+
>+ if (!aWithNS) {
>+ nsAutoString name;
>+ aAttr.GetName(name);
>+ oldNi = mContent->GetExistingAttrNameFromQName(name);
>+ }
>+ else {
Nit, } else {
>+ if (oldAttr) {
>+ attr = RemoveNamedItem(oldNi, aError);
>+ NS_ASSERTION(attr->NodeInfo()->NameAndNamespaceEquals(oldNi),
>+ "RemoveNamedItem() called, attr->NodeInfo() should be equal to oldNi!");
Not really about this bug, but I wonder what happens if mutation event listener does something mean here, like
move the to-be-set-attribute to some other element...
> // Add the new attribute to the attribute map before updating
> // its value in the element. @see bug 364413.
> nsAttrKey attrkey(ni->NamespaceID(), ni->NameAtom());
> mAttributeCache.Put(attrkey, &aAttr);
> aAttr.SetMap(this);
... looks like it will end up being in both mAttributeCaches (current and new element), but
attr.ownerElement will point to this element here.
Sure, not about this bug, but perhaps we should add a check after RemoveNamedItem that
aAttr's GetMap is still null (or 'this') and that its ownerDocument is still the right one.
Feel free to add such check in this bug or file a followup
Attachment #8663244 -
Flags: review?(bugs) → review+
![]() |
Assignee | |
Comment 9•10 years ago
|
||
> So we don't have any callers for GetNameAtom anymore, want to remove it?
Done.
> A bit annoying that we don't have nsIAtom* taking methods for this
I suppose we could add something taking namespace id and localname atom. Seems fairly edge-case.
> Nit, } else {
Fixed.
> Not really about this bug, but I wonder what happens if mutation event listener does
> something mean here, like move the to-be-set-attribute to some other element...
Seems like a spec issue, except the spec pretends mutation events don't exist. :(
Anyway, I added this inside that block:
+
+ // That might have run mutation event listeners, so re-verify
+ // our assumptions.
+ nsDOMAttributeMap* newOwner = aAttr.GetMap();
+ if (newOwner) {
+ if (newOwner == this) {
+ // OK, we're just done here.
+ return attr.forget();
+ }
+
+ // The attr we're trying to set got stuck on some other
+ // element. Just throw, for lack of anything better to do.
+ aError.Throw(NS_ERROR_DOM_INUSE_ATTRIBUTE_ERR);
+ return nullptr;
+ } else if (mContent->OwnerDoc() != aAttr.OwnerDoc()) {
+ // Got moved into a different document, boo.
+ aError.Throw(NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
+ return nullptr;
+ }
Let me know if you think it's off the rails somehow.
Comment 10•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 12•9 years ago
|
||
Posted the site compat doc: https://www.fxsitecompat.com/en-US/docs/2015/document-createattribute-now-lowercases-the-input/
Keywords: dev-doc-needed,
site-compat
Comment 13•9 years ago
|
||
Updated:
https://developer.mozilla.org/en-US/docs/Web/API/Document/createAttribute
and
https://developer.mozilla.org/en-US/Firefox/Releases/44#DOM_HTML_DOM
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•