303048 - GTK scrollbar does not have normal GTK right-click behavior

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Ruben Vermeersch]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

Rightclicking a GTK2 scrollbar arrow does nothing in Firefox. In normal GTK2
environments, this would scroll up/down all the way to the top. Though this is
not a very eyecatching problem, I feel that it's very important to not only
mimic the appearance, but also the behavior of GTK2.

I am willing to write a patch for this, if the developers think it would be
useful. Some hints where I should look would be appreciated then though, I'm not
very familiar with the Mozilla source code.

Reproducible: Always

Comment 1

[Ruben Vermeersch]

Also not working: middle mouse click on the arrow scrolls one entire screen in
GTK , does nothing in Firefox

Comment 2

[Adam Guthrie]

This looks good as long as it's the correct thing to do. CCing some people who 
might be able to weigh in on this.

Comment 3

[[e-mail]]

(In reply to comment #1)
>Also not working: middle mouse click on the arrow scrolls one entire screen in
>GTK, does nothing in Firefox
There's a pref for middle-click to scroll to the clicked position,
I don't know if it's on or off by default for GTK.

Comment 4

[Adam Guthrie]

Attachment: work in progress

What this does:
* handles right and middle clicks on scrollbars appropriately.

What this doesn't do:
* handles this behavior on a per-platform basis,
* fixes nsSliderFrame::HandlePress so it deals with right and middle clicks appropriately.

I'm not sure how I'm going to make this GTK-only. There would need to be profuse ifdeffing, which doesn't seem right.

Comment 5

[Adam Guthrie]

Also, there is no visual indication of the button being pressed when it's right or middle clicked. I'm trying to figure out where this is handled so I can change this.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Don't modify nsFrame::HandleEvent like that. That will change lots of things that we don't want to change! Instead, you can override HandleEvent in nsScrollbarButtonFrame.

I would return a boolean value from MouseClicked to indicate whether we should repeat or not, instead of testing the button.

To make this behaviour per-platform, we can add new settings to nsILookAndFeel. We should probably have three settings, one setting per mouse button, and let each setting take one of four values: "button scrolls by one line", "button scrolls by one page", "button scrolls to end", "button ignored".

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Making the button look depressed is done with :active CSS rules. We only make content active when the left button is pressed:
http://lxr.mozilla.org/mozilla/source/content/events/src/nsEventStateManager.cpp#1923
and I think we don't want to change that.

One way to fix this would be to set an attribute on the element to indicate that it's active (e.g. active="true"), and change the CSS rules to look at that instead. Then we can set and remove the attribute when any button is pressed or released.

Comment 8

[Adam Guthrie]

Attachment: work in progress (with diff -w)

Here's another wip taking into account roc's suggestions. 

* I realized that this isn't the scrollbar thumb, so I'm going to change the pref. name.
* I know I'll probably need to modify all of the branched css files (once I get the right rules).
* The constructor of nsScrollbarButtonFrame feels a bit odd, but I'm not quite sure what to do about it.
* nsScrollbarButtonFrame::MouseClicked() feels a bit hacky, but again, I'm not sure how else to do it.
* As of now, the attribute setting / CSS parts I implemented aren't working. I'm not sure why, though.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, that attribute-setting code looks OK. In fact it mostly looks good :-).

You are building this on Windows and using Winstripe, right? Make sure that changing the CSS rules, e.g. to set the border colors to yellow, does work.

Check the result of SetAttr. You could also put a GetAttr after it to print out the value of the attribute.

Don't store the nsILookAndFeel value in the frame. Just re-get it every time you need it. This will not hurt performance in any measurable way and it will let you respond to dynamic changes in preferences, system settings, etc.

Comment 10

[Adam Guthrie]

Attachment: patch

I haven't tested this on Windows, so if anyone could that would be great. It should not change anything really. Just make sure that middle/right clicking on scrollbars does nothing.

Comment 11

[[e-mail]]

I would have thought it very unlikely that the increment changes between repeats, so you could hoist the increment calculation into MouseDown thus caching mIncrement instead of mClickType mLeftButtonAction mMiddleButtonAction and mRightButtonAction.

You also shouldn't name a PRBool result rv, only nsresults are allowed that.

MouseClicked() doesn't need to be virtual, does it? (I also think that it's a bad name for that method, because it never occurs on a click...)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What Neil said.

Also,
+        aState->active = CheckBooleanAttr(aFrame, nsWidgetAtoms::active);
I think you should just do "if (CheckBooleanAttr(...)) aState->active = PR_TRUE;"
because if the CSS active state is set, we still want to honor that.

Comment 13

[Adam Guthrie]

Attachment: patch

