Closed Bug 395564 Opened 14 years ago Closed 13 years ago

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

Categories

(Core :: Widget: Gtk, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9beta4

People

(Reporter: wildriding, Assigned: twanno)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Summary: scrollbar's arrow buttons doesn't support theming completely → scrollbar's arrow buttons don't support theming completely
The patch fixes partially. Everything would be fine if arrow buttons drew before scrollbar did.
Seems like the drawing order should be changed or mozilla should update arrow buttons in some cases after scrollbar drawn. 
Mr. blizzard CCed
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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. 

 
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");
> +}
Assignee: nobody → twanno
Status: NEW → ASSIGNED
Attachment #297782 - Flags: superreview?(roc)
Attachment #297782 - Flags: review?(enndeakin)
Summary: scrollbar's arrow buttons don't support theming completely → scrollbar's arrow buttons should be disabled when they can't scroll further
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
Attachment #297782 - Flags: review?(enndeakin) → review-
(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.
Duplicate of this bug: 414233
Version: unspecified → Trunk
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.
Attachment #297782 - Attachment is obsolete: true
Attachment #301183 - Flags: superreview?(roc)
Attachment #301183 - Flags: review?(roc)
Attachment #297782 - Flags: superreview?(roc)
Is it really a problem if we just always repaint the buttons when scrolling?
(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.
Suboptimal is fine if it simplifies the code and the performance difference is not actually significant.
Attached patch patch v2Splinter Review
Always repaint the scrollbar buttons when scrolling, and paint disabled when the scrollbar is scrolled to the corresponding end.
Attachment #301183 - Attachment is obsolete: true
Attachment #301416 - Flags: superreview?(roc)
Attachment #301416 - Flags: review?(roc)
Attachment #301183 - Flags: superreview?(roc)
Attachment #301183 - Flags: review?(roc)
Attachment #301416 - Flags: superreview?(roc)
Attachment #301416 - Flags: superreview+
Attachment #301416 - Flags: review?(roc)
Attachment #301416 - Flags: review+
Attachment #301416 - Flags: approval1.9?
Duplicate of this bug: 412865
Attachment #301416 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.