305160 - Strange bubbling behaviour for DOM scroll events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Check this out:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericElement.cpp#2118

2118   if (lm &&
2119       !(NS_EVENT_FLAG_CANT_BUBBLE & aEvent->flags &&
2120         NS_EVENT_FLAG_BUBBLE & aFlags && !(NS_EVENT_FLAG_INIT & aFlags)) &&
2121       !(aEvent->flags & NS_EVENT_FLAG_NO_CONTENT_DISPATCH)) {
... code elided ...
2132     // We don't want scroll events to bubble further after it has been
2133     // handled at the local stage.
2134     if (aEvent->message == NS_SCROLL_EVENT && aFlags & NS_EVENT_FLAG_BUBBLE)
2135       aEvent->flags |= NS_EVENT_FLAG_CANT_BUBBLE;
2136   }

This means that whether the scroll event is allowed to bubble further depends,
in part, on whether the element has an event listener manager. In particular, in
the case where this element is the original target, if the element never had any
event listeners, then the event will bubble, otherwise it won't. This is clearly
incorrect...

The fix is presumably to move that code out of the "if (lm ...". Unfortunately
it's not clear to me under what circumstances the scroll event should bubble.
See bug 35011.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Martijn, if you're bored you might want to figure out how "onscroll" event
bubbling should work :-)

Comment 2

[Martijn Wargers (dead)]

When looking at bug 35011, comment 36 and further, you can conclude two things:
- IE does not bubble scroll events.
- The dom level 2 spec says that scroll events on a document level should bubble.
What the dom level 2 spec says doesn't seem to make much sense, see bug 35011,
comment 40.
For scroll events on an element level, the dom spec doesn't state anything.

Maybe this is something that thw whatwg will pick up at some time in the future.
Hixie?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, so you're saying that the viewport scrollbar should fire scroll events
targeted at the document? And that other scrollbars should fire scroll events
targeted at the element with the relevant 'overflow' property set. That makes
sense to me. In that case we need to change our implementation to never bubble
scroll events and target viewport scroll events at the document instead of at
the root element.

Comment 4

[Martijn Wargers (dead)]

Well, yes. That would make Mozilla compatible with IE6.
Except, IE6 doesn't do anything with document.onscroll, it only listens to
window.onscroll.
So I think window.onscroll should also work.
event.scrElement is null for scroll events in IE6, by the way.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd rather not add a window event handler, that doesn't seem consistent with DOM
events.

Comment 6

[Martijn Wargers (dead)]

You mean that you rather would not support window.onscroll?
But that would change Mozilla's behavior, see:
http://www.mozilla.org/docs/dom/domref/dom_window_ref72.html
I think many scripts rely on this to work.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reason "window" event handlers currently work (sometimes) is that if the
root element has no event handlers, the scroll event bubbles from root element
to the document and then to the window.

So the easiest way to get this to work would be to have viewport scroll events
target the document and bubble, and have element scroll events target the
element and not bubble. That would give us the maximum IE compatibility, but is
it too inconsistent?

Comment 8

[Hixie (not reading bugmail)]

If we assuming that bubbling events bubble from document to window, then it
seems reasonable for scroll events that bubble to be fired at the document if
the window is resized, and scroll events that don't bubble to be fired at
elements if they are scrolled. window.onscroll and document.onscroll should both
work.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> If we assuming that bubbling events bubble from document to window, then it
> seems reasonable for scroll events that bubble to be fired at the document if
> the window is resized,

Do you mean scrolled? We do fire onscroll if the window is resized if that
causes the window to scroll up, but that's a bit of an edge case...

Comment 10

[Hixie (not reading bugmail)]

I meant scrolled, my bad.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

This does what I said.

The way events are enabled for DOM capturing/bubbling could use a cleanup...
later.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 193645 [details] [diff] [review]
fix

Seems reasonable, yes. r+sr=jst

Comment 13

[Alex Vincent [:WeirdAl]]

Has this patch landed?  I see r+sr, open trunk.

Comment 14

[Martijn Wargers (dead)]

No, this hasn't been landed on trunk yet (and this should probably also land on
branch). Unfortunately, Robert isn't here until 6 october.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in. Remind me to apply for branch approval next week.

Comment 16

[Alex Vincent [:WeirdAl]]

roc: branch is pretty much frozen now; 1.5b2 is in release candidates...

Comment 17

[Martijn Wargers (dead)]

Robert, it is "next week", you should ask for branch approval.
Alex, you're right. But I think this is important to fix for 1.5. The remarks of
Robert in bug 189308, comment 81 also apply for this bug, and I think it is
rather important that this bug is also fixed because bug 189308 is fixed.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193645 [details] [diff] [review]
fix

This is a simple low-risk patch. It clearly only affects onscroll events. It
makes them much saner and more self-consistent and fixes a bug in a way that
makes us more Web-compatible.

Comment 19

[Asa Dotzler [:asa]]

Can we get this verified on the trunk in a way that gives us confidence it
doesn't regress anything?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's been on trunk for a week and no regressions have been attributed to it.
Martijn, would you care to verify?

Comment 21

[Gérard Talbot]

> Can we get this verified on the trunk in a way that gives us confidence it
> doesn't regress anything?

I'd be happy to verify whatever needs to be verified but I don't know how
exactly to verify what was fixed or what exactly needs to be verified. FWIW, I
can tell you that these 2 pages
http://www.gtalbot.org/DHTMLSection/WindowEventsNS6.html
http://www.gtalbot.org/BugzillaSection/Bug189308_ScrollEvent.html
work accordingly, as expected, as intended when tried with Seamonkey 1.1a
rv:1.9a1 build 2005101205 under XP Pro SP2 here.

Comment 22

[Martijn Wargers (dead)]

(In reply to comment #20)
> It's been on trunk for a week and no regressions have been attributed to it.
> Martijn, would you care to verify?
I can verify that this patch didn't cause any regressions that has been filed at
bugzilla, if that's what you mean.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm not really sure what Asa wants. Maybe if you just check all the onscroll
testcases you know of? :-)

Comment 24

[Martijn Wargers (dead)]

I tested these ones:
http://www.hixie.ch/tests/adhoc/dom/level0/001.html
https://bugzilla.mozilla.org/attachment.cgi?id=7344
https://bugzilla.mozilla.org/attachment.cgi?id=90436
https://bugzilla.mozilla.org/attachment.cgi?id=103366
https://bugzilla.mozilla.org/attachment.cgi?id=140108
https://bugzilla.mozilla.org/attachment.cgi?id=170901
https://bugzilla.mozilla.org/attachment.cgi?id=181248
These all work fine for me.
I've modified this demo to see if I could get it compatible with Mozilla -> also
works fine.
 
This patch also fixes the issue mentioned in bug 189308, comment 75, isn't it roc?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 193645 [details] [diff] [review]
fix

Too late for non-critical changes.

Comment 27

[Martijn Wargers (dead)]

This isn't on the 1.8.1 branch, is it? So roc, maybe you should ask for 1.8.1 approval?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193645 [details] [diff] [review]
fix

I think this should go on the 1.8.1 branch.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked into MOZILLA_1_8_BRANCH.

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 309996 has been marked as a duplicate of this bug. ***

Comment 31

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old