Patch with comments addressed. (Also, I realized that someone already added the "active" atom to nsGkAtomList.h! How convenient.)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (aEvent->message == NS_MOUSE_EXIT_SYNTH|| aEvent->message == NS_MOUSE_RIGHT_BUTTON_UP || aEvent->message == NS_MOUSE_LEFT_BUTTON_UP || aEvent->message == NS_MOUSE_MIDDLE_BUTTON_UP)

Break this so it fits in 80-character lines please!

I think probably if a button is set to "ignore" then we should pass it to superclass's HandleEvent. I think HandlePress should set aEventStatus appropriately and we should test that in our HandleEvent.

+  if (aPresContext && (lookAndFeel = aPresContext->LookAndFeel())) {
+    PRInt32 tmpAction;
+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_ScrollButtonLeftMouseButtonAction,
+                                            tmpAction)))
+      leftButtonAction = tmpAction;
+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_ScrollButtonMiddleMouseButtonAction,
+                                            tmpAction)))
+      middleButtonAction = tmpAction;
+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(nsILookAndFeel::eMetric_ScrollButtonRightMouseButtonAction,
+                                            tmpAction)))
+      rightButtonAction = tmpAction;
+  }

Instead, it would be simpler to aEvent->message to select the right *Action constant and make one call to GetMetric.

+  mContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::type, value);

Instead, use FindAttrValueIn. In fact this code would be simplified if you call FindAttrValueIn, and exit early if it's not "increment" or "decrement". Set a PRInt32 direction to 1 or -1 depending on which direction it is. Then just have one switch statement on the action type, you can use direction*increment or direction*pageIncrement, and an if statement for case 2. The getting of maxpos and curpos can be moved into case 2. 'increment' should be called lineIncrement.

Rename MouseClicked to DoButtonAction.

Comment 15

[Adam Guthrie]

Comment on attachment 226614 [details] [diff] [review]
patch

Thanks for the quick review, roc! I'll try to come up with a new patch tonight.

Comment 16

[Adam Guthrie]

OK. I addressed all of your comments except the one about the "ignore" event.

In HandleEvent I have something like this:
  if ((aEvent->message == NS_MOUSE_RIGHT_BUTTON_DOWN ||
      aEvent->message == NS_MOUSE_MIDDLE_BUTTON_DOWN) &&
      *aEventStatus != nsEventStatus_eIgnore)
    HandlePress(aPresContext, aEvent, aEventStatus);
  else
    return nsButtonBoxFrame::HandleEvent(aPresContext, aEvent, aEventStatus);

and then in HandlePress I'm seeing aEventStatus like so in the switch statement:
    case 3:
      *aEventStatus = nsEventStatus_eIgnore;
      break;

The thing is, right and middle clicks already have their status set to nsEventStatus_eIgnore, so they never fall into our HandlePress method. :(

Comment 17

[Adam Guthrie]

Attachment: patch v3

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if ((aEvent->message == NS_MOUSE_RIGHT_BUTTON_DOWN || 
+      aEvent->message == NS_MOUSE_MIDDLE_BUTTON_DOWN) /*&&
+      *aEventStatus != nsEventStatus_eIgnore*/)
+    HandlePress(aPresContext, aEvent, aEventStatus);

What I meant was, in HandlePress, if you do something with the button (i.e. the corresponding button nsILookFeel setting is not 3), set *aEventStatus to nsEventStatus_eConsumeNoDefault. Then after you return from this call to HandlePress, if *aEventStatus is not nsEventStatus_eIgnore, carry on to call HandleEvent.

Actually it would be nice if all this event handling could just be done by the button XBL binding...

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually a cleaner solution would be to make your HandlePress a seperate function that does not override nsIFrame::HandlePress, e.g., HandleButtonPress. Have it just return a PRBool to indicate whether the press has been handled. Call it, then carry on to superclass HandleEvent if it returns PR_FALSE.

Comment 20

[[e-mail]]

(In reply to comment #18)
>Actually it would be nice if all this event handling could just be done by the
>button XBL binding...
Well maybe with XBL2 we can execute scripts on the scrollbar binding even when they are disabled for the rest of the page by the docshell/midas/whomever...

Comment 21

[[e-mail]]

Comment on attachment 226765 [details] [diff] [review]
patch v3

>+    case 3:
>+      *aEventStatus = nsEventStatus_eIgnore;
>+      break;
>+    default:
>+      break;
I don't know why you made 3 different from any higher number but surely returning early is a good idea if the button is to do nothing?

Comment 22

[[e-mail]]

Comment on attachment 226765 [details] [diff] [review]
patch v3

>Index: widget/src/gtk2/nsNativeThemeGTK.cpp
You've tried to make the code available on all platforms. However you're only fixing the styling for the native GTK2 theme. You should correct all themes.

>+  { "ui.scrollbarButtonLeftMouseButtonAction",
>+    eMetric_ScrollButtonLeftMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 0 },
>+  { "ui.scrollbarButtonMiddleMouseButtonAction",
>+    eMetric_ScrollButtonMiddleMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 3 },
>+  { "ui.scrollbarButtonRightMouseButtonAction",
>+    eMetric_ScrollButtonRightMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 3 },
You know that PR_FALSE means that the default value is ignored? I don't know why all the other metrics use PR_FALSE and 0 but it looks like this structure is supposed to hold the user override values (if any).

>+  // Get the desired action for the scrollbar button.
>+  nsILookAndFeel::nsMetricID tmpAction;
>+  if (aEvent->message == NS_MOUSE_LEFT_BUTTON_DOWN) 
>+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonLeftMouseButtonAction;
>+  else if (aEvent->message == NS_MOUSE_MIDDLE_BUTTON_DOWN)
>+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonMiddleMouseButtonAction;
>+  else if (aEvent->message == NS_MOUSE_RIGHT_BUTTON_DOWN)
>+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonRightMouseButtonAction;
else?

>+  PRInt32 pressedButtonAction;
>+  nsILookAndFeel* lookAndFeel = nsnull;
>+  if (aPresContext && (lookAndFeel = aPresContext->LookAndFeel())) {
>+    PRInt32 action;
>+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(tmpAction,
>+                                            action)))
>+      pressedButtonAction = action;
>+  }
But unfortunately if anything fails along the way then pressedButtonAction's value will be undefined.

