Last Comment Bug 189308 - javascript <body onscroll> -event don't work with mouse scroll, mouse wheel or keyboard arrow
: javascript <body onscroll> -event don't work with mouse scroll, mouse wheel o...
Status: RESOLVED FIXED
[no l10n impact]
: fixed1.8, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 All
: -- major with 16 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Prashant Desale
Mentors:
: 186175 197435 208224 209916 212115 221452 224011 229291 254954 264588 291370 292046 293414 297482 297578 298166 301429 305609 309353 310387 312299 312545 (view as bug list)
Depends on: 316992
Blocks: 144868
  Show dependency treegraph
 
Reported: 2003-01-16 06:41 PST by Heikki Kniivilä
Modified: 2006-04-11 21:04 PDT (History)
50 users (show)
asa: blocking1.8b3-
asa: blocking1.8b5+
asa: blocking‑aviary1.5-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Cross-browser testcase (11.04 KB, text/html)
2004-01-28 14:53 PST, Gérard Talbot
no flags Details
Fire onscroll from nsScrollbarFrame (4.69 KB, patch)
2004-09-22 02:29 PDT, Andreas Lange
no flags Details | Diff | Splinter Review
Working patch (6.83 KB, patch)
2004-12-17 05:26 PST, Andreas Lange
roc: review-
Details | Diff | Splinter Review
Testcase, overflow and scripted scrolling (3.87 KB, text/html)
2005-01-11 00:44 PST, Andreas Lange
no flags Details
Testcase, smoothscroll crasher with patch (5.15 KB, text/html)
2005-04-20 00:18 PDT, Andreas Lange
no flags Details
patch, updated from "Working patch" (6.03 KB, patch)
2005-07-20 10:48 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
Event in nsScrollPortView::ScrollTo (not ok) (4.31 KB, patch)
2005-07-21 07:22 PDT, Andreas Lange
no flags Details | Diff | Splinter Review
fix (5.40 KB, patch)
2005-08-03 16:15 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix #2 (10.91 KB, patch)
2005-08-08 21:33 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
dbaron: review+
dbaron: superreview+
asa: approval1.8b4+
Details | Diff | Splinter Review

Description Heikki Kniivilä 2003-01-16 06:41:59 PST
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
Comment 1 Ruslan Ismailov 2003-01-16 08:14:16 PST
It is bug 35011, see from bug 35011 comment #76.

But it is direct dupe of bug 175301.
Comment 2 Tuukka Tolvanen (sp3000) 2003-01-16 08:34:49 PST
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?)
Comment 3 Tuukka Tolvanen (sp3000) 2003-01-16 08:35:25 PST
really reassign
Comment 4 Frank Wein [:mcsmurf] 2003-01-16 11:55:43 PST
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.
Comment 5 Aaron Leventhal 2003-01-16 21:28:48 PST
*** Bug 186175 has been marked as a duplicate of this bug. ***
Comment 6 Gérard Talbot 2003-03-10 11:51:50 PST
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?
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2003-03-23 14:06:25 PST
Mass-reassigning bugs to dom_bugs@netscape.com
Comment 8 Zibi Braniecki [:gandalf][:zibi] 2003-04-14 09:47:00 PDT
*** Bug 197435 has been marked as a duplicate of this bug. ***
Comment 9 Jo Hermans 2003-06-04 00:05:59 PDT
*** Bug 208224 has been marked as a duplicate of this bug. ***
Comment 10 Boris Zbarsky [:bz] 2003-06-19 13:02:34 PDT
*** Bug 209916 has been marked as a duplicate of this bug. ***
Comment 11 Sergey Fradkov 2003-06-20 02:00:15 PDT
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);
Comment 12 Gérard Talbot 2003-06-24 11:20:51 PDT
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.
Comment 13 Boris Zbarsky [:bz] 2003-07-08 22:27:18 PDT
*** Bug 212115 has been marked as a duplicate of this bug. ***
Comment 14 Jo Hermans 2003-10-07 03:49:16 PDT
*** Bug 221452 has been marked as a duplicate of this bug. ***
Comment 15 Boris Zbarsky [:bz] 2003-10-28 21:12:40 PST
*** Bug 224011 has been marked as a duplicate of this bug. ***
Comment 16 Gérard Talbot 2004-01-28 14:53:46 PST
Created attachment 140108 [details]
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.
Comment 17 Jesiah S 2004-02-09 12:41:21 PST
*** Bug 229291 has been marked as a duplicate of this bug. ***
Comment 18 Ivan Sagalaev 2004-04-01 01:33:51 PST
Almost missed this bug when trying not to file a dupe. Please, owner or
submitter, add the word 'wheel' to summary.
Comment 19 Kevin Ar18 2004-04-01 12:21:03 PST
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)
Comment 20 Gérard Talbot 2004-04-01 15:54:15 PST
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.
Comment 21 Zibi Braniecki [:gandalf][:zibi] 2004-04-02 03:58:22 PST
It's DOM lvl 0 for me
Comment 22 Zook Valem 2004-04-25 14:04:06 PDT
I'm seeing this problem in the iframe tag as well.
Comment 23 José Jeria 2004-08-09 15:08:15 PDT
*** Bug 254954 has been marked as a duplicate of this bug. ***
Comment 24 Andreas Lange 2004-09-22 02:29:53 PDT
Created attachment 159713 [details] [diff] [review]
Fire onscroll from nsScrollbarFrame

