Closed Bug 305160 Opened 19 years ago Closed 19 years ago

Strange bubbling behaviour for DOM scroll events

Categories

(Core :: DOM: Events, defect)

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

fix
4.52 KB, patch
jst
: review+
jst
: superreview+
asa
: approval1.8rc1-
jst
: approval-branch-1.8.1+
Details | Diff | Splinter Review 
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.
Martijn, if you're bored you might want to figure out how "onscroll" event
bubbling should work :-)
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?
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.
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.
I'd rather not add a window event handler, that doesn't seem consistent with DOM
events.


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.
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?
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.
(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...
I meant scrolled, my bad.
Attached patch fix — — Splinter Review 
This does what I said.

The way events are enabled for DOM capturing/bubbling could use a cleanup...
later.
Assignee: events → roc
Status: NEW → ASSIGNED
Attachment #193645 - Flags: superreview?(jst)
Attachment #193645 - Flags: review?(jst)
Comment on attachment 193645 [details] [diff] [review]
fix

Seems reasonable, yes. r+sr=jst
Attachment #193645 - Flags: superreview?(jst)
Attachment #193645 - Flags: superreview+
Attachment #193645 - Flags: review?(jst)
Attachment #193645 - Flags: review+
Has this patch landed?  I see r+sr, open trunk.
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.
checked in. Remind me to apply for branch approval next week.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
roc: branch is pretty much frozen now; 1.5b2 is in release candidates...
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 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.
Attachment #193645 - Flags: approval1.8rc1?
Can we get this verified on the trunk in a way that gives us confidence it
doesn't regress anything?
It's been on trunk for a week and no regressions have been attributed to it.
Martijn, would you care to verify?
> 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.
Keywords: verifyme
(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.

I'm not really sure what Asa wants. Maybe if you just check all the onscroll
testcases you know of? :-)
Comment on attachment 193645 [details] [diff] [review]
fix

Too late for non-critical changes.
Attachment #193645 - Flags: approval1.8rc1? → approval1.8rc1-
This isn't on the 1.8.1 branch, is it? So roc, maybe you should ask for 1.8.1 approval?
Comment on attachment 193645 [details] [diff] [review]
fix

I think this should go on the 1.8.1 branch.
Attachment #193645 - Flags: approval-branch-1.8.1?(jst)
Attachment #193645 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
Checked into MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
*** Bug 309996 has been marked as a duplicate of this bug. ***
Depends on: 359393
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: