Closed Bug 324378 Opened 14 years ago Closed 14 years ago

[FIXr]Attribute setting isn't always "first wins"


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






(Reporter: bzbarsky, Assigned: bzbarsky)




(Keywords: compat)


(1 file)

The net result of the changes for bug 213347 and bug 214577 is that in some cases the first attr doesn't win.  Example is in the URL bar -- none of the attributes in DOM inspector should have the value "b".

Fix coming up.
Attached patch FixSplinter Review
Attachment #209333 - Flags: superreview?(jst)
Attachment #209333 - Flags: review?(bugmail)
Keywords: compat
Comment on attachment 209333 [details] [diff] [review]

Attachment #209333 - Flags: superreview?(jst) → superreview+
Summary: Attribute setting isn't always "first wins" → [FIX]Attribute setting isn't always "first wins"
Comment on attachment 209333 [details] [diff] [review]

>     case eHTMLTag_head:
>       rv = OpenHeadContext();
>       if (NS_SUCCEEDED(rv)) {
>-        rv = AddAttributes(aNode, mHead, PR_FALSE, PR_TRUE);
>+        rv = AddAttributes(aNode, mHead, PR_FALSE, mHaveSeenHead);
>+        mHaveSeenHead = PR_TRUE;

You could always use PR_TRUE here, it would be slightly slower, but there's not usually that many head attributes. That way mHaveSeenHead isn't needed.

>       }
>       break;
>     case eHTMLTag_body:
>       rv = OpenBody(aNode);
>       break;
>     case eHTMLTag_html:
>       if (mRoot) {
>-        AddAttributes(aNode, mRoot, PR_TRUE, PR_TRUE);
>+        // If we've already hit this code once, need to check for
>+        // already-present attributes on the root.
>+        AddAttributes(aNode, mRoot, PR_TRUE, mNotifiedRootInsertion);

Same here, though we already do have the member so less of an advantage here.

Attachment #209333 - Flags: review?(bugmail) → review+
> You could always use PR_TRUE here, it would be slightly slower

It would also reverse the order of attributes on <head> every single round trip through the editor.  I thought that undesirable.  ;)

> Same here

Yeah, same issue here.

Reversing the attrs on the second and later nodes seems ok to me since it's a one-time fixup.  But I didn't want to break round-tripping in general.
Summary: [FIX]Attribute setting isn't always "first wins" → [FIXr]Attribute setting isn't always "first wins"
Closed: 14 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
Test checked in to mochitest.
Flags: in-testsuite+
The Mochitest for this bug fails with the HTML5 parser, because the HTML parser doesn't add the attributes of a second <head> tag to the head element.

The spec aligns with IE and Safari here; they don't add the attributes, either. Opera adds the attributes of a second <head> tag to the head element.

I tried to find the rationale for adding the attributes of a second <head> to the head element. The explicit code for doing that specifically for head was introduced by vidur without a bug number in

Do we want to keep the old behavior here (and get the spec changed) or shall I just weaken the test case not to test attributes of the head element? I favor weakening the test case and adopting the IE/Safari behavior, because it's hard to believe this to be a Web compat issue if IE and Safari get away with not implementing the special case.
I would be fine with that change.
OK. The change is tracked as bug 539882.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.