Closed Bug 189308 Opened 22 years ago Closed 20 years ago

javascript <body onscroll> -event don't work with mouse scroll, mouse wheel or keyboard arrow

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: heikkikk, Assigned: roc)

References

(Depends on 1 open bug)

Details

(Keywords: fixed1.8, regression, testcase, Whiteboard: [no l10n impact])

Attachments

(7 files, 2 obsolete files)

Cross-browser testcase
11.04 KB, text/html
Details
Testcase, overflow and scripted scrolling
3.87 KB, text/html
Details
Testcase, smoothscroll crasher with patch
5.15 KB, text/html
Details
patch, updated from "Working patch"
6.03 KB, patch
Details | Diff | Splinter Review
Event in nsScrollPortView::ScrollTo (not ok)
4.31 KB, patch
Details | Diff | Splinter Review
fix
5.40 KB, patch
Details | Diff | Splinter Review
fix #2
10.91 KB, patch
dbaron
: review+
dbaron
: superreview+
asa
: approval1.8b4+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3a) Gecko/20021212 javascript <body onscroll> -event don't work with mouse scroll, or keyboard arrow Reproducible: Always Steps to Reproduce: 1.make a web page with <body onscroll="javascript:alert('Hello');"> 2.Try scrolling with scroll bar 3.try scrolling with mouse scroll or keyboard cursor key Actual Results: nothing Expected Results: ;) Show the alert popup
It is bug 35011, see from bug 35011 comment #76. But it is direct dupe of bug 175301.
Confirming anyway, since bug 175301 is marked dup to bug 35011, and 35011 doesn't seem to be quite focused... Testcase from 175301: attachment 103329 [details]. linux trunk nightly 2003-01-16: onscroll only fired when manipulating scrollbar directly with the mouse. ->dom0 (right?)
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM Level 0
Ever confirmed: true
OS: Windows 2000 → All
really reassign
Assignee: rogerl → jst
QA Contact: pschwartau → desale
Whiteboard: DUPEME
ok no dupe. I thought this was fixed. But it seems, that this doesn't get/got fixed (also there was a patch). For me under Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030114 the onscroll event only fires, when i move the scrollbar with the mouse. No event gets fired when using arrow keys,pg up/down and the mouse wheel.
Whiteboard: DUPEME
*** Bug 186175 has been marked as a duplicate of this bug. ***
1- Attachment 90522 [details] is also a good testcase for this bug. 2- The keyword "regression" should be in there since when bug 35011 was fixed, we all tested the keys, mouse, mousewheel on all possible elements triggering a scroll event and everything (mouse, key, mousewheel) was working fine back around june 21st 2002. 3- This bug has to be a DUPLICATE of bug 156321 (see comment #6). Anyway, either way I don't care at all which bug gets RESOLVED as DUPLICATE... as long as the problem is solved. Agreed?
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
*** Bug 197435 has been marked as a duplicate of this bug. ***
*** Bug 208224 has been marked as a duplicate of this bug. ***
Keywords: regression
*** Bug 209916 has been marked as a duplicate of this bug. ***
Please, don't forget that the onscroll event is not fired not only for keyboard or mouse wheel, but also for program scrolling like this: top.frames['data'].scrollTo(x, y);
I'm using 1.4RC2 build 20030623 under XP Pro SP1 and I can not understand that this regression bug will not be fixed before final release of Moz 1.4. Right now, it appears that this bug will not be fixed before final release of Moz 1.4. FWIW, attachment 90522 [details] is working without a problem with NS 7.02 and with old releases of Mozilla 1.x.
Blocks: 144868
*** Bug 212115 has been marked as a duplicate of this bug. ***
*** Bug 221452 has been marked as a duplicate of this bug. ***
*** Bug 224011 has been marked as a duplicate of this bug. ***
Attached file Cross-browser testcase —
I'm loading a testcase in bug 151300 which also show that values for window.pageX/YOffset, window.scrollX/Y and document.documentElement.scrollLeft/Top are not updated on window.scroll*() methods.
Keywords: testcase
*** Bug 229291 has been marked as a duplicate of this bug. ***
Almost missed this bug when trying not to file a dupe. Please, owner or submitter, add the word 'wheel' to summary.
Summary: javascript <body onscroll> -event don't work with mouse scroll, or keyboard arrow → javascript <body onscroll> -event don't work with mouse scroll, mouse wheel or keyboard arrow
Is this a DOM 1 or DOM 2 bug? If so, then the following changes should be made: Component = DOM: Events Keywords += dom1 or dom2 (not sure which)
I think bug 35011 should have been DOM Events. This bug should be DOM extensions actually because pageX/YOffset, scrollLeft/Top, scrollX/Y are extensions. These values are not updated on a mousewheel roll, keyboard actions and mousewheel click-N-move; also the scroll events are not fired by mousewheel rolling, keyboard actions, mousewheel click-N-move too. I think the current summary is not accurately describing the bug.
It's DOM lvl 0 for me
I'm seeing this problem in the iframe tag as well.
*** Bug 254954 has been marked as a duplicate of this bug. ***
The easiest fix, move the onscroll event code from nsGfxScrollFrame to nsScrollbarFrame. Not necessarily right though.
Assignee: general → anlan
Status: NEW → ASSIGNED
Comment on attachment 159713 [details] [diff] [review] Fire onscroll from nsScrollbarFrame Onscroll is only fired for scrollbar usage. One simple solution for this is to move the event firing from nsGfxScrollFrame to nsScrollbarFrame. + Fires events for keys, scrollwheel or javascript scrolling + Works for iframes as well (try attachment 140109 [details]) + Less code, we don't need to dig for frame and content - Fires three times for a scrollbar click (one curpos, two newpos) due to some smoothscroll extras. I guess the last negative point here isn't acceptable? Possible solutions are a) Make nsScrollbarFrame query the scrollview and check if the pos actually changed, or b) a bit more complex restructuring in nsGfxScrollFrame.
Attachment #159713 - Flags: review?(dbaron)
Comment on attachment 159713 [details] [diff] [review] Fire onscroll from nsScrollbarFrame Got some feedback and tree changed. New patch later.
Attachment #159713 - Attachment is obsolete: true
Attachment #159713 - Flags: review?(dbaron)
It would be best to fire in nsGfxScrollFrameInner::CurPosAttributeChanged() when mViewInitiatedScroll is true. That works for everything - except scrollbar dragging... :-/
Roc, I would like your comments on this. After tracing how all these scrollpos values bounce around, I'm still a bit uncertain where to put this code. - nsGfxScrollFrameInner::CurPosAttributeChanged Your recommended location, where the event is fired currently. It is difficult to know here if we are called "for real" or just for extra syncing. The current flags are not enough for the onScroll event, and as lots of code tries to avoid going here unnecessarily it seems at bit fragile to trust this. Btw, the code as it is now fires the onScroll with bogus offset values if smooth scroll is enabled. :-) - nsScrollPortView::ScrollToImpl The place with total knowledge of what is going on. But I don't know how to dig out the content here, seems like a lot of unnecessary work. - nsScrollbarFrame::AttributeChanged As suggested before, fewest lines of code necessary here. Might need an extra GetScrollPosition() to avoid duplicate events (otoh, there is a duplicate one in CurPosAttributeChanged() so perhaps it evens out). I think that regardless of implementation, we will always fire two events when performing bi-directional scrolling as the scrollbars updates separately.
*** Bug 264588 has been marked as a duplicate of this bug. ***
This problem also occurs if you click somewhere in the empty space between the scrollbar and the directional arrow to quick-scroll, at least under Linux.
Attached patch Working patch (obsolete) — — Splinter Review
Patch to fix scroll events, see comment 28 for discussion on implementation.
Attachment #168956 - Flags: review?(roc)
Andreas, what about elements that have overflow:hidden? They can still be scrolled by script. Are they supposed to fire onscroll events?
Also, it's a bit odd to have mLastScrollPosition be a point, since a scrollbar only has a one-dimensional position
Assignee: anlan → general
Status: ASSIGNED → NEW
QA Contact: desale → ian
Also, it's a bit odd to have mLastScrollPosition be a point, since a scrollbar only has a one-dimensional position
QA Contact: ian → desale
I used a point as that is what I get out of GetScrollPosition(). Couldn't find any easy way to get a coord (possibly due to my limited architectural knowledge). The DOM2 spec says this: The scroll event occurs when a document view is scrolled. * Bubbles: Yes * Cancelable: No * Context Info: None DOM3 only specifies the target elements: HTMLDocument and HTMLElement. I guess overflow:hidden should fire events too if scrolled. I can check what Opera and IE does. Most uses of onScroll I have seen are "recoveries" from the user's scrolling action. Having cases when the event isn't fired (our current situation) will make such scripts pretty pointless.
> I can check what Opera and IE does. Thanks, please do.
Assignee: general → anlan
Comment on attachment 168956 [details] [diff] [review] Working patch minusing because I still have questions here
Attachment #168956 - Flags: review?(roc) → review-
Modified attachment 140109 [details] to test scripted scrolling.
(In reply to comment #32) > Andreas, what about elements that have overflow:hidden? They can still be > scrolled by script. Are they supposed to fire onscroll events? Opera disallows any scrolling (keyboard or script) when overflow:hidden, hence no onscroll events. IE behaves as Mozilla when overflow:hidden; removes scrollbars but still allows scripted scrolling. IE still fires onscroll events in this case. A difference is that IE will fire one event when scrolling in both directions while Mozilla will fire two.
(In reply to comment #39) > IE behaves as Mozilla when overflow:hidden; removes scrollbars but still > allows scripted scrolling. Right, we did that intentionally (and it was quite a bit of work!) for site compatibility. > IE still fires onscroll events in this case. A > difference is that IE will fire one event when scrolling in both directions > while Mozilla will fire two. But your patch won't work in this case, right?
One additional case to catch: If the user tabs to a focusable element such as a link, which is clipped by an overflow:"scroll" or overflow:"hidden" parent, the parent will scroll to show the focusable element. In this case onscroll should also be fired on the parent.
(In reply to comment #41) > One additional case to catch: Nice one. I think the previous patch will cover this, but how about providing a testcase? I think the basic change in the patch (move event creation out of donsGfxScrollFrameInner::CurPosAttributeChanged) is the right thing to do. However, I found a very rare corner case which could lead to crashes with the patch. Something leads to lost events and reentries in functions that have assertions for that. Haven't had the time to dig through all this again. :-/ Will try to sort it out again - some time - unless someone beats me to it.
So I don't lose it somewhere: add evil testcase which uses the onscroll event to open a new window and log window scroll positions. Will crash with the patch and smootscroll on. Obsolete patch as tree has changed a bit around here.
Attachment #168956 - Attachment is obsolete: true
*** Bug 291370 has been marked as a duplicate of this bug. ***
*** Bug 292046 has been marked as a duplicate of this bug. ***
> how about providing a testcase? > So I don't lose it somewhere: add evil testcase I added a link and an anchor in this interactive meta-testcase http://www.gtalbot.org/Bugzilla/Bug189308_ScrollEvent.html When clicking the link, MSIE 6 SV1 (on XP Pro SP2) will fire one (1) scroll event and Opera 8.0 final build 7561 will fire two (2) scroll events.
*** Bug 293414 has been marked as a duplicate of this bug. ***
Any hope for a fix for Gecko 1.8 ?
I'm working on a new product that will have a lot of usage. It works well in IE and I'd like it to work at least as well in Mozilla. This bug is one of the things blocking, so a fix for FF1.1 would be great
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.1+ → blocking-aviary1.1?
Flags: blocking1.8b3?
do not ever set blocking flag to +/- unless you're driver. If it's so important to you, remember, we're accepting patches.
*** Bug 297482 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b4?
Flags: blocking1.8b3?
Flags: blocking1.8b3-
*** Bug 298166 has been marked as a duplicate of this bug. ***
Whiteboard: [no l10n impact]
Flags: blocking1.8b4?
Flags: blocking1.8b4-
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1-
Blocks: 301429
*** Bug 301429 has been marked as a duplicate of this bug. ***
This is "working patch", updated to trunk. It still crashes with https://bugzilla.mozilla.org/attachment.cgi?id=181248 - Testcase, smoothscroll crasher with patch.
This is the assertion, I'm getting a lot: http://wargers.org/mozilla/bug189308/assertion.txt And this is the segv, causing the crash: http://wargers.org/mozilla/bug189308/segv.txt Any suggestions how to solve this? And the patch needs work on the issue in comment 33.
Thanks Martijn for updating this. I've left it and sort of hoped that the crash would be fixed by some other resolved event bug, but so far that hasn't happened. Looking at the crashlog, it seems like a listener picks up the event and tries to reposition the scrollbars again making things cyclic and unhappy. I haven't found any code that seems to do that though. Comment 28 discusses alternative locations from where to fire the event. I looked into nsScrollPortView. This patch is really simple, the problem is that not all events reach the html. I guess this is because the view/frame here isn't always the correct one (depending on whether we scroll with keys or scrollbar etc). It has the same crash problem with the testcase as the other patch. Roc, could you offer some guidance here?
mw: is mSmoothScroll null? i'm guessing it's clobbered on the way out (sorry i really don't have time to read any code, i'm trying to catch up on bugmail)
I don't have any experience with gdb. I hope this is correct. I get this info: (gdb) frame 0 #0 0x04f15e41 in nsScrollPortView::IncrementalScroll() (this=0xd869870) at c:/mozilla/mozilla/view/src/nsScrollPortView.cpp:746 746 mSmoothScroll->mFrameIndex++; (gdb) p mSmoothScroll warning: can't find linker symbol for virtual table for `nsScrollPortView' value $3 = (SmoothScroll *) 0x0
Attached patch fix — — Splinter Review
Try this patch. Pretty simple, seems to work okay, seems correct to me. If you guys can test this a bit I'll go for reviews.
Assignee: mozilla → roc
Status: NEW → ASSIGNED
this fix works for me. Thanks roc! All testcases passed!
Thanks roc! Unfortunately I get a crash with the patch when I try the testcase from bug 297482, which is https://bugzilla.mozilla.org/attachment.cgi?id=186026 This is the backtrace I'm getting: http://wargers.org/mozilla/bug189308/bt2.txt To reproduce, you have to have smootscrolling enabled. Scroll once with your mousewheel, it should trigger a couple of alerts Click on them, after a while clicking on them I get the crash.
confirming. Crash with smoothscroll on. The event must be fired after the scroll probably.
The problem is that onscroll pops up a modal alert, and then while in its event loop another smoothscroll timer fires, we pop up another alert inside the modal loop of the first, then another timer fires ... etc etc until we run out of stack space or something else bad happens. This is a tricky one. But somehow we do stop this problem in other cases. E.g., we stop setTimeout from firing while the page has an alert up. I just need to figure out how that works and apply it to onscroll.
(In reply to comment #65) > But somehow we do stop this problem in other cases. E.g., > we stop setTimeout from firing while the page has an alert up. Actually we don't. And in fact I can reproduce the recursion problem here with another testcase based on spawning alerts during timeouts.
Filed bug 303484 on that. I think probably we should go ahead and fix this, and try to address the crash issues in bug 303484.
Attachment #191537 - Flags: superreview?(dbaron)
Attachment #191537 - Flags: review?(dbaron)
Attached patch fix #2 — — Splinter Review
The fix for bug 303484 doesn't quite fix the issues in attachment 186026 [details] in conjunction with the previous version of this patch. The problem is that we survive the Attack Of The Alerts, but after we return from firing the initial scroll event in ScrollPositionDidChange, we crash in nsScrollPortView because mSmoothScroll is null --- the rest of the smooth scroll events already fired in the modal event loops inside this one. This sort of reentrancy during scrolling seems like a bad idea. Rather than try to handle it, this iteration of the patch fires the DOM onscroll event off a PLevent after scrolling has completed. This version of the patch also coalesces multiple pending onscroll events into one.
Attachment #191537 - Attachment is obsolete: true
Attachment #191537 - Attachment is obsolete: false
Attachment #191537 - Flags: superreview?(dbaron)
Attachment #191537 - Flags: review?(dbaron)
Attachment #192056 - Flags: superreview?(dbaron)
Attachment #192056 - Flags: superreview?
Attachment #192056 - Flags: review?(dbaron)
Attachment #192056 - Flags: review?
The comments in the code make it seem like this will mean we won't post scroll events for the cases where we used to do so. I didn't verify this, but could you either fix that or fix the comments? (e.g., the one above ScrollPositionDidChange and the one in CurPosAttributeChanged) Should the call be in InternalScrollPositionDidChange? (I'm not sure why one of the calls to that is conditional on |isSmooth|, though, at least based on the current comments.)
(In reply to comment #69) > The comments in the code make it seem like this will mean we won't post scroll > events for the cases where we used to do so. I didn't verify this, but could > you either fix that or fix the comments? (e.g., the one above > ScrollPositionDidChange We're calling PostScrollEvent() from ScrollPositionDidChange. > and the one in CurPosAttributeChanged) Not a problem. The comments there refer to what can change the scrollbar attributes, which is not the same as actual scrolling. Actual scrolling always goes through the view, which always issues a callback to ScrollPositionDidChange(). > Should the call > be in InternalScrollPositionDidChange? That's really just a setter for the scrollbar attributes. It doesn't indicate actual scrolling. Bad name.
Thanks for picking this one up roc! The reentrancy problems was a little bit over my head. :-) Atleast it flushed out bug 303484. fix #2 seems to meet all possible tests in the original testcase (attachment 140108 [details]). Whoho! I found one strange issue (focus-related?) with "Testcase, overflow and scripted scrolling" (attachment 170901 [details]). Press the first button to the left below the iframe, drag the scrollbar and then use scrollwheel over the iframe. No more scrollevents will be fired from now on, regardless of scrollmethod. Weird.
Comment on attachment 192056 [details] [diff] [review] fix #2 ok, r+sr=dbaron if: * you change "static void PR_CALLBACK" to "PR_STATIC_CALLBACK(void)" * you fix the comments that confuse me so they reflect reality
Attachment #192056 - Flags: superreview?(dbaron)
Attachment #192056 - Flags: superreview+
Attachment #192056 - Flags: review?(dbaron)
Attachment #192056 - Flags: review+
Also, ScrollEvent doesn't really need two copies of the nsGfxScrollFrameInner pointer -- you could just use event->owner (and thus even avoid the new class). (Previous comment applied to void and void*.)
(In reply to comment #71) > I found one strange issue (focus-related?) with "Testcase, overflow and scripted > scrolling" (attachment 170901 [details] [edit]). Press the first button to the left below > the iframe, drag the scrollbar and then use scrollwheel over the iframe. No more > scrollevents will be fired from now on, regardless of scrollmethod. Weird. The iframe scrolls but scroll events don't fire? That sounds bad. But I don't have a wheelmouse...
Okay, I tested this on a machine with a wheelmouse and indeed, something disturbing is happening. In attachment 170901 [details], if you do a scripted scrollTo and then scroll with the mousewheel, no more scroll events are ever fired on anything. (Dragging in the scrollbar is not required.) It appears to be a DOM event dispatch problem, since the code seems to be calling HandleDOMEvent. I need to investigate further.
I've located the bubbling problem with mousewheel. It's because of bug 305160, which I just filed ... the decision about whether to bubble the scroll event from the root element to the document is made in a rather bogus and unpredictable way. I will go ahead and land this patch on Monday. We may want to consider taking it on the branch too, because it's a potentially important DOM compatibility issue.
checked in on trunk. I'll apply for branch approval in a couple of days.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
No longer blocks: 301429
My last checkin didn't have dbaron's comment changes. I just checked those in.
*** Bug 297578 has been marked as a duplicate of this bug. ***
*** Bug 305609 has been marked as a duplicate of this bug. ***
Comment on attachment 192056 [details] [diff] [review] fix #2 No known regressions. This patch is important for Web compatibility and just general consistency of the platform.
Attachment #192056 - Flags: approval1.8b4?
Flags: blocking1.8b4- → blocking1.8b4+
Attachment #192056 - Flags: approval1.8b4? → approval1.8b4+
Checked in on branch (original patch + followup changes for dbaron + bustage fix)
Keywords: fixed1.8
*** Bug 309353 has been marked as a duplicate of this bug. ***
*** Bug 312299 has been marked as a duplicate of this bug. ***
*** Bug 312545 has been marked as a duplicate of this bug. ***
Depends on: 316992
*** Bug 310387 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: