Closed Bug 642145 Opened 13 years ago Closed 13 years ago

'document-element-inserted' event is fired when inserting non-element nodes to the document node

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla5
Tracking Flags:
Tracking Status
blocking2.0 --- -
status2.0 --- .1-fixed

People

(Reporter: ochameau, Assigned: hsivonen)

References

Details

Attachments

(2 files, 1 obsolete file)

Check for elementness before firing the notification
1.25 KB, patch
sicking
: review+
christian
: approval2.0+
Details | Diff | Splinter Review
Chrome mochitest
3.06 KB, patch
sicking
: review+
Details | Diff | Splinter Review 
Here at Jetpack project we discover a bug around 'document-element-inserted' event fired throught nsIObserverService.

Here is a testcase that can be run directly in JS Console:

#########################################

var Cc = Components.classes; 
var Ci = Components.interfaces;

var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].getService(Ci.nsIWindowWatcher);
var win = ww.openWindow(null, "data:text/html,<!DOCTYPE%20html><h1>hello</h1>", "test", "chrome,centerscreen", null);

var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
obs.addObserver({
    observe: function (subject, topic, data) {
      Components.utils.reportError("Event : "+subject.location.href);
    }
  }, 'document-element-inserted', false);

#########################################

You will see two errors messages whereas we are waiting for only one.
And if you remove the "<!DOCTYPE%20html>" from the call to openWindow, and so load a non-HTML5 web page, you are going to receive only one event.


Here is the supposed stack trace:

nsHtml5TreeOperation::Perform
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#339

nsHtml5TreeOperation::AppendToDocument
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#250

nsContentUtils::AddScriptRunner
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#4738

nsDocElementCreatedNotificationRunner::Run
  http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#240

nsContentSink::NotifyDocElementCreated
  http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1665
I assume this would be the case for any standards doctype, right?  We don't have any "non-HTML5 web pages".

Henri, why would this differ between quirks and standards mode?
Component: General → HTML: Parser
QA Contact: general → parser
Summary: 'document-element-inserted' event is fired twice on HTML5 documents → 'document-element-inserted' event is fired twice on strict-mode documents
Is this a difference between quirks and standards or a difference between no doctype and any doctype?

What happens with <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN"> ?
(In reply to comment #2)
> Is this a difference between quirks and standards or a difference between no
> doctype and any doctype?
> 
> What happens with <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN"> ?

It is the same than <!DOCTYPE html>, you have two events fired.
Component: HTML: Parser → General
Component: General → HTML: Parser
OK. The problem is simply that http://hg.mozilla.org/mozilla-central/rev/34afbe32fb0e made the notification fire when *any* node is appended to the document. In addition to the root element, a doctype or comments may be appended to the document.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Summary: 'document-element-inserted' event is fired twice on strict-mode documents → 'document-element-inserted' event is fired when inserting non-element nodes to the document node
Attached patch Test with universalXPconnect (obsolete) — — Splinter Review 
What changes should I make to remove universalXPConnect? I tried to make this a chrome mochitest, but I failed to find the right boilerplate for the harness inclusion.
Attachment #520168 - Flags: feedback?(jonas)
You can get chrome mochitest boilerplate by running:

  testing/mochitest/gen_template.pl -type chrome -b 642145

Or were you looking for something else there?
(In reply to comment #7)
> You can get chrome mochitest boilerplate by running:
> 
>   testing/mochitest/gen_template.pl -type chrome -b 642145
> 
> Or were you looking for something else there?

I was looking for an HTML template. Do chrome-privileged mochitests have to be bootstrapped from XUL? (I already tried using the scripts from the XUL template, but they didn't work right in HTML.)
> Do chrome-privileged mochitests have to be bootstrapped from XUL?

Not sure, but it wouldn't surprise me. :(
Depends on: 579764
We should get this into Macaw so this new Firefox 4 feature works the way folks expect it to on the many DOCTYPE-wielding pages out there.
blocking2.0: --- → ?
Attached patch Chrome mochitest — — Splinter Review 

        Attachment #520168 -
        Attachment is obsolete: true

        Attachment #520168 -
        Flags: feedback?(jonas)

        Attachment #522624 -
        Flags: review?(jonas)

    
    

    
  
Hmm. Looks like the test case triggers bug 635548. :-( Need to experiment more with the test case. The fix itself is ready, though.

    
    

    
  
I suggest landing the fix separately from the test so that the fix can make it to Firefox 5 even if the test has to wait for bug 635548 to get fixed.

    
    

    
  
(In reply to comment #10)
> We should get this into Macaw so this new Firefox 4 feature works the way folks
> expect it to on the many DOCTYPE-wielding pages out there.

What's the status of Macaw? Does the fix for this patch have any chance of making it anymore? The deadline for Firefox 5 is also near.

    
    

    
  
Fix pushed to cedar:
http://hg.mozilla.org/projects/cedar/rev/119dd70688e2

Test *not* pushed per comment 13.
Flags: in-testsuite?
Whiteboard: [fixed-in-cedar]

    
    

    
  
This can't block macaw at this point, but you still have a couple of days to land on trunk and request mozilla2.0 approval to get it in.
blocking2.0: ? → -

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/119dd70688e2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2

    
    

    
  
Comment on attachment 520169 [details] [diff] [review]
Check for elementness before firing the notification

Requesting approval for 2.0 in reference to comment 10, comment 16 and comment 17.

        Attachment #520169 -
        Flags: approval2.0?

    
    

    
  
Comment on attachment 520169 [details] [diff] [review]
Check for elementness before firing the notification

Approved for Macaw but we won't block on it.

        Attachment #520169 -
        Flags: approval2.0? → approval2.0+

    
    

    
  
Marking as dependent of bug 635548, since that bug is blocking the landing of the test case here.

          status2.0:
          .1-fixed → ---
Depends on: 635548
Target Milestone: mozilla5 → ---
Version: Trunk → Other Branch

    
    

    
  
Shouldn't this be marked as "status2.0: .1-fixed"?

    
  

          status2.0:
          --- → .1-fixed
Target Milestone: --- → mozilla5

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: