xul window onload event fired twice if iframe is contained

RESOLVED FIXED

Status

()

Core
XUL
--
major
RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: Karsten Düsterloh, Assigned: David Hyatt)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking-aviary1.5 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

16 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030305
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030305

When writing XUL pages with the root XUL elements window/page/dialog, the onload
event is fired twice if an iframe exists in it. If no iframe is given, the event
fires once. If the iframe itself has an onload handler, three events are
processed: one for the iframe, two for the root element.

This bug must actually be around at least a year now, it is the reason for bug
147068.

Reproducible: Always

Steps to Reproduce:
 



(Testcase follows.)
(Reporter)

Updated

16 years ago
Blocks: 147068
(Reporter)

Comment 1

16 years ago
Created attachment 116329 [details]
XUL window with iframe and onload alerts

same bug with page/dialog instead of window
Sounds like something broke and the onload event is bubbling....
(Reporter)

Comment 3

15 years ago
Bug 147068 has a simple workaround to circumvent this bug here, else bug 195885
would have been stuck. The workaround should be removed as soon as this bug here
is resolved.
(Reporter)

Comment 4

15 years ago
As Boris stated in a posting to npm.dom:
> load events do not bubble, per DOM2 Events.
> See
http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-eventgroupings-htmlevents
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 5

15 years ago
The onload events that are firing on the iframe and bubbling up to the window
don't have a target! The third event that fires on the window's document is
correctly targetted at that document.

Question: should you even be able to listen to a load of a frame using an
bubbling event handler on the frame's container element?
Per DOM spec, no.  Does it work with HTML?  I would not be one bit surprised if
this is a XUL hack...

Comment 7

15 years ago
OK, so using this html the main document actually loaded first - once...
Then the iframe loads, and its body event handler target is its document.
Then the iframe element onload handler fires, but with a null target.
I can reload the iframe which fires the same two load events.
hmm... That looks like in HTML the load event is in fact not bubbling up to the
window.  Not sure where the extra event comes from, though.
*** Bug 155188 has been marked as a duplicate of this bug. ***
Created attachment 134745 [details] [diff] [review]
Possible patch

This fixes the testcase in this bug and in bug 155188, by just syncing
nsXULElement to nsGenericElement (again, dammit).
Comment on attachment 134745 [details] [diff] [review]
Possible patch

bryner?  jst?  What do you think?  I have to say I'm not too happy with this
fix... I'd prefer to introduce a NS_EVENT_FLAG_CANT_CAPTURE and set that as
well as NS_EVENT_FLAG_CANT_BUBBLE on all these load/error events.  Then we can
just check for those flags here and not have to know about event types... Right
now it looks like CANT_BUBBLE events do in fact bubble and just don't get
processed during said bubble (I could be wrong here, though).

Oh, and as for the event with no target, that's the result of the code
athttp://lxr.mozilla.org/seamonkey/source/dom/src/base/nsGlobalWindow.cpp#883 (
 GlobalWindowImpl::HandleDOMEvent).  It creates an nsEvent (which hence has no
target) and which also has no prescontext (so GetTarget always returns null).
Attachment #134745 - Flags: superreview?(jst)
Attachment #134745 - Flags: review?(bryner)

Comment 12

15 years ago
Comment on attachment 134745 [details] [diff] [review]
Possible patch

Note that NS_IMAGE_LOAD is already "handled" at line 3057 (should the other
types go there too?) while the tabbed browser currently requires NS_IMAGE_ERROR
to bubble.
In fact, error events do bubble per the DOM spec.  Why don't we bubble them up?
 Is it to prevent window.onerror from firing for broken images?
Although, the spec talks about "error" as applying to script execution only...
too bad that's so out of touch with th reality of the past many years.  :(

Comment 15

15 years ago
Comment on attachment 134745 [details] [diff] [review]
Possible patch

I'm really confused now. I think this patch stops error events from bubbling,
which is not what I think you want. As for image load events, they can't bubble
or be captured (as per my previous reference) but I don't know what you want
there.
Neil, at the moment image error events bubble in XUL (and stuff depends on
that).  They do NOT bubble in non-XUL (and stuff may depend on that too).  We
need to resolve that issue.

I think we all agree this patch is not what we want in the tree.  It was just a
demonstration of where the problem lies.  The question is what behavior we _do_
want.

My suggestion is as follows:

