Closed Bug 189308 Opened 22 years ago Closed 19 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: 19 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: