395564 - scrollbar's arrow buttons should be disabled when they can't scroll further

Status: VERIFIED FIXED

(Core :: Widget: Gtk, defect)

Description

[Anton Rogaynis (antonr)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007090502 SeaMonkey/2.0a1pre
Build Identifier: 

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.6)
Gecko/20070723 Iceweasel/2.0.0.6 (Debian-2.0.0.6-0etch1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre)
Gecko/2007082201 SeaMonkey/2.0a1pre

bluecurve theme. (suppose the same effect with other themes)

Reproducible: Always

Steps to Reproduce:
1. Open any web page with scrollbars. (ex: bbc.co.uk)
2. Look at scrollbar on the right.

Actual Results:  
Scrollbar has upper state and upper arrow button should be dimmed.

Expected Results:  
Scrollbar has upper state and upper arrow button is not dimmed.

this bug is not reproducible if you open general console. You can notice that arrow buttons there works fine. It's because of they belong to the widget GtkScrollbar. But in our case mozilla draws arrow buttons separately.

Comment 1

[Anton Rogaynis (antonr)]

Attachment: this patch fixes partially that problem.

The patch fixes partially. Everything would be fine if arrow buttons drew before scrollbar did.

Comment 2

[Anton Rogaynis (antonr)]

Seems like the drawing order should be changed or mozilla should update arrow buttons in some cases after scrollbar drawn. 

Comment 3

[Anton Rogaynis (antonr)]

Mr. blizzard CCed

Comment 4

[Teune van Steeg]

(In reply to comment #2)
> Seems like the drawing order should be changed or mozilla should update arrow
> buttons in some cases after scrollbar drawn. 
> 

Reversing the drawing order wouldn't matter, it's just that the buttons won't get repainted/updated if the state doesn't change. 

And also when you scroll with the mouse wheel or drag the thumb and the beginning or end of the scrollbar is reached, only for the up/left buttons the paint function is called (but no visual painting is done).

So the buttons must be disabled/enabled in CSS or somewhere similar when scrolling reaches the end or beginning.

Comment 5

[Teune van Steeg]

(In reply to comment #4)
> So the buttons must be disabled/enabled in CSS or somewhere similar when
> scrolling reaches the end or beginning.
> 
Instead of in CSS, I meant in the scrollbar binding. 

The scrollbar buttons can also inherit curpos and maxpos in the binding, and then nsNativeThemeGTK::WidgetStateChanged could request a repaint when curpos changes on the scrollbar buttons. 

 

Comment 6

[Teune van Steeg]

Attachment: add a binding to handle scrollbar button disabling on Linux

This can probably be done in nsNativeThemeGTK as well: comparing curpos / maxpos in WidgetStateChanged to see if the button should repaint because the disabled state must change, and also in GetGtkWidgetAndState to actually change the disabled state.

But I personally like the solution in this patch better, because it involves comparing values only one time a value changes, and the scrollbar buttons /are/ actually disabled from xul/js/css perspective.

The only change in the new gnomestripe xulscrollbars.css compared to winstripes is the addition of the following lines:
> +/* ::::: disable non functional scrollbar buttons ::::: */
> +
> +scrollbar:not([disabled="true"]) > scrollbarbutton {
> +  -moz-binding: url("chrome://global/content/bindings/scrollbar.xml#scrollbar-button");
> +}

Comment 7

[Neil Deakin]

Comment on attachment 297782 [details] [diff] [review]
add a binding to handle scrollbar button disabling on Linux 

>+      <handler event="DOMAttrModified">
>+        <![CDATA[
>+          if (event.attrName != "curpos" && event.attrName != "maxpos")

This will enable mutation listeners on web pages, which we don't want as it will cause a significant performance penalty.

This is something that should really be done in nsSliderFrame

Comment 8

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

(In reply to comment #6)
> Created an attachment (id=297782) [details]
> add a binding to handle scrollbar button disabling on Linux 
> 
> This can probably be done in nsNativeThemeGTK as well: comparing curpos /
> maxpos in WidgetStateChanged to see if the button should repaint because the
> disabled state must change, and also in GetGtkWidgetAndState to actually change
> the disabled state.

Let's go with that please.

Comment 10

[Teune van Steeg]

Attachment: handle disabling in nsNativeThemeGTK

This patch sets the disabled attribute for a scrollbar button when the content is scrolled to the beginning or the end, and unsets it when it is scrolled away from the beginning or the end. At the same time the code is told to repaint the button, so that when painting the whole button is painted in the correct state.

When curpos is 0 or curpos is maxpos it is obvious a scrollbar button has to be repainted in the disabled state, if the button is at the correct end. But it is AFAICT impossible to get the previous curpos from WidgetStateChanged, so when curpos is neither 0 nor maxpos it is impossible to find out if curpos was before, (and thus the button needs repainting in the enabled state).

To have some way of knowing if the previous value was 0 or maxpos the disabled attribute is set (without notifying) when the current value is 0 or maxpos. This has the advantage that GetGtkWidgetAndState doesn't need to be modified, since the button will be painted disabled when the disabled attribute is set.

Comment 11

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

Is it really a problem if we just always repaint the buttons when scrolling?

Comment 12

[Teune van Steeg]

(In reply to comment #11)
> Is it really a problem if we just always repaint the buttons when scrolling?
> 
It just sounds suboptimal to me, but I really don't know. You're supposed to be the expert ;).

Assuming always repainting is the preferred solution, I'll attach an updated patch shortly.

Comment 13

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

Suboptimal is fine if it simplifies the code and the performance difference is not actually significant.

Comment 14

[Teune van Steeg]

Attachment: patch v2

Always repaint the scrollbar buttons when scrolling, and paint disabled when the scrollbar is scrolled to the corresponding end.

Comment 16

[Reed Loden [:reed]]

Checking in toolkit/content/widgets/scrollbar.xml;
/cvsroot/mozilla/toolkit/content/widgets/scrollbar.xml,v  <--  scrollbar.xml
new revision: 1.9; previous revision: 1.8
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.142; previous revision: 1.141
done

Comment 17

[Anton Rogaynis (antonr)]

verified.