Closed Bug 324378 Opened 19 years ago Closed 19 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(
URL
)

Details

(Keywords: compat)

Attachments

(1 file)

Fix
5.11 KB, patch
sicking
: review+
jst
: superreview+
Details | Diff | Splinter Review 
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 Fix — — Splinter 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: 19 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: