Closed Bug 364125 Opened 16 years ago Closed 16 years ago

middle-click on scroll bar keeps scrolling until the left mouse button is pressed

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vvasaitis, Assigned: badsector)

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a1) Gecko/20061217 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9a1) Gecko/20061217 SeaMonkey/1.5a

  Hello,

  Under X, the standard behaviour of applications for a middle click on the scroll bar is to move the scroller at the position of the click. So this is what SeaMonkey does too. However, with recent SeaMonkey nightly builds, after the middle button is released the scroller keeps following the mouse pointer, until the left button is pressed (until it's released actually). This is inconsistent with every other application and IMHO extremely annoying.

  With a little bit of binary search, I pinpointed the 20061117 nightly build as the first to introduce this problem. Oh, and apologies if this is a duplicate, if it's been reported before I couldn't find it.

Thanks,
Vasilis


Reproducible: Always
confirmed with a linux seamonkey CVS trunk build

the regression range fingers bug 354694.

==> Event handling
Assignee: general → events
Status: UNCONFIRMED → NEW
Component: General → Event Handling
Ever confirmed: true
Keywords: regression
Product: Mozilla Application Suite → Core
QA Contact: general → ian
Version: unspecified → Trunk
A check in nsSliderFrame.cpp (this one: http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsSliderFrame.cpp#537) does check if the left mouse button is released in order to remove the capture, but it doesn't check for the middle button (although a check for that is done in the case's beginning).

This patch just adds this extra check which fixes the bug.
Attachment #248942 - Flags: review?(Olli.Pettay)
Comment on attachment 248942 [details] [diff] [review]
A patch for nsSliderFrame.cpp that fixes the bug.

>       if (aEvent->eventStructType != NS_MOUSE_EVENT ||
I don't know why smaug put this check here (it's not done anywhere else in the file) but if it's unnecessary then the button checks could be merged into one if statement.
(In reply to comment #3)
> if it's unnecessary then the button checks could be merged into one
> if statement.
> 
Yes, it should be ok to have only one |if|.

The new patch works almost like the previous, but it uses only one if for button checking, as said in the previous comment :-).
Attachment #248942 - Attachment is obsolete: true
Attachment #248947 - Flags: review?(Olli.Pettay)
Attachment #248942 - Flags: review?(Olli.Pettay)
Comment on attachment 248947 [details] [diff] [review]
Code-wise improved patch for this bug.


>+      if (aEvent->eventStructType == NS_MOUSE_EVENT) {

As Neil said, you shouldn't actually need this. Currently if the event is NS_MOUSE_BUTTON_UP, 
eventStructType == NS_MOUSE_EVENT.
(I shouldn't have added that |!=| check there.)
Attachment #248947 - Flags: review?(Olli.Pettay) → review-
Sorry, i don't know much of SeaMonkey's internals, i was just looking at the existing logic :-).

So, here is an improved, "one-if" version of the improved patch :-).
Attachment #248947 - Attachment is obsolete: true
Attachment #248974 - Flags: review?(Olli.Pettay)
Comment on attachment 248974 [details] [diff] [review]
Even better patch that fixes this bug.


>+    case NS_MOUSE_BUTTON_UP: {

Is there any reason to add |{|

>+        nsMouseEvent  *ev = NS_STATIC_CAST(nsMouseEvent*, aEvent);

this seems to have 4 spaces as indentation, 2 is enough.
And 2 spaces before *ev, 1 is enough.
(Personally I prefer nsMouseEvent* ev to nsMouseEvent *ev, 
but that doesn't really matter)

>       }
>+
>     }
> 

Extra newline.

With those fixed, r=me
Attachment #248974 - Flags: review?(Olli.Pettay) → review+
> Is there any reason to add |{|

Well, i declare a variable in there (the "ev" one), so i think it's better to keep it local. If i don't add brackets, the variable will be visible in a possible "case" or "default" that might be added at the future, and that wouldn't be good, i think.
(In reply to comment #9)
> Well, i declare a variable in there (the "ev" one), so i think it's better to
> keep it local. 

Ok, that is fine :)
Ok, fixed the identation (to 2 spaces), star placement and removed the extra newline :-).
Attachment #248974 - Attachment is obsolete: true
Attachment #248977 - Flags: review?(Olli.Pettay)
ugh, nsSliderFrame doesn't have any consistent coding style.
Indentation is wrong in any case :(
Sorry , I should have noticed that.
If you write the patch in a simpler way, there isn't
need to care about that problem:
-      if (aEvent->eventStructType != NS_MOUSE_EVENT ||
+      if (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eLeftButton ||
           (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eMiddleButton &&
-           !gMiddlePref)) {
-        break;
-      }
-
-      if (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eLeftButton) {
+           gMiddlePref)) {
Well, i've been told in #seamonkey that using these casts isn't a good thing, so i used the variable instead (my first patch used casting).
Ok, here is a simpler patch that uses casting.
Attachment #248977 - Attachment is obsolete: true
Attachment #248984 - Flags: review?(Olli.Pettay)
Attachment #248977 - Flags: review?(Olli.Pettay)
Comment on attachment 248984 [details] [diff] [review]
A simple patch that fixes this bug.


>     case NS_MOUSE_BUTTON_UP:
>-      if (aEvent->eventStructType != NS_MOUSE_EVENT ||
>-          (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eMiddleButton &&
>-           !gMiddlePref)) {
>-        break;
>-      }
>-
>-      if (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eLeftButton) {
>-         // stop capturing
>+      if (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eLeftButton ||
>+          (NS_STATIC_CAST(nsMouseEvent*, aEvent)->button == nsMouseEvent::eMiddleButton && gMiddlePref))
>+      {
>+        // stop capturing

Still some nits ;)
Newline after  && , the line length is too much.


>-        }
>+        } 

This is not needed.


I'll fix those when checking this in, if roc sr+s.
Attachment #248984 - Flags: superreview?(roc)
Attachment #248984 - Flags: review?(Olli.Pettay)
Attachment #248984 - Flags: review+
Attachment #248984 - Flags: superreview?(roc) → superreview+
Assignee: events → badsector
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.