>+  // get the increment amount
>+  PRInt32 lineIncrement = nsSliderFrame::GetIncrement(content);
>+  PRInt32 pageIncrement = nsSliderFrame::GetPageIncrement(content);
Why not get these when you actually need them?

>+#ifdef MOZ_WIDGET_COCOA
>+  // Emulate the Mac IE behavior of scrolling 2 lines instead of 1
>+  // on a button press.  This makes scrolling appear smoother and
>+  // keeps us competitive with IE.
>+  increment *= 2;
>+#endif
increment?

>+    case 3:
>+      *aEventStatus = nsEventStatus_eIgnore;
>+      break;
>+    default:
>+      break;
Don't forget.

Comment 23

[Adam Guthrie]

Attachment: patch v4

Comment 24

[Adam Guthrie]

(In reply to comment #22)
> (From update of attachment 226765 [details] [diff] [review] [edit])
> >Index: widget/src/gtk2/nsNativeThemeGTK.cpp
> You've tried to make the code available on all platforms. However you're only
> fixing the styling for the native GTK2 theme. You should correct all themes.
> 
> >+  { "ui.scrollbarButtonLeftMouseButtonAction",
> >+    eMetric_ScrollButtonLeftMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 0 },
> >+  { "ui.scrollbarButtonMiddleMouseButtonAction",
> >+    eMetric_ScrollButtonMiddleMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 3 },
> >+  { "ui.scrollbarButtonRightMouseButtonAction",
> >+    eMetric_ScrollButtonRightMouseButtonAction, PR_FALSE, nsLookAndFeelTypeInt, 3 },
> You know that PR_FALSE means that the default value is ignored? I don't know
> why all the other metrics use PR_FALSE and 0 but it looks like this structure
> is supposed to hold the user override values (if any).

I set left these at PR_TRUE and tested on Mac and everything seems to be OK. Although, I do remember them having a totally different scrollbar implementation...

> >+  // Get the desired action for the scrollbar button.
> >+  nsILookAndFeel::nsMetricID tmpAction;
> >+  if (aEvent->message == NS_MOUSE_LEFT_BUTTON_DOWN) 
> >+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonLeftMouseButtonAction;
> >+  else if (aEvent->message == NS_MOUSE_MIDDLE_BUTTON_DOWN)
> >+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonMiddleMouseButtonAction;
> >+  else if (aEvent->message == NS_MOUSE_RIGHT_BUTTON_DOWN)
> >+    tmpAction = nsILookAndFeel::eMetric_ScrollButtonRightMouseButtonAction;
> else?

Done.

> >+  PRInt32 pressedButtonAction;
> >+  nsILookAndFeel* lookAndFeel = nsnull;
> >+  if (aPresContext && (lookAndFeel = aPresContext->LookAndFeel())) {
> >+    PRInt32 action;
> >+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(tmpAction,
> >+                                            action)))
> >+      pressedButtonAction = action;
> >+  }
> But unfortunately if anything fails along the way then pressedButtonAction's
> value will be undefined.

I'm just going to return NS_ERROR_FAILURE if we can't get the metric from the presshell's lookandfeel. Otherwise, I think I'd have to re-implement what I wanted to be the default actions for these buttons.

> >+  // get the increment amount
> >+  PRInt32 lineIncrement = nsSliderFrame::GetIncrement(content);
> >+  PRInt32 pageIncrement = nsSliderFrame::GetPageIncrement(content);
> Why not get these when you actually need them?