The easiest fix, move the onscroll event code from nsGfxScrollFrame to
nsScrollbarFrame. Not necessarily right though.
Comment 25 Andreas Lange 2004-09-22 02:37:49 PDT
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.
Comment 26 Andreas Lange 2004-09-28 13:33:12 PDT
Comment on attachment 159713 [details] [diff] [review]
Fire onscroll from nsScrollbarFrame

Got some feedback and tree changed. New patch later.
Comment 27 Andreas Lange 2004-10-05 09:12:03 PDT
It would be best to fire in nsGfxScrollFrameInner::CurPosAttributeChanged() when
mViewInitiatedScroll is true. That works for everything - except scrollbar
dragging... :-/
Comment 28 Andreas Lange 2004-10-08 05:45:38 PDT
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.
Comment 29 Alfonso Martinez 2004-11-09 14:27:07 PST
*** Bug 264588 has been marked as a duplicate of this bug. ***
Comment 30 Darren Winsper 2004-12-09 02:29:47 PST
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.
Comment 31 Andreas Lange 2004-12-17 05:26:39 PST
Created attachment 168956 [details] [diff] [review]
Working patch

Patch to fix scroll events, see comment 28 for discussion on implementation.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-12-20 20:23:22 PST
Andreas, what about elements that have overflow:hidden? They can still be
scrolled by script. Are they supposed to fire onscroll events?
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-12-20 20:24:21 PST
Also, it's a bit odd to have mLastScrollPosition be a point, since a scrollbar
only has a one-dimensional position
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-12-20 20:24:29 PST
Also, it's a bit odd to have mLastScrollPosition be a point, since a scrollbar
only has a one-dimensional position
Comment 35 Andreas Lange 2004-12-21 14:18:29 PST
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.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-12-22 13:45:12 PST
> I can check what Opera and IE does.

Thanks, please do.
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-12-29 14:11:46 PST
Comment on attachment 168956 [details] [diff] [review]
Working patch

minusing because I still have questions here
Comment 38 Andreas Lange 2005-01-11 00:44:18 PST
Created attachment 170901 [details]
Testcase, overflow and scripted scrolling

Modified attachment 140109 [details] to test scripted scrolling.
Comment 39 Andreas Lange 2005-01-11 00:59:26 PST
(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.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-01-31 17:34:23 PST
(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?
Comment 41 Isomorphic Software 2005-04-19 15:36:42 PDT
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.
Comment 42 Andreas Lange 2005-04-20 00:10:09 PDT
(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.
Comment 43 Andreas Lange 2005-04-20 00:18:45 PDT
Created attachment 181248 [details]
Testcase, smoothscroll crasher with patch

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.
Comment 44 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-04-21 15:09:45 PDT
*** Bug 291370 has been marked as a duplicate of this bug. ***
Comment 45 José Jeria 2005-04-27 00:43:52 PDT
*** Bug 292046 has been marked as a duplicate of this bug. ***
Comment 46 Gérard Talbot 2005-04-30 13:03:17 PDT
> 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.
Comment 47 Elmar Ludwig 2005-05-08 18:27:01 PDT
*** Bug 293414 has been marked as a duplicate of this bug. ***
Comment 48 Darin Fisher 2005-05-10 16:06:23 PDT
Any hope for a fix for Gecko 1.8 ?
Comment 49 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-05-10 16:59:57 PDT
could be...
Comment 50 kcoleman 2005-06-03 14:38:15 PDT
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
Comment 51 Zibi Braniecki [:gandalf][:zibi] 2005-06-04 12:27:37 PDT
do not ever set blocking flag to +/- unless you're driver.

If it's so important to you, remember, we're accepting patches.
Comment 52 José Jeria 2005-06-12 09:13:59 PDT
*** Bug 297482 has been marked as a duplicate of this bug. ***
Comment 53 :Gavin Sharp [email: gavin@gavinsharp.com] 2005-06-19 09:40:37 PDT
*** Bug 298166 has been marked as a duplicate of this bug. ***
Comment 54 Andreas Lange 2005-07-19 08:43:23 PDT
http://wiki.mozilla.org/Gecko:How_Scrolling_Works is very good for this bug.
Comment 55 Greg K Nicholson [:gkn] 2005-07-20 08:21:37 PDT
*** Bug 301429 has been marked as a duplicate of this bug. ***
Comment 56 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-07-20 10:48:06 PDT
Created attachment 189916 [details] [diff] [review]
patch, updated from "Working patch"

This is "working patch", updated to trunk.
It still crashes with https://bugzilla.mozilla.org/attachment.cgi?id=181248 -
Testcase, smoothscroll crasher with patch.
Comment 57 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-07-20 11:27:04 PDT
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.
Comment 58 Andreas Lange 2005-07-21 07:22:37 PDT
Created attachment 190022 [details] [diff] [review]
Event in nsScrollPortView::ScrollTo (not ok)

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?
Comment 59 timeless 2005-07-22 01:49:31 PDT
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)
Comment 60 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-08-03 02:22:13 PDT
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
Comment 61 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-03 16:15:48 PDT
Created attachment 191537 [details] [diff] [review]
fix

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.
Comment 62 Zibi Braniecki [:gandalf][:zibi] 2005-08-04 01:28:03 PDT
this fix works for me. Thanks roc! All testcases passed!
Comment 63 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-08-04 02:14:32 PDT
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.
Comment 64 Zibi Braniecki [:gandalf][:zibi] 2005-08-04 02:39:10 PDT
confirming. Crash with smoothscroll on. The event must be fired after the scroll
probably.
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-04 14:38:34 PDT
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.
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-04 17:30:34 PDT
(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.

Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-04 17:44:02 PDT
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.
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-08 21:33:15 PDT
Created attachment 192056 [details] [diff] [review]
fix #2

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.
Comment 69 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2005-08-15 16:49:56 PDT
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.)
Comment 70 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-15 22:09:33 PDT
(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.
Comment 71 Andreas Lange 2005-08-16 07:34:01 PDT
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 72 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2005-08-16 15:30:42 PDT
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
Comment 73 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2005-08-16 15:32:49 PDT
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*.)
Comment 74 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-17 15:52:01 PDT
(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...
Comment 75 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-17 21:55:16 PDT
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.
Comment 76 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-18 18:14:47 PDT
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.
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-21 16:38:09 PDT
checked in on trunk. I'll apply for branch approval in a couple of days.
Comment 78 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-21 21:17:05 PDT
My last checkin didn't have dbaron's comment changes. I just checked those in.
Comment 79 Dave Townsend [:mossop] 2005-08-23 02:52:52 PDT
*** Bug 297578 has been marked as a duplicate of this bug. ***
Comment 80 Dave Townsend [:mossop] 2005-08-23 06:10:40 PDT
*** Bug 305609 has been marked as a duplicate of this bug. ***
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-23 12:55:14 PDT
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.
Comment 82 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2005-08-24 13:37:26 PDT
Checked in on branch (original patch + followup changes for dbaron + bustage fix)
Comment 83 Gérard Talbot 2005-09-20 11:25:23 PDT
*** Bug 309353 has been marked as a duplicate of this bug. ***
Comment 84 José Jeria 2005-10-13 05:48:49 PDT
*** Bug 312299 has been marked as a duplicate of this bug. ***
Comment 85 Magnus Melin 2005-10-15 07:57:37 PDT
*** Bug 312545 has been marked as a duplicate of this bug. ***
Comment 86 Boris Zbarsky [:bz] 2006-04-11 21:04:05 PDT
*** Bug 310387 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.