Closed Bug 312831 Opened 19 years ago Closed 18 years ago

scroll wheel scrolls page after reaching end of a <select> box

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: nick.langille, Assigned: masayuki)

References

(Depends on 1 open bug, )

Details

(Keywords: polish)

Attachments

(4 files, 13 obsolete files)

14.60 KB, image/png
Details
1.07 KB, patch
Details | Diff | Splinter Review
10.86 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
6.66 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051016 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20051016 Firefox/1.6a1

When a <select> box contans enough options to be scrollable, and I use the
scrollwheel to scroll through the options, the page starts to scroll once it
reaches the top or bottom of the box.

Reproducible: Always

Steps to Reproduce:
1. Put the cursor over a scrollable select box
2. Scroll to the bottom of it with the mousewheel and keep scrolling

Actual Results:  
The page scrolls.

Expected Results:  
The page should not scroll when the mouse is over a select box.
We should also stick with the hover-context from where the scroll started so
that we don't do that freaky thing where you start scrolling the page, then hit
a select box and scroll through that, then go back to scrolling the page. If you
started scrolling the page, then until the scroll action stops, you should only
be scrolling the page. Same goes for textareas - if that's the context of the
start of the scroll, then we shouldn't bleed the scroll outside of that.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Whiteboard: ui-polish
This change was introduced in bug 97283. Specifically, see bug 97283 comment 117
and bug 97283 comment 132.
Assignee: nobody → events
Status: ASSIGNED → NEW
Component: General → DOM: Events
OS: Linux → All
Product: Firefox → Core
QA Contact: general → ian
Hardware: PC → All
Version: unspecified → Trunk
So the first question here is:  What behavior do we actually want?  Should this behavior be hardcoded in Gecko or up to the app?
This behavior is hardcoded on ESM. The wheel event passes to parent element if it cannot scroll.
Yes, I know what we do now.  The question is what we _should_ be doing.
(In reply to comment #5)
> Yes, I know what we do now.  The question is what we _should_ be doing.

Ah, sorry. I think it should hardcoded on ESM.
I think we should not change current behavior. Because the focused scroll area doesn't capture the wheel event on Gecko.

I think we should know what is trouble by current behavior. The reporter didn't say the reason.
Leaving it up to the app is problematic:  this bug reads to me as being mainly about HTML controls (select, textarea).  It thus may become a "How do other browsers react" issue.

XUL applications such as ESM run by a different set of rules: they're Mozilla-specific, so I think that case should be handled separately.

Clarification requested:  I don't recall hearing of a wheel event.  Are we talking about the scroll event from DOM 2 Events, or something else?
(In reply to comment #6)
> I think we should know what is trouble by current behavior. The reporter didn't
> say the reason.

Here is my main irritation, and it happens in IE as well, not sure about Opera:

I move my mouse onto the page and roll the mouse wheel to scroll. As expected the page scrolls nicely until suddenly it stops because my pointer is now over a textbox/listbox. Now that scrolls till its at the end at which point the page scroll resumes.

Very annoying when all you want to do is scroll the page, and likewise so when all you want to do is scroll the select box and suddenly it vanishes off screen because you went too far.
WinIE6:
textarea and overflowed box have same behavior as Gecko.
select box doesn't so. because WinIE6's select box is native widget, therefore, it only scrolls when it has focus.
so, WinIE6 has this issue on textarea and overflowed box.
# WinIE7 will use non-native widget for select box. but I cannot test it.

Opera8:
all box is only scrolls when it has focus. so, opera doesn't have this issue.
> all box is only scrolls when it has focus.

all boxes scroll only when it has focus.

Sorry.
(In reply to comment #8)
> Very annoying when all you want to do is scroll the page, and likewise so when
> all you want to do is scroll the select box and suddenly it vanishes off screen
> because you went too far.

I think that whether the behavior is loved is influenced by user's liking or user's environment. Because on GTK2, non-focused widget can be scrolled.

Should be the behavior changeable by pref?
(In reply to comment #11)
> I think that whether the behavior is loved is influenced by user's liking or
> user's environment. Because on GTK2, non-focused widget can be scrolled.
> 
> Should be the behavior changeable by pref?

I don't think whether the widget is focused is the issue. I hope the fix for this is not to only scroll the focused widget. I want to be able to move to something and scroll and that widget then receive all scroll events until I have finished scrolling. Of course thats somewhat difficult to determine, possibly using one of how long since I last scrolled and if I've moved the mouse pointer.
Attached patch Patch rv1.0 (obsolete) — Splinter Review
Do you want this behavior? I don't think that this behavior is usable (or natural).
Because page scroll is stopped on scrollable box. So, we cannot ensure that we can scroll the page on every point. This is very irritating.
As I see it there are a couple of issues:
a) whether to scroll based on focus or mouse pointer. I don't generally have access to a scroll wheel on this laptop, but when I do use a wheel I am continually annoyed that it doesn't scroll what I point at (at which point if I can I use autoscroll instead). Note also that our XUL scrolling emulation in listboxes, menupopups and trees all scroll based on the pointer.
b) what happens when more than one scrollable object is available in the hierarchy of elements and frames under the pointer. I thought there was a case for only scrolling the innermost object, as then the object doesn't move unexpectedly, but that doesn't solve the case of a scrollable object that wasn't originally under the pointer getting scrolled so that it is.
A possible compromise would be to synchronise scrolling and focus so that if the focused object is not under the mouse then focus a new scrollable object.
FYI:

We have haven a report that is bug 33732. Bug 33732 was very unpopularity bug then.
Attached patch Patch rv2.0 (proposal) (obsolete) — Splinter Review
I have an idea for this issue.

I think that many users may decide to scroll the target view at *stopping* the cursor. Therefore, I think that the mouse wheel scrolling transaction begins at turning the wheel, and ends at moving the mouse (or pressing mouse buttons or pressing the key of keyboard).

This patch keeping the wheel scrolling target on the transaction.

Who can review this patch and who can decide the way?
Attachment #222404 - Attachment is obsolete: true
Comment on attachment 223028 [details] [diff] [review]
Patch rv2.0 (proposal)

For the present, I request the review to roc.
Attachment #223028 - Flags: superreview?(roc)
Attachment #223028 - Flags: review?(roc)
Comment on attachment 223028 [details] [diff] [review]
Patch rv2.0 (proposal)

I'd like to keep the ancestor scrolling if possible, but at the
same time fix the problem described in comment 1/comment 8.

I think that can be solved by saving the starting frame (like you did)
and the time for the last scroll event and then only scroll the starting
frame or its ancestors as long this "scroll transaction" is in progress.
I think the transaction should be limited by a time period since the
last scroll event occured and if some other non-scroll event occurs -
with one exception: I think we need to allow for some mouse move events
(at least for some distance), I have a 1600x1200 screen and it's near
impossible to keep the mouse exactly still while using the mouse wheel
(and this makes the proposed patch not work very well for me).

Regarding the patch, you should clear the frame pointer in
nsEventStateManager::ClearFrameRefs() not in ContentRemoved().

(BTW, there are good testcases in bug 97283 if you need them)
Comment on attachment 223028 [details] [diff] [review]
Patch rv2.0 (proposal)

I agree with Mats, so I'll minus this.
Attachment #223028 - Flags: superreview?(roc)
Attachment #223028 - Flags: superreview-
Attachment #223028 - Flags: review?(roc)
Attachment #223028 - Flags: review-
Attached patch Patch rv3.0 (obsolete) — Splinter Review
Mats:

Are you check this?
This patch checks cursor position threshold and time out.
I use drag starting threshold for the value of cursor position threshold.
And I set 10 sec for time out.
# We need more discussion for the default values.
Attachment #223028 - Attachment is obsolete: true
Attachment #223214 - Flags: review?(mats.palmgren)
(In reply to comment #21)
> Created an attachment (id=223214) [edit]
> Patch rv3.0

Just a thought. The patch as is will end the scrolling transaction if the mouse has moved by more than the threshold no matter how long that takes. If I'm scrolling for a while then I can imagine it could be easy to slowly drift over the threshold. But then I guess the chances of that coinciding with hitting a select box are slimmer.
> If I'm scrolling for a while then I can imagine it could be easy to slowly
> drift over the threshold.

The cursor position is updated on each wheel event. Therefore, if it's over the threshold on between 2 wheel events, the transaction is finished.
Masayuki: I'm having trouble understanding what behaviour you're implementing. I *think* you're saying:

 - scoll whatever element the mouse is pointing at
 - if the mouse moves by some threshold, you change the focus of the scroll

is that right?
Keywords: polish
Whiteboard: ui-polish
Ah, not right, maybe.

Current behavior changes the focus to scroll on every mouse move event. But my patch doesn't change the focus until:
1. the mouse position moves out to threshold from last position at last wheel scroll event.
2. time out from last wheel event.
Attached patch Patch rv3.1 (obsolete) — Splinter Review
fix some nits.
Attachment #223214 - Attachment is obsolete: true
Attachment #223230 - Flags: review?(mats.palmgren)
Attachment #223214 - Flags: review?(mats.palmgren)
Comment on attachment 223230 [details] [diff] [review]
Patch rv3.1

This patch has some problems. I'll update this patch.
Attachment #223230 - Flags: review?(mats.palmgren) → review-
Attached patch Patch rv3.2 (obsolete) — Splinter Review
fix some bugs:
1. crash after destroy the target frame.
2. in some cases, |aEvent->time| is invalid value. if so, using current time instead.
3. if the order of events doesn't match to the event creation order, it was time out.

changing:
the threshold is calculated from current screen resolution now.
Attachment #223230 - Attachment is obsolete: true
Attachment #223332 - Flags: review?(mats.palmgren)
Attached patch Patch rv3.3 (obsolete) — Splinter Review
Sorry for the spam.

I think that this patch is better.
This patch has twin time out mechanism.
1. (short time) in that time, we should not abort the transaction by mouse move event. Because this event may be non-meant event by user.
2. (long time) after the term of 1, we should check the mouse move event for aborting the transaction. I think that the threshold of the cursor position should be small value for not killing the user meant mouse move event.
# current value of the patch may be large.
Attachment #223332 - Attachment is obsolete: true
Attachment #223345 - Flags: review?(mats.palmgren)
Attachment #223332 - Flags: review?(mats.palmgren)
Ah, sorry, this patch has a warning.
I'll rewrite 0.5 * 1000 to 500 in line 5480 next.
I was thinking about this in this weekend.
I think we should not use the threshold for the mouse cursor position. E.g., on the testcase of bug 97283 that has many scroll views in small area, I was confused by the threshold. I think mats needs the threshold for non-meaning mouse move by turning the wheel. But the events are eaten by the holding time mechanism. So, we don't need the threshold now, maybe.
Attached patch Patch rv3.4 (obsolete) — Splinter Review
I think that this is better.
Assignee: events → masayuki
Attachment #223345 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #223778 - Flags: review?(mats.palmgren)
Attachment #223345 - Flags: review?(mats.palmgren)
(In reply to comment #21)
> I use drag starting threshold for the value of cursor position threshold.
> And I set 10 sec for time out.
> # We need more discussion for the default values.

Max 10 sec between mouse scroll events to keep the transaction
going seems reasonable, but you changed it in later versions
to 120 sec - was this intentional, was there a problem with 10 sec?

My opinion is that we should try to avoid any kind of "stickiness" -
meaning that the user should be able to scroll something and then move
the mouse to a new element and start to scroll that without feeling
that there is a timer involved and that two items can't be scrolled
in rapid succession. This is of course the dilemma of this bug since
it conflicts with the goal of not interrupting an ongoing scroll.

The latest patch (rv3.4) allows mouse-moves to reset the transaction
if they occur after a certain interval (0.5 sec) from the last
mouse-scroll. Additionally, if the coordinates of the mouse-move
falls outside the original frame the transaction ends even if the
time is shorter.

An alternative (or additional) mechanism would be to measure the
distance of the mouse-move and end the transaction if the distance
is too long. I don't know which mechanism(s) will work best though.

I'm guessing that the "outside-frame" mechanism will not work
well with hierarchical scrolling and if you remove this mechanism
the mouse-move-timer by itself is probably going to feel sticky
when for example moving the mouse between a set scrollable form
elements that are close to each other, because it's very hard
to differentiate this case from the case we are trying to fix.

You had a mouse-move-distance mechanism for while, could you
elaborate on why you dropped it (I don't see why it would be
"confusing" as you said in comment 31). Maybe it was interference
with the other mouse-move mechansims? (My guess is that a
mouse-move-distance would work best without the other two).
Comment on attachment 223778 [details] [diff] [review]
Patch rv3.4

On my (Linux) machine, all mouse-wheel events generates this warning:

WARNING: The event time isn't set, using current time instead.: file nsEventStateManager.cpp, line 5486

so the timers did not work at all for me.
I changed it so to use "PR_IntervalToMilliseconds(PR_IntervalNow())"
everywhere instead and then the timers worked as intended.

It seems this patch removes hierarchical scrolling,
I think this is unfortunate. I still think it would be
possible to fix this bug and keeping it.

The class nsMouseWheelTransaction declaration can probably move to
nsEventStateManager.cpp instead of being in the header.

While testing rv3.4 I saw some problem with list items bleeding
outside its container (I'll attach a screenshot) - I suspect it's
this patch that caused it but I'm not 100% sure since I didn't
track down the cause...
Attachment #223778 - Flags: review?(mats.palmgren) → review-
Mouse-wheel scrolling the page up and down a few times triggers it.
(In reply to comment #34)
> (From update of attachment 223778 [details] [diff] [review] [edit])
> On my (Linux) machine, all mouse-wheel events generates this warning:
> 
> WARNING: The event time isn't set, using current time instead.: file
> nsEventStateManager.cpp, line 5486

Yeah, I confirmed it. That is a bug of nsWindow of gtk2. Should we fix the bug too in same time? Or should separate the bug?

> While testing rv3.4 I saw some problem with list items bleeding
> outside its container (I'll attach a screenshot) - I suspect it's
> this patch that caused it but I'm not 100% sure since I didn't
> track down the cause...

Really? I don't think so...
(In reply to comment #33)
> You had a mouse-move-distance mechanism for while, could you
> elaborate on why you dropped it (I don't see why it would be
> "confusing" as you said in comment 31). Maybe it was interference
> with the other mouse-move mechansims? (My guess is that a
> mouse-move-distance would work best without the other two).

I tested some values for the mouse-move-distance. But I could not found natural behavior by the way. I think, because the mouse move events only occurred by the turning the mouse wheel. So, it may occur immediately after (or between) the mouse wheel events. If so, I think that the 0.5 sec rule is better than mouse-move-distance rule. Because it's not related to the screen resolutions and the kind of pointer devices.
> I'm guessing that the "outside-frame" mechanism will not work
> well with hierarchical scrolling and if you remove this mechanism
> the mouse-move-timer by itself is probably going to feel sticky
> when for example moving the mouse between a set scrollable form
> elements that are close to each other, because it's very hard
> to differentiate this case from the case we are trying to fix.

I thought that a feeling was very bad when the non-related scroll view is scrolling. E.g., the mouse cursor is on the toolbar, but if the transaction is not over, the page still can be scrolled by the point.
I think that this fixes the time of GTK2 event bug.
(In reply to comment #36)
> > While testing rv3.4 I saw some problem with list items bleeding
> > outside its container ...
> 
> Really? I don't think so...

You're right - I filed bug 340110 on it.
Attached patch Patch rv3.5 (obsolete) — Splinter Review
Mats:

Would you re-review this?

changes:
1. sPoint is removed.
2. the class definition moves .h to .cpp
3. remove another direction checking for hierarchical scrolling (backing to current behavior).
Attachment #223778 - Attachment is obsolete: true
Attachment #224212 - Flags: review?(mats.palmgren)
I forgot to explain for 120 sec.

I think that the reading user may need long transaction. Because if people reads the text, who is not looking the view under the cursor. I think the user wants to scroll same viewer until the mouse move.
The patch doesn't work fine on GTK2.
Because GTK2 doesn't set event creation time for mouse move too.
I'll create new GTK2 widget patch for the problem.
I separate the bug for GTK2 widget.
See bug 340149.
Attached patch Patch rv3.6 (obsolete) — Splinter Review
adding workaround for the problem. (there may be same problem on other platforms...)
Attachment #224212 - Attachment is obsolete: true
Attachment #224235 - Flags: review?(mats.palmgren)
Attachment #224212 - Flags: review?(mats.palmgren)
Comment on attachment 224235 [details] [diff] [review]
Patch rv3.6

There is still a regression with hierarchical scrolling with
this version. Try scrolling the nested views in the testcases
for bug 97283 for example. When a transaction starts on an
inner view and scrolling reaches its end position the scroll
does not continue to the surrounding view.
If I move the mouse so the transaction stops then hierarchical
scrolling works though.
Attachment #224235 - Flags: review?(mats.palmgren) → review-
(In reply to comment #46)
>When a transaction starts on an inner view and scrolling reaches its
>end position the scroll does not continue to the surrounding view.
Isn't that the point of this bug, or am I missing something? See comment 1.
(In reply to comment #47)
> Isn't that the point of this bug, or am I missing something? See comment 1.

As I said in comment 19, I think it's possible to solve the problem
in comment 1/comment 8 and still keep hierarchical scrolling as is.
When we start a scroll we would scroll that view and all it's scrollable
ancestors until the transaction ends, it's only the unrelated views that
pass in under the mouse that we are trying to avoid.
At least this is what I thought the intention was. Comment 41 point 3
claims there was not supposed to be any change so that's the reason
for the review-.

Maybe Masayuki can say if the change was intentional and what the
new behaviour is supposed to be, then I'll redo the review.
Mats:

No, I wanted to say in comment 41 that at the starting transaction we should keep current behavior that is hierarchical scrolling. But in the transaction, that is not meant by user, right? I think that it is that the reporter just thinks that he is dissatisfied.

I think that I and Neil get same result for this issue by comment 47.
(In reply to comment #49)
Ok, let's try it and see what people say...
Comment on attachment 224235 [details] [diff] [review]
Patch rv3.6

>+    if (!scrollView) {
>+      nsMouseWheelTransaction::EndTransaction();
>+      lastScrollFrame = nsnull;
>+    } else {
>+      nsMouseWheelTransaction::UpdateTransaction(aEvent);
>+    }

Just a style nit, you can swap the then/else-blocks and then write
"if (scrollView) {" instead.


>+/******************************************************************/
>+/* nsMouseWheelTransaction                                        */
>+/******************************************************************/

Could you move the methods to just after the class declaration instead?
And remove this block comment.


>+nsMouseWheelTransaction::UpdateTransaction(nsGUIEvent* aEvent)
>+{
>+  // XXX should we always use actual time for the case
>+  // where the responses become late?
>+  if (aEvent->time) {
>+    sTime = aEvent->time;
>+  } else {
>+    NS_WARNING("The event time isn't set, using current time instead.");
>+    sTime = PR_IntervalToMilliseconds(PR_IntervalNow());
>+  }
>+}

Could we resolve the XXX comment before checkin?
I think it can be removed since I'm guessing "aEvent->time" better reflects
when the event occurred (given that it was actually set that is).


>+        if (pt.x < r.x || pt.y < r.y || pt.x >= r.XMost() || pt.y >= r.YMost())

I think "if (!r.Contains(pt))" is clearer.


With those nits fixed, r=mats
Attachment #224235 - Flags: review- → review+
(In reply to comment #51)

> Could you move the methods to just after the class declaration instead?

Well, after these lines I mean:
>+PRUint32  nsMouseWheelTransaction::sHoldingTime = 0;
>+
(In reply to comment #51)
Thank you for your review. But you have a mistake.

> >+        if (pt.x < r.x || pt.y < r.y || pt.x >= r.XMost() || pt.y >= r.YMost())
> 
> I think "if (!r.Contains(pt))" is clearer.

nsRect has Contains. But nsIntRect doesn't have it ifdef NS_COORD_IS_FLOAT :-(
See http://lxr.mozilla.org/mozilla/source/gfx/public/nsRect.h#179
NS_COORD_IS_FLOAT is not defined, and if you define it, hundreds of files fail to buld. so don't worry about that until 2010 or later :-)
Attached patch Patch rv3.7 (obsolete) — Splinter Review
Thank you, roc.
I fixed some nits.
Attachment #224235 - Attachment is obsolete: true
Attachment #224860 - Flags: superreview?(roc)
Attachment #224860 - Flags: review+
+  return (now > aBaseTime &&

The "now > aBaseTime" check is not needed because time can't flow backwards. I hope.

+  sTimeout =
+    nsContentUtils::GetIntPref("mousewheel.transaction.timeout", 120000);
+  sHoldingTime =
+    nsContentUtils::GetIntPref("mousewheel.transaction.holding", 500);

I don't think you should cache these prefs. They won't get used very often (at most once per user event).

+    if (svp)
+      scrollView = svp->GetScrollableView();
+    if (scrollView) {

I don't think you need to check that the scrollview is non-null.

You need a comment explaining the overall strategy here --- describe exactly the behaviour that you're trying to implement.

Fix those up and post a new patch, then I'll rewrite your comments a bit and you can check in.
Attached patch Patch rv3.8 (obsolete) — Splinter Review
Thank you roc for your review. Would you check this?
Attachment #224860 - Attachment is obsolete: true
Attachment #226122 - Flags: superreview?(roc)
Attachment #226122 - Flags: review+
Attachment #224860 - Flags: superreview?(roc)
I don't think the 120 second timeout is useful. 120 seconds might as well be forever. I think we should just drop that. The comments below assume that we get rid of that timeout.

+  //    the event creation time doesn't keep rial time.

"real"

I think we should call "mousewheel.transaction.holding" something more clear. How about "mousewheel.transaction.ignoremovedelay"?

+        // If the cursor is moved on the frame, should check holding time.
+        // By this, the movement which is not meant by user can be ignored.
+        // But we should not check it if the cursor moves to out of the frame.

"If the cursor is moving inside the frame, and it is less than ignoremovedelay milliseconds since the last scroll operation, ignore the mouse move; otherwise, terminate the scrollwheel transaction."

+  // If the user scrolled by the wheel just before, the user wants to
+  // scroll the same view as a previous instead of the view under the cursor.
+  // Therefore, we are controling the wheel scrolling transaction in
+  // nsMouseWheelTransaction. That aborts the transaction by time out or
+  // by the mouse move event automatically.

"If the user recently scrolled with the mousewheel, then they probably want to scroll the same view as before instead of the view under the cursor. nsMouseWheelTransaction tracks the frame currently being scrolled with the mousewheel. We consider the transaction ended when the mouse moves more than 'mousewheel.transaction.ignoremovedelay" milliseconds after the last scroll operation, or any time the mouse moves out of the frame."
(In reply to comment #58)
> I don't think the 120 second timeout is useful. 120 seconds might as well be
> forever. I think we should just drop that. The comments below assume that we
> get rid of that timeout.

Roc, I don't think drop is best way. Because it may not behave sometimes satisfactorily. I think that we should change the default value to 10 sec is better than drop. How about you?
> I think that we should change the default value to 10 sec is
> better than drop.

or shorter time.
Can you explain what problem is solved by that delay?
(In reply to comment #61)
> Can you explain what problem is solved by that delay?

Simply, if I was scrolling slowly, the transaction was aborted. To begin with, the timeout mechanism, caching the scrolling view and aborting by non-wheel events are bases of the patch...
Attached patch Patch rv3.9 (obsolete) — Splinter Review
Roc:

Sorry, my previous comment is wrong that is not a reason for it. I thought about reverse things.

But I have a reason for keeping the timeout mechanism. If the timeout mechanism is obsoleted, the transaction is holding forever if the mouse events and key events doesn't occur. So, if the user uses the notebook PC that has wheel, the transaction doesn't abort forever if the user only use wheel. I think that the transaction should finish in suitable time.

I change the time to 3 sec. I think that if the time is too long, the user may be confused why the view is scrolled. If the time is too short, the user may not be confused. But it may not be ideal. However, it's not to regress from current behavior. I think that we should keep this mechanism and we should set short value in default settings. It's better than the user being confused.
Attachment #226122 - Attachment is obsolete: true
Attachment #227961 - Flags: superreview?(roc)
Attachment #227961 - Flags: review+
Attachment #226122 - Flags: superreview?(roc)
If I understand correctly the confusion would happen like this:
-- User scrolls window down with the mousewheel
-- A listbox (say) is scrolled under the mouse cursor
-- User stops scrolling for a while
... a few seconds later ...
-- User tries to scroll the listbox with the mousewheel, but the whole window scrolls, confusing the user.

A 120s timeout would have been way too long to prevent this, but 3s sounds reasonable.
Comment on attachment 227961 [details] [diff] [review]
Patch rv3.9

+inline void
+SetFrameExternalReference(nsIFrame* aFrame)
+{
+  aFrame->AddStateBits(NS_FRAME_EXTERNAL_REFERENCE);
+}

This should be static

+  // mousewheel. We consider the transaction ended when the mouse moves more than
+  // "mousewheel.transaction.ignoremovedelay" milliseconds after the last scroll
+  // operation, or any time the mouse moves out of the frame.

now we need to add

or when more than "mousewheel.transaction.timeout" milliseconds have passed after the last operation, even if the mouse hasn't moved.
Attachment #227961 - Flags: superreview?(roc) → superreview+
Attached patch Patch rv3.10Splinter Review
Attachment #227961 - Attachment is obsolete: true
Attachment #228901 - Flags: superreview+
Attachment #228901 - Flags: review+
checked-in, thank you.

(In reply to comment #64)
> If I understand correctly the confusion would happen like this:
snip.
> A 120s timeout would have been way too long to prevent this, but 3s sounds
> reasonable.

Yes. You are right. But if some people may think 3sec is too long, we need to change the value...
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
I retested with nightly build. But I was confused sometimes.
The 0.5 sec of ignoremovedelay may be too long... 0.2 sec may be better value. But I don't have a confidence to it.
# or we may need more check for ignoremovedelay...

I'll try to think for better implementation.
(In reply to comment #64)
I think the 3s timeout here as well as the 0.5 ignore move is too long myself. Personally I don't think I would stop scrolling for more than a second and still consider it the same scroll event.

I also notice that someone on the forums spotted a problem, even to the point of thinking they couldn't scroll an iframe without clicking on it.

I tried changing my values to 1s and 200ms and things seems a lot more sensible to me, I really had to do some strange things with my mouse to get the scroll to do something I didnt want it to.

On minor problem with the current setup, if I scroll to the bottom of a page and at which point my cursor goes over a frame, if I keep scrolling down at just below the threshold, the frame never scrolls.
(In reply to comment #69)
> I think the 3s timeout here as well as the 0.5 ignore move is too long myself.

> I tried changing my values to 1s and 200ms and things seems a lot more sensible
> to me,

Yeah, me too. I'll back to this bug and I'm thinking better algorithm and better settings. Please wait.
-> REOPEN for better default settings and better implementation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
Attached patch Additional patch rv1.0 (obsolete) — Splinter Review
I improve the ignore mouse move mechanism. The current code doesn't care the mouse move events just before the wheel event. We should keep the transaction after mouse moving, instead of that, we should check how long time between mouse move event and wheel event.

I think that by this change, we can shrink the default value of ignore mouse move. I change the values:
ignore mouse move: 0.5s -> 0.1s
timeout threshold: 3s -> 1.5s

I think that this patch makes more natural interface.
Attachment #231437 - Flags: superreview?(roc)
Attachment #231437 - Flags: review?(roc)
Note that new patch changes the ignore mouse move period.

Current build only ignores the immediately events.

  ----------------------| wheel event |----------------------
                               +---------------------------> 0.5s

But new patch ignores *around* the wheel events.

  ----------------------| wheel event |----------------------
                0.1s <---------+---------> 0.1s

So, actual ignore mouse move period is |ignoremousemove * 2|.
Comment on attachment 231437 [details] [diff] [review]
Additional patch rv1.0

+        // If the cursor is moving to outsied the frame,

"to be outside the frame"

+        // If the cursor has moved, and it is less than ignoremovedelay
+        // milliseconds until this wheel event, ignore the mouse move events;
+        // otherwise, terminate the wheelscroll transaction.

"Terminate the current mousewheel transaction if the mouse moved more than ignoremovedelay milliseconds ago"

+        // the mouse move; otherwise, record the current time for the wheel
+        // event which may happen immediately.

"otherwise, record the current mouse move time to be checked later"
Attachment #231437 - Flags: superreview?(roc)
Attachment #231437 - Flags: superreview+
Attachment #231437 - Flags: review?(roc)
Attachment #231437 - Flags: review+
Attachment #231437 - Attachment is obsolete: true
Attachment #232498 - Flags: superreview+
Attachment #232498 - Flags: review+
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Fixed in build 2006-08-12-04-trunk.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.