I changed getting the page increment when I needed it, but I left the line increment so I could double it for the Mac case.

> >+#ifdef MOZ_WIDGET_COCOA
> >+  // Emulate the Mac IE behavior of scrolling 2 lines instead of 1
> >+  // on a button press.  This makes scrolling appear smoother and
> >+  // keeps us competitive with IE.
> >+  increment *= 2;
> >+#endif
> increment?

Done.

> >+    case 3:
> >+      *aEventStatus = nsEventStatus_eIgnore;
> >+      break;
> >+    default:
> >+      break;
> Don't forget.

Done.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    return NS_ERROR_FAILURE;

You're returning NS_ERROR_FAILURE for a PRBool. That's not good at all.

+  if (aPresContext && (lookAndFeel = aPresContext->LookAndFeel())) {

You can rely on aPresContext not being null here.

+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(tmpAction,
+                                            action)))

One line.

+  // get the increment amount
+  PRInt32 lineIncrement = nsSliderFrame::GetIncrement(content);

Move this down to the one place where you need it.

+  content->SetAttr(kNameSpaceID_None, nsXULAtoms::smooth, NS_LITERAL_STRING("true"), PR_FALSE);
+  content->SetAttr(kNameSpaceID_None, nsXULAtoms::curpos, curposStr, PR_TRUE);
+  content->UnsetAttr(kNameSpaceID_None, nsXULAtoms::smooth, PR_FALSE);

We're doing a smooth scroll to the top or bottom. When Home/End are pressed we don't do a smooth scroll, and I think you shouldn't do one here either. How about adding a boolean parameter to DoButtonAction controlling whether the scroll is smooth or not.

Comment 26

[Adam Guthrie]

Attachment: patch v5

This patch feels more and more hacky on each go-around. :(

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 228626 [details] [diff] [review]
patch v5

I think your instincts are off; it's not that bad.

Comment 28

[[e-mail]]

Comment on attachment 228626 [details] [diff] [review]
patch v5

>+  nsILookAndFeel* lookAndFeel = nsnull;
>+  if (lookAndFeel = aPresContext->LookAndFeel()) {
>+    PRInt32 action;
>+    if (NS_SUCCEEDED(lookAndFeel->GetMetric(tmpAction, action)))
>+      pressedButtonAction = action;
>+    else
>+      return PR_FALSE;
>+  }
This isn't the way I would write it for several reasons:
a) you're assigning twice to lookAndFeel e.g. instead use
nsILookAndFeel* lookAndFeel = aPresContext->LookAndFeel();
b) The very name LookAndFeel makes me suspicious. Layout tends to use GetXXX for methods that can return null. nsPresContext::Init fails if it can't get the look and feel, so I feel sure you don't need to check.
c) you're using a temporary action instead of passing pressedButtonAction
So, assuming b) is correct, I would have written
if (NS_FAILED(aPresContext->LookAndFeel()->GetMetric(tmpAction, pressedButtonAction))) return PR_FALSE;
(well maybe something that fits into 80 columns but you get the idea)

>+    default:
>+      break;

But you didn't set mIncrement...

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> Layout tends to use GetXXX

Except when the getter can never return null. So might a well get rid of the "if" test.

Comment 30

[Adam Guthrie]

Attachment: patch v6

Final round, hopefully...

Comment 31

[Adam Guthrie]

Attachment: patch v6 (with all the files)

Comment 32

[[e-mail]]

Comment on attachment 229334 [details] [diff] [review]
patch v6 (with all the files)

roc, thanks for confirming that.

Comment 33

[Adam Guthrie]

Thanks for all the help with this, Robert and Neil! 

If one of you two could check this in for me once the tree opens that'd be great. Also, if you could change the comment in the default part of the switch statement to read: "value to the button's action" instead of "value the button's action" that would be great as well. :)

Comment 34

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp 	1.82
mozilla/widget/src/xpwidgets/nsWidgetAtomList.h 	1.12
mozilla/widget/src/xpwidgets/nsXPLookAndFeel.cpp 	1.50
mozilla/layout/xul/base/src/nsScrollbarButtonFrame.h 	1.22
mozilla/layout/xul/base/src/nsScrollbarButtonFrame.cpp 	1.47
mozilla/widget/src/gtk2/nsLookAndFeel.cpp 	1.28
mozilla/content/base/src/nsGkAtomList.h 	3.27

Comment 35

[:Gavin Sharp [email: [e-mail]]]

mozilla/widget/public/nsILookAndFeel.h 	1.55, too (oops)

Comment 36

[[e-mail]]

I just wanted to harp on about the fact that your use of look and feel is now a very expensive ifdef.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Except that #ifdefs suck and this solution scales better if other platforms want to customize their behaviour.