The default bug view has changed. See this FAQ.

'document-element-inserted' doesn't fire on ImageDocument

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ochameau, Assigned: krizsa)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jetpack:p2])

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
In Jetpack, we rely on this event to propose page-mod API, 
that allow developers to modify web contents.
Unfortunately, this event is not dispatched on images that have an ImageDocument instead of the regular HTMLDocument.

(FYI: 'content-document-global-created' works fine)

I was wondering if we can use a web standard event instead of document-element-inserted ? For example, listen to content-document-global-created and then watch for readystate change? Would it be equivalent ?
(Reporter)

Updated

6 years ago
Blocks: 668258
ImageDocuments aren't created by the HTML parser, so moving this to a more generic component. Need to check media and plug-in docs while at it.

document-element-inserted has no equivalent readyState transition. Furthermore, readyStates for image, media and plug-in docs are mostly bogus; bug 666134.
Component: HTML: Parser → DOM
QA Contact: parser → general
CCing sicking based on a recollection that he has worked on these events.
I did, but I doubt I'll have time to fix this anytime soon. Would love someone else to pick it up.
Eddy, Gábor: perhaps one of you could take this on?
(Assignee)

Comment 5

6 years ago
I can pick it up. sicking: could you give me some hints where to start with it?
Somewhere in the ImageDocument code there should be code that creates the elements that it contains. You'll have to modify that code to explicitly send the relevant notification.
(Assignee)

Updated

6 years ago
Assignee: nobody → gkrizsanits
(Assignee)

Comment 7

6 years ago
Created attachment 557851 [details] [diff] [review]
first attempt to fix the bug

Sicking: I added you as reviewer, if you don't have the time, could you suggest/CC someone else for review?
Attachment #557851 - Flags: review?(jonas)
Attachment #557851 - Flags: review?(jonas) → review+
Jonas: does this need sr, or is it ready for commission?
It's pretty simple, I don't think sr is needed
Keywords: checkin-needed
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
Version: unspecified → Trunk
In my queue with a few other bits that are being sent to try first and then onto inbound.

To save time for future patches, could you set your hgrc to include the author automatically & also add a commit message, along the lines of:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Thanks :-)
Keywords: checkin-needed
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=87ba0b8cbf99

https://hg.mozilla.org/integration/mozilla-inbound/rev/379147b5215f
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/379147b5215f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 689106
https://hg.mozilla.org/mozilla-central/rev/c62d1af37761

this was backed out in the above.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is the correct link for the backout diff:
http://hg.mozilla.org/mozilla-central/rev/00060b8e57f7
(Assignee)

Comment 15

6 years ago
Probably this can be avoided by simple firing this event only for ImageDocument, instead of MultimediaDocument (where I just discovered that the event will be fired by xbl utils anyway), but before I create a new patch for this I would like to understand what is going on in Bug 689106. Alex, do you need this event only for ImageDocuments or do you know about any other cases when you don't receive this event?
(Assignee)

Comment 16

6 years ago
Created attachment 565901 [details] [diff] [review]
fix - 2nd version

This seems to fix the issue we had with the first version. I still have to come up with a good test for this.
Attachment #557851 - Attachment is obsolete: true
Attachment #565901 - Flags: review?(bzbarsky)
(Reporter)

Comment 17

6 years ago
I opened this bug only for images documents. We are not aware of any other case where we lack this event. Did you verified that we receive document-element-inserted in case of video?
Because I don't see any other usecase of this event, except from HTML parser:
http://mxr.mozilla.org/mozilla-central/search?string=document-element-inserted

And now that you got more into this document creation code, do you have, by any chance, something to say about bug 668258 comment 3 question??
(Assignee)

Comment 18

