Closed Bug 235441 Opened 20 years ago Closed 11 years ago

Capture event listeners fire on target (?!)

Categories

(Core :: DOM: Events, defect)

Product:
Core
Component:
DOM: Events
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: ian, Assigned: smaug)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files, 1 obsolete file)

TC with IMG as capture target
1.06 KB, text/html
Details
patch
40.54 KB, patch
Details | Diff | Splinter Review 
STEPS TO REPRODUCE
   1. Attach a capture listener to an element.
   2. Fire an event on the element.

ACTUAL RESULTS
   Listener is called.

EXPECTED RESULTS
   Nothing.

TESTCASE:
   http://www.hixie.ch/tests/adhoc/dom/events/capture/001.html

This works in Opera. (o125100)
> EXPECTED RESULTS
>   Nothing.

Why?  That's not what the DOM events spec says, as far as I can tell.

http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html#Events-flow-basic
clearly says:

  Each event has an EventTarget toward which the event is directed by the DOM
  implementation. This EventTarget is specified in the Event's target attribute.
  When the event reaches the target, any event listeners registered on the
  EventTarget are triggered.

and the capturing listener is certainly an event listener registered on the
EventTarget here.
But it also clearly says (wherever you look up "capturing") that capture event
listeners are only called during the capture phase, and that the capture phase
is only done to the target's ancestors.
And as far as I can tell, the quoted sentence above has been removed from DOM3
Events. See, in particular, this section, which I think is pretty explicit:

http://www.w3.org/TR/DOM-Level-3-Events/events.html#Events-listeners-activation-h4
Ah, interesting. It does say:

  A capturing EventListener will not be triggered by events dispatched directly to
  the EventTarget upon which it is registered.

I wonder whether this is an event retargeting issue.  That is, since the
original target is the textnode....
Depends on: 234455
This bug was originally found on an image onclick.
Ah, good.  Saves me writing a testcase.  ;)