1)  Load events should capture but not bubble.  This requires fixing whatever
    chrome capturing event listeners we have to stop sucking and check the
    target (last time I tried making this change Tp skyrocketed).  Even so, Tp
    may suffer due to added event propagation for all those images in webpages.
    Perhaps load events should not capture up the tree either, and we should fix
    chrome handlers that depend on them to? (I doubt we have many, or any, since
    they don't capture up the tree right now.)
2)  Error events should capture and bubble.
3)  Whether events capture/bubble should be controlled by flags per comment 11,
    so that the decision on whether it captures/bubbles or not only has to be
    made once.
bz, I'm all for introducing an NS_EVENT_FLAG_CANT_CAPTURE flag and eliminating
the event type checking in all the places where we can. I'd much prefer that
over the patch you attached... :-)
Comment on attachment 134745 [details] [diff] [review]
Possible patch

OK. Should we also change the behavior of the DONT_BUBBLE flag to actually skip
bubbling instead of just preventing handling while bubbling?
Attachment #134745 - Flags: superreview?(jst)
Attachment #134745 - Flags: review?(bryner)
I don't think we want to change how the bubbling works (bryner probably knows
more about this than I do tho), since IIRC there's code that cares about those
events during the bubbling stage (even if no registerd handlers are called) up
the tree, or in chrome, or somewhere.
Erk.  Could we fix that or something?  We should really not be having code that
cares about bubbling events that don't bubble....
Yeah, of course, assuming there's no good reason for such code (and assuming I'm
not dreaming things up here), but shouldn't the bubbling change be a separate bug?
Well... this bug is about the fact that "onload" events bubble in XUL.  They
should not be doing that.  So no, the bubbling change is exactly this bug.  The
capture change is what's sort of 'extra'.
Um, right. Then yeah, that should be fixed here :-)
Perhaps the right thing to do is to make the DONT_BUBBLE flag work as it does
now (bubble up, but don't get processed) and set it on the relevant events...

Then consider changing how the flag works in 1.7a?  bryner?  Thoughts?

Comment 25

14 years ago
*** Bug 244532 has been marked as a duplicate of this bug. ***

Comment 26

14 years ago
Confirmed in 1.7b under Linux, OS should be changed to "All".

Updated

14 years ago
OS: Windows 2000 → All

Comment 27

14 years ago
patching file `nsXULElement.cpp'
Assertion failed: hunk, file patch.c, line 321

abnormal program termination

Updated

14 years ago
No longer blocks: 258033

Comment 28

14 years ago
Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
Gecko/20040616

Comment 29

14 years ago
(In reply to comment #28)
> Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
> Gecko/20040616

Same here, with FireFox 1.0 on XP. My problem is a bit different tho, the onload
is triggered when I load content into an iframe...possibly related to the same bug.

Comment 30

14 years ago
(In reply to comment #29)
> (In reply to comment #28)
> > Confirmed to still exist in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7)
> > Gecko/20040616
> 
> Same here, with FireFox 1.0 on XP. My problem is a bit different tho, the onload
> is triggered when I load content into an iframe...possibly related to the same
bug.
> 
I managed a quick workaround to make it work for now in my project. Just
setAttribute the onload to 'null' after the first onload (assuming its calling a
js function), that keeps the window onload from triggering every single time I
try loading content in an iframe in the same window.

Updated

14 years ago
Flags: blocking1.8b?

Updated

14 years ago
Flags: blocking-aviary1.1?

Comment 31

14 years ago
Created attachment 172627 [details] [diff] [review]
Adapted patch for trunk

With that patch using the testcase the "window" alert is displayed only once.

Updated

14 years ago
Attachment #172627 - Flags: review?(dbaron)
Attachment #172627 - Flags: review?(dbaron) → review?(jst)
Comment on attachment 172627 [details] [diff] [review]
Adapted patch for trunk

r- See comment #16.
Attachment #172627 - Flags: review?(jst) → review-
Flags: blocking1.8b?
Flags: blocking1.8b2?
Flags: blocking1.8b-

Comment 33

13 years ago
*** Bug 288359 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Blocks: 294621

Comment 34

13 years ago
*** Bug 294621 has been marked as a duplicate of this bug. ***

Comment 35

13 years ago
no approved patch -> moving to beta3 nomination
Flags: blocking1.8b3?
Flags: blocking1.8b2?
Flags: blocking1.8b2-

Updated

13 years ago
Flags: blocking1.8b3?
Flags: blocking1.8b3-
Flags: blocking1.8b2-
Flags: blocking1.8b-

Updated

13 years ago
Flags: blocking-aviary1.1? → blocking-aviary1.1-

Comment 36

13 years ago
please contact certain vendors which are known to heavily rely on addEventListener('load',object,true) before you change the api such that all their code breaks permanently.

Comment 37

13 years ago
This should be now fixed (bug 234455), at least the test case wfm.
Please reopen if there is still something to fix here.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.