6 years ago
(In reply to Alexandre Poirot (:ochameau) from comment #17)
> I opened this bug only for images documents. We are not aware of any other
> case where we lack this event. Did you verified that we receive
> document-element-inserted in case of video?
> Because I don't see any other usecase of this event, except from HTML parser:
> http://mxr.mozilla.org/mozilla-central/search?string=document-element-
> inserted

Good that you asked! Yes, I checked it and I was tricked at first (see my prev. comment) since for videos there was indeed a document-element-inserted event fired (in some case) BUT not for the video document. It was for some XML document created in the background for video utils (XBL stuff). So you will have to check the document in that event for addons probably. 

Also I just realized while double checking this, is that I do fire the event twice now, since SetScriptGlobalObject can be called with null as the global object, in which case we probably don't want to fire the event. Will correct the patch soon and I will have to write test for it anyway...

> 
> And now that you got more into this document creation code, do you have, by
> any chance, something to say about bug 668258 comment 3 question??

I digged into this a little I'll try to answer it on the other thread.
(Assignee)

Comment 19

6 years ago
Created attachment 565961 [details] [diff] [review]
some fixes for the previous version

I fixed the previous patch, a good test is still needed for it.
Attachment #565901 - Attachment is obsolete: true
Attachment #565901 - Flags: review?(bzbarsky)
Attachment #565961 - Flags: review?(bzbarsky)
Comment on attachment 565961 [details] [diff] [review]
some fixes for the previous version

Shouldn't MediaDocument::SetScriptGlobalObject call nsHTMLDocument::SetScriptGlobalObject?

Won't this misbehave (send a second notification) when the image document comes out of bfcache?  Presumably you want to make sure that the notification is only sent once for a given document.....
Attachment #565961 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 21

6 years ago
Created attachment 567070 [details] [diff] [review]
patch v4

(In reply to Boris Zbarsky (:bz) from comment #20)
> Won't this misbehave (send a second notification) when the image document
> comes out of bfcache?  Presumably you want to make sure that the
> notification is only sent once for a given document.....

I really couldn't find a way to ensure this without introducing a new bool member.
Attachment #565961 - Attachment is obsolete: true
Attachment #567070 - Flags: review?(bzbarsky)
Comment on attachment 567070 [details] [diff] [review]
patch v4

Yeah, bool member is fine here.

Maybe add a test for this?  Followup is fine.
Attachment #567070 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

6 years ago
Depends on: 697775
Blocks: 697775
No longer depends on: 697775
(Assignee)

Comment 23

6 years ago
Created attachment 572757 [details] [diff] [review]
final version with test

Here is an updated patch with test. Since this is the first time I attach a test, I thought it's might be better if some takes a look at it... Any feedback are welcome so I don't make the same mistake with the next tests I'm going to file soon. The code is the same as it was in the previous reviewed version.
Attachment #567070 - Attachment is obsolete: true
Attachment #572757 - Flags: review?(bzbarsky)
Comment on attachment 572757 [details] [diff] [review]
final version with test

This test may better belong in content/html/document/test.

Do you need a huge png image here?  If not, just us a small one!

You shouldn't be using enablePrivilege in new tests.  Does SpecialPowers not let you do what you want?

Using timeouts like this test does is a recipe for random orange; nothing guarantees that the media will load in 3 seconds.  Just make your document-element-inserted observer or observers drive the test progression instead.
Attachment #572757 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 25

6 years ago
Created attachment 573181 [details] [diff] [review]
fixed test

(In reply to Boris Zbarsky (:bz) from comment #24)
> Comment on attachment 572757 [details] [diff] [review] [diff] [details] [review]
I realized I can use resource files from other directories too so I don't have to add any new image, and I can put the test in the directory where it really belongs...
 
> You shouldn't be using enablePrivilege in new tests.  Does SpecialPowers not
> let you do what you want?
I was not aware of SpecialPowers, thanks, I changed it.
Attachment #572757 - Attachment is obsolete: true
Attachment #573181 - Flags: review?(bzbarsky)

Updated

5 years ago
Whiteboard: [jetpack:p2]
Comment on attachment 573181 [details] [diff] [review]
fixed test

>+        media.setAttribute("src",tests[0]);
>+        tests = tests.slice(1);

  media.setAttribute("src", tests.shift());

But there's a bigger issue here: this test will pass as long as document-element-inserted is called on _some_ documents.  It should be checking that it's being called on the right documents.
Attachment #573181 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 27

5 years ago
Created attachment 581254 [details] [diff] [review]
fixed test

In the test, the reason why I'm checking against media.contentDocument.location instead of doc.location, is that since I'm not in a chrome context I cannot access the properties of doc. But since I check that they are the same object, this should not be a problem.
Attachment #573181 - Attachment is obsolete: true
Attachment #581254 - Flags: review?(bzbarsky)
Comment on attachment 581254 [details] [diff] [review]
fixed test

>+++ b/content/html/document/test/test_document-element-inserted.html
>+        ok(media.contentDocument.location.toString().indexOf(loc) != -1);

This should have a test description as the scond argument, something like:

  "The loaded media should be " + loc

>+var observer = { observe: observe };

You probably don't need this.  You should be able to pass the |observe| function directly to add/removeObserver.

r=me with those nits.
Attachment #581254 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 29

5 years ago
Created attachment 581572 [details] [diff] [review]
final version

Here is the final version. Ed, could you check this in for me?
Attachment #581254 - Attachment is obsolete: true
Attachment #581572 - Flags: review+
Attachment #581572 - Flags: checkin?(bmo)
This is waiting on bug 709193. Also, has this been past try? :-)
Keywords: checkin-needed
Whiteboard: [jetpack:p2] → [jetpack:p2] [waiting on bug 709193]
(Assignee)

Comment 31

5 years ago
(In reply to Ed Morley [:edmorley] from comment #30)
> This is waiting on bug 709193. Also, has this been past try? :-)

This looks like a pass to me: https://tbpl.mozilla.org/?tree=Try&rev=4a33ea8a7a0a
The jetpack tests had some oranges but I checked with Alex and it's a none issue ( bug 705011 )
(Assignee)

Comment 32

5 years ago
So I misinterpreted the output of the try server, ignore my previous comment.
Keywords: checkin-needed
Whiteboard: [jetpack:p2] [waiting on bug 709193] → [jetpack:p2]
Attachment #581572 - Flags: checkin?(bmo)
https://tbpl.mozilla.org/?tree=Try&rev=52aa1b06d4c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/49481d05bd7c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla9 → mozilla11
Whoops, pre-emptive FIXED, need to get back into inbound mode, after our spell in m-c :-)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/49481d05bd7c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.