This should really just get fixed as part of bug 234455...
*** Bug 242763 has been marked as a duplicate of this bug. ***
(FWIW I've received complaints from people about this.)
Flags: testcase+
A TC with IMG for Boris since I had one lying around :-)
How should we handle this when the element has an XBL binding and event is 
originated in the anonymous content?
If this is something you want to give feedback on you can do so on <public-webapi@w3.org>. The Web APIs WG is currently taking care of DOM Level 3 Events.
(In reply to comment #10)
> How should we handle this when the element has an XBL binding and event is 
> originated in the anonymous content?
> 
hixie or jonas, any comments to this?

It would be great if you could raise an issue with the webapi WG on this. From a logical point of view I can see both behaviours being useful, and mozs current behaviour less confusing. But compat with the DOM L2 spec would be bad to break for this.

Again, please raise an issue on the public list.
In the discussion it was discovered that DOM Level 2 Events unambiguously states that listeners registered with useCapture = true should not fire in the target phase, and that the DOM Level 2 Events test suite actually checks this.

So sounds like we should fix this... and audit all the addEventListener calls in our chrome that add capturing listeners.
But what to do with XBL? (comment #10)
Attached patch proposed patch (obsolete) — — Splinter Review 
If we want to do this, it should happen before 1.9a1, imo.
This may cause some regressions in chrome, but those should be easy to 
fix. Dialog buttons were the only ones I found easily.
(I'll go through all the addEventListener calls)
Assignee: events → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #223434 - Flags: superreview?(jst)
Attachment #223434 - Flags: review?(bugmail)
Comment on attachment 223434 [details] [diff] [review]
proposed patch

hmm, better to check all the addEventListener calls first
Attachment #223434 - Flags: superreview?(jst)
Attachment #223434 - Flags: review?(bugmail)
If/when fixing this, remember to check that Bug 342011 works too.
Marking as blocking, we need to decide what to do here. We don't follow the spec, but it's known that if we fix it sites will break.
Flags: blocking1.9? → blocking1.9+
> Marking as blocking, we need to decide what to do here.
> We don't follow the spec, but it's known that if we fix it sites will break.

Sites already break in Opera and in Safari when it didn't fired capturing event listeners in target.
The fix for the websites code is trivial - just make sure the proper documentation is then supplied to the authors.
This change should be listed in the changelog in bold.

jst, jonas, bz, what should we do with this?
If we decide to fix this, I can sure do it and fix all the callers in
chrome. 
We could perhaps take the fix and during a6 test how many sites 
break. The fix is after all just setting a simple flag in event dispatcher and something similar in nsDOMEvent::GetEventPhase, so backing it out is easy.
Though, I'm a bit worried that this might break quite many sites.
Yeah, testing this in alpha6 might not be a bad idea. Though i suspect we're not going to get that much testing until we release a beta.

I would really like to see us comply with the spec, and since the spec isn't going to change it seems likely that we would have to :(

Basically we should try to evangelize sites that break because of this, and figuring out which ones those are is best done by simply fixing this bug...

Interested to hear other peoples opinion.

Would it be possible to warn in FF2 every time we actually fire a capturing listener on the target? That seems like a good way of getting people to start fixing their code.
For what it's worth, I still think that as the spec is written it makes it difficult to attach a capturing listener on a subtree, because it won't fire for the root of the subtree.  But if the spec is really not going to change, I guess that's just life.  Not the first suboptimal thing in a spec...
You could take a look at Opera's browser.js where they are fixing some sites abuse of capturing listeners with a few lines of JavaScript. Apparently the main problem is sites are using window.addEventListener("load", ..., true). Since the DOM events spec doesn't cover the window object anyway it might make sense to make an exception for this case.
OS: Windows 2000 → All
Hardware: PC → All
The DOM specs do now cover the 'window' object:

   http://www.whatwg.org/specs/web-apps/current-work/#events0

Indeed, they explicitly say that the 'load' event does not fire at the 'window' object. I don't remember why we excluded the 'load' event, though.

bz: I do not remember hearing that argument (subtrees) brought up in the discussion. Could you bring it up on the group and see why the group wants to not allow it? I don't remember the arguments.
> bz: I do not remember hearing that argument (subtrees) brought up

http://lists.w3.org/Archives/Public/public-webapi/2006Mar/0300.html down near the bottom.  Starting with "One advantage of interpretation A..."

I don't think this point was actually seriously discussed.  In fact, I see no serious discussion in that whole thread, really.

I don't really have time to follow an e-mail thread in the next month or so; someone else would have to bring it up if something more than the initial post is expected.
I did some research for you. As far as I can tell the reasoning was that Safari and Opera did the no-capture-at-targets behaviour, and that matched the spec. However, Safari has since been forced to change back to the Gecko behaviour of firing capture handlers at targets for compatibility with Web pages. So maybe you should just ignore this bug until the spec changes...
For what it's worth, I do think we could do what Opera does -- have a permanent bubbling load listener on Window that enumerates the capturing load listeners and dispatches the event to them, and not fire capturing listeners at target.  That would be fully spec-compliant as far as I can see, and I suspect that would lead to very little breakage.
The subtree argument has been brought up several times iirc. Basically there are use cases for behaving either way. Ideal would have been if the argument was a bitfield rather than a boolean...
There is still the issue mentioned in comment #10. 
Right solution for that would be probably https://bugzilla.mozilla.org/show_bug.cgi?id=334216#c19, but unfortunately that
breaks lots of existing bindings in ff chrome.
Clarification: I believe capturing load events for window and capturing events on target are two separate issues. This bug is only about the latter issue (so my browser.js patches are irrelevant really).

At Opera we've had greater real-world problems with window load events and relatively few problems with following the spec for capturing listeners at target (think I've seen about a bug a year on it - now one of the problems was at spreadfirefox.com so you may want to fix that :-) ).

I think you should fix this bug. The spec for the window object is written carefully to be bug-compatible with Gecko not capturing load events on target, but what this bug is about is already spec'ed in detail in DOM2 Events, which is not likely to change.
(should have said "Gecko not capturing load events on *window*")
Given that Safari was forced to switch to the capture-on-target behaviour, it seems that there is some argument to say that the spec _should_ change. And it could change easily, we just need to say that it's changed and that's it...
How do you propose we get the spec changed? Last time I tried it was deemed too incompatible change compared to DOM Events L2
You'd have Apple's support, now.
AFAIK WebKit was "forced" to change to non-standard behaviour by one single site - live.com. Getting them to fix it should not be too hard, we know whom to talk to. (They were however concerned that the issue might cause problems on other Atlas-powered sites. I don't know if that is the case.)
This wouldn't be as simple change as I first thought. Handling XBL1 (comment #10) and native anonymous content in some reasonable way needs bigger changes.
We could leave XBL1 alone, but the handling of native anonymous content
would have to be fixed.
Like i said before. I don't really care how XBL1 behaves, as long as we don't break any XUL widgets. Same thing applies to native-anon content really.
Native anonymous is more important IMO, because that means basically form 
elements. Handling mousemove for example should be consistent everywhere, it
shouldn't depend on whether the element has native anonymous content or not.
Attached patch patch — — Splinter Review 
Fixes the bug, also with native anonymous content (but not with xbl).
Some changes to chrome and mochitests are needed. 
This is possibly too late for 1.9, but better to have at least the
full patch to help evaluation whether we really want this for 1.9.
Attachment #223434 - Attachment is obsolete: true
Attachment #273663 - Flags: review?(jonas)
I think we decided that we did not want to do this since it risks breaking too many sites?

Anyone opposed?
Comment on attachment 273663 [details] [diff] [review]
patch

Removing request for now. Smaug, feel free to mark this one WONTFIX, or rerequest review if you think we indeed want this.
Attachment #273663 - Flags: review?(jonas)
If WONTFIX, question of Bug 242763 will revive;
  (A) eventPhase property value when event target object
      "2"(Target Phase) was set in both event for true and for false.
  (B) Order of event scheduling was as follows when event target object 
      1.event handler, 2.event listener(true), 3.event listener(false)
I think following oreder/value is reasonable when event target object.
  1. event listner(true) with "Target Phase"
  2. event handler of the event target object
  3. event listener(false) with "Bubble Phase"
Yes, if we WONTFIX this bug reopening that bug seems like a reasonable thing to do.
I don't think we want this for 1.9. Would break too many sites.
But perhaps removing blocking1.9+ would be enough, and leave this open
for moz2.
QA Contact: ian → events
A note, since the spec has changed since the last bug activity:

(In reply to Maciej Stachowiak from comment #15)
> In the discussion it was discovered that DOM Level 2 Events unambiguously
> states that listeners registered with useCapture = true should not fire in
> the target phase, and that the DOM Level 2 Events test suite actually checks
> this.

The spec is less unambiguous today than it was then.  Between the 2009 and 2010 revisions,[1][2] the spec was changed so that the description of the useCapture parameter says:

    If true, useCapture indicates that the user wishes to add the event
    listener for the capture phase and target only, i.e. this event
    listener will not be triggered during the bubbling phase. If false,
    the event listener must only be triggered during the target and
    bubbling phases.

That section is (almost) identical in the most current revision today.  The addEventListener description itself still suggests the old behavior:

    Registers an event listener, depending on the useCapture
    parameter, on the capture phase of the DOM event flow or its
    target and bubbling phases. 

1. <http://www.w3.org/TR/2009/WD-DOM-Level-3-Events-20090908/#events-Events-EventTarget-addEventListener>
2. <http://www.w3.org/TR/2010/WD-DOM-Level-3-Events-20100907/#events-EventTarget-addEventListener>
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: