Closed Bug 324378 Opened 14 years ago Closed 14 years ago

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

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

()

Details

(Keywords: compat)

Attachments

(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]
Fix

sr=jst
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]
Fix

>     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.

r=me
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"
Fixed.
Status: NEW → RESOLVED
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
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLContentSink.cpp&rev=3.182

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.