Closed Bug 305160 Opened 19 years ago Closed 19 years ago

Strange bubbling behaviour for DOM scroll events

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

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 fixSplinter 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

Created:
Updated:
Size: