410489 - gtk: wrong scrollbar in nodoka theme (git)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[u294409]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; pl; rv:1.9b3pre) Gecko/2008010204 Fedora/8 (Werewolf) Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; pl; rv:1.9b3pre) Gecko/2008010204 Fedora/8 (Werewolf) Minefield/3.0b3pre

nodoka is evolved from murrine gtk engines.
from 0.7 alpha 1 or 2 it provides nicer focus than dotted ring and new, macish scrollbars.

firefox doesn't use both features. i have screenshot to compare focused buttons and scrollbars.

Reproducible: Always

Actual Results:  
wrong appearance.

Expected Results:  
gtk-drawn widgets.

i have screenshot.

Comment 1

[u294409]

Attachment: screenshot

Comment 2

[Martin Sourada]

Hi,

I am the maintainer of nodoka gtk engine. The new scrollbars design is disabled if parent widget is GtkFixed due to buggy behaviour. Would be better (IMHO) if you used GtkScrolledWindow for scrolling in frames/pages (I noticed WebKit uses it).

In the matter of wrong display of focus, I didn't investigated it much, but seems like bug in firefox. I check for focus via 

widget && GTK_WIDGET_HAS_FOCUS (widget)

where widget is GtkWidget that is meant to be drawn. When focus is not found to be set, old focus handling is used (i.e. is drawn independently on widget using the old dotted style).

Comment 3

[Reed Loden [:reed]]

GTK Widget Guys (GWGs :P), take a look at the theme maintainer's comment above.

Comment 4

[u294409]

Sometimes I see doubled focus. New Clearlooks'/Nodoka's style outlined with dotted outline. Especially on close buttons and buttons in "findbar" at bottom.

Comment 5

[Teune van Steeg]

(In reply to comment #4)
> Sometimes I see doubled focus. New Clearlooks'/Nodoka's style outlined with
> dotted outline. Especially on close buttons and buttons in "findbar" at bottom.
> 

That is bug 410789

Comment 6

[u294409]

I know, I've filed both this and 410789 bug ;) .

Comment 7

[Ian Spence]

Could you stop that? Personally, I keep a listing of bugs reported by certain people, and the last thing I need is the same person reporting a bug multiple times in various places.

Comment 8

[Teune van Steeg]

I tested this with Nodoka 0.7 beta 1, and there seems nothing wrong with focus on the buttons. 

The button from Minefield in your screenshot has a smaller focus ring because it is the default button in that dialog, and it didn't have focus at that moment.

So I'm adjusting the summary to make it clear that the only problem here is the scrollbar.

Comment 9

[Martin Sourada]

I think these issues are similar so letting people in this bug now that there is another similar issue, i.e. bug 410789, might be helpful, but only if he mentioned the bug number with it...

But back to this bug, I handled the focus representation via gtk_draw_focus function (which 'fixes' the non-gtk focus problem in Nodoka theme), so this bug should be rather about properly letting know libraries/programmes of the focus state of widgets, since we can draw the focus when its parent widget is drawn which is faster and gives us better starting position for widget focus theming.

That is especially useful for CheckButtons (and RadioButtons) which have currently different focus handling when in gtk application (I handle these when the widget is drawn, so its done by styling the check box), when in firefox's setting (focus is drawn for the label) and when in web pages displayed by firefox (focus ring is drawn for the check box).

My conclusion is that this bug should be about widgets correctly reporting their focus state when asked for via GTK_WIDGET_HAS_FOCUS (widget). More info can be get at 

http://library.gnome.org/devel/gtk/unstable/GtkWidget.html

Comment 10

[Ian Spence]

Apparently we only use GTK_HAS_FOCUS for the entry widget. The fix for that bug, which doesn't really belong in this bug, would likely be to just create a function that takes in a GtkState and a widget, and sets the state options accordingly

Comment 11

[Martin Sourada]

About the scrollbar issue. I told it already, but I disabled the new styling when parent is GtkFixed because in older versions it behaves buggy (it seems that wrong stepper IDs are returned when I check for them) and in latest FF3 I tried it with it didn't draw the rounded part of steppers when sliding. 

As for the steppers ID it is also wrong in latest firefox, but only for the secondary steppers. I didn't explore it further, just treat the as if I didn't know which one it is. But as the checking for steppers is done via gdk_rectangle_intersect where I the intersecting rectangles positions and sizes are counted from widget->allocation.{x,y} and given x, y, width and height of drawn widget (that are obviously correct, since the stepper is drawn on proper place with proper size), I suspect the problem could be with wrong widget->allocation.

In previous versions on Nodoka and Clearlooks engines (and most of current versions of other engines) is a (now obsolete) workaround for drawing proper background for buttons when in firefox, which was initiated when widget->allocation.x and widget->allocation.y was -1... So this only supports (IMO) my idea.

Comment 12

[Martin Sourada]

Oh, and I forgot, it also make use of GTK_RANGE (widget)->has_stepper_{a,b,c,d} to check which steppers the scrollbar has. Maybe the issue is rather here...

Comment 13

[Teune van Steeg]

(In reply to comment #11)
> About the scrollbar issue. I told it already, but I disabled the new styling
> when parent is GtkFixed because in older versions it behaves buggy (it seems
> that wrong stepper IDs are returned when I check for them)
I compiled 0.7 beta 1 without disabling the new styling. Indeed in older versions of Firefox (Firefox 2 in my case), the steppers are not rounded. This is as you said probably because widget->allocation.{x,y} is -1. However that is fixed in current trunk.

In current trunk the only issue I see is, as you mentioned, with the secondary steppers not being rounded. But this is not fixed by setting the scrollbars as child of GtkScrolledWindow.

> and in latest FF3 I
> tried it with it didn't draw the rounded part of steppers when sliding. 
> 
I didn't see this with latest trunk, can you explain how and where you saw this?

I tested with and without setting GtkScrolledWindow as parent of the scrollbars. And it doesn't seem to make a difference as how the scrollbar/steppers/through is being drawn (with the patched theme).

So I think it would be best that Nodoka would just enable the new styling everywhere, and that we fix it so that Nodoka gets the secondary steppers ID correctly. And maybe the other problem I didn't see.

Comment 14

[Martin Sourada]

(In reply to comment #13)
> In current trunk the only issue I see is, as you mentioned, with the secondary
> steppers not being rounded. But this is not fixed by setting the scrollbars as
> child of GtkScrolledWindow.
> 
I don't know how you implement it, but I believe GtkScrolledWindow already contains it's pair of scrollbars, so there's no point in adding additional ones to it.

> > and in latest FF3 I
> > tried it with it didn't draw the rounded part of steppers when sliding. 
> > 
> I didn't see this with latest trunk, can you explain how and where you saw
> this?
> 
See the screenshot I'll attach. It seems to happen only when using slider to slide and mostly only when you start with slider on one of the two ends of scrollbar.

> I tested with and without setting GtkScrolledWindow as parent of the
> scrollbars. And it doesn't seem to make a difference as how the
> scrollbar/steppers/through is being drawn (with the patched theme).
> 
> So I think it would be best that Nodoka would just enable the new styling
> everywhere, and that we fix it so that Nodoka gets the secondary steppers ID
> correctly. And maybe the other problem I didn't see.
> 
Ok, will be enabled with next git commit.

Comment 15

[Martin Sourada]

Attachment: Rendering problem when sliding

Comment 16

[Teune van Steeg]

(In reply to comment #14)
> I don't know how you implement it, but I believe GtkScrolledWindow already
> contains it's pair of scrollbars, so there's no point in adding additional ones
> to it.
Sorry for the misunderstanding, I was using the scrollbars that are made available by GtkScrolledWindow.
 
> See the screenshot I'll attach. It seems to happen only when using slider to
> slide and mostly only when you start with slider on one of the two ends of
> scrollbar.
OK, I see this too.

> Ok, will be enabled with next git commit.
> 
Thanks

Comment 17

[Teune van Steeg]

Attachment: make it possible for GTK themes to distinguish secondary steppers

This patch adds distinction between the four possible scrollbar button positions, and it places the buttons in the fake scrollbar allocation accordingly.

Comment 18

[Teune van Steeg]

About the second problem: It seems the scrollbar through is painted on top of the extra scrollbar button rounding. I'm not sure if it is possible to fix that.

Comment 19

[Teune van Steeg]

Attachment: make it possible for GTK themes to distinguish secondary steppers

Now with a small error fixed.

Comment 20

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

I would make UP/DOWN and TOP/BOTTOM be bits instead of enumerating them all. Same for ScrollbarButtonType, use two bools instead or an int with two flag bits. I think that would simplify code. Even better if you can arrange for ScrollbarButtonType to have the same layout as the GTK flags so we don't have to convert.

I think we used to have a gap between the stepper allocations, now we don't. Probably we should use the height*5 etc.

Comment 21

[Teune van Steeg]

(In reply to comment #20)
> I think we used to have a gap between the stepper allocations, now we don't.
> Probably we should use the height*5 etc.
> 
The gap was there so that the bottom down scrollbar button would not be mistaken for the top down scrollbar button, if I'm correct.

Assuming a vertical scrollbar, the GTK theme looks first for a button at the top of the allocation (top up) then for a button at the top + button size of the allocation (top down) and then at the bottom - button size of the allocation (bottom up) and finally at the bottom of the allocation (bottom down). This is covered by the patch in its current state (four quarters).

Comment 22

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

Isn't that just what most themes happen to do? Maybe it would be more robust if there was a gap in case there are themes that do something else.

Comment 24

[Teune van Steeg]

Attachment: make it possible for GTK themes to distinguish secondary steppers (v2)

I hope this is something like you had in mind. 
Also I got tired of having to type GTK_WIDGET(scrollbar) every time, so since scrollbar is never used as a GtkScrollbar here, I changed it to be a GtkWidget.

Comment 25

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

+  switch (aFrame->GetContent()->FindAttrValueIn(kNameSpaceID_None,
+                                                nsWidgetAtoms::sbattr,
+                                                strings, eCaseMatters)) {
+    case 0: return eScrollbarButton_Down | eScrollbarButton_Bottom;
+    case 1: return eScrollbarButton_Down;
+    case 2: return eScrollbarButton_Bottom;
+    case 3: return eScrollbarButton_UpTop;
+  }

You can be even more clever here and use an array instead of a switch. Don't forget to check for a -1 result though.

+typedef enum {
+  MOZ_GTK_STEPPER_DOWN        = 1 << 0,
+  MOZ_GTK_STEPPER_BOTTOM      = 1 << 1,
+  MOZ_GTK_STEPPER_VERTICAL    = 1 << 2
+} GtkScrollbarButtonFlags;

Instead of keeping these in sync with ScrollbarButtonType, why not just use ScrollbarButtonType directly?

+                  scrollbar, (flags & MOZ_GTK_STEPPER_VERTICAL) ?
+                    "vscrollbar" : "hscrollbar",

Factor the widget name into a local const char*?

Otherwise looks good.

Comment 26

[Teune van Steeg]

(In reply to comment #25)
> You can be even more clever here and use an array instead of a switch. Don't
> forget to check for a -1 result though.
>
I was thinking, maybe it is better to also define eScrollbarButton_Top (1 << 2) and eScrollbarButton_Up (1 << 3), in case this may be used in another way on other platforms someday?

> 
> Instead of keeping these in sync with ScrollbarButtonType, why not just use
> ScrollbarButtonType directly?
> 
If I understand you correctly, that would mean nsNativeTheme.h has to be included in gtkdrawing.h, is that even possible (considering C vs. C++)?

Comment 27

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

Comment on attachment 296323 [details] [diff] [review]
make it possible for GTK themes to distinguish secondary steppers (v2)

this is fine then

Comment 28

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

Comment on attachment 296323 [details] [diff] [review]
make it possible for GTK themes to distinguish secondary steppers (v2)

this is fine then

Comment 29

[Reed Loden [:reed]]

Comment on attachment 296323 [details] [diff] [review]
make it possible for GTK themes to distinguish secondary steppers (v2)

Fix an issue with native theming with some themes.

Comment 30

[Reed Loden [:reed]]

Teune, is this ready, or do you want to address any other comments/nits first?

Comment 31

[Teune van Steeg]

Attachment: make it possible for GTK themes to distinguish secondary steppers (for checkin)

(In reply to comment #30)
> Teune, is this ready, or do you want to address any other comments/nits first?

I changed this part:
> +                  scrollbar, (flags & MOZ_GTK_STEPPER_VERTICAL) ?
> +                    "vscrollbar" : "hscrollbar",
> 
> Factor the widget name into a local const char*?

Comment 32

[u294409]

/me is clapping my hands

Bravo, bravissimo guys!

It works. Can we close this bug now?

Comment 33

[Reed Loden [:reed]]

(In reply to comment #32)
> Bravo, bravissimo guys!
> 
> It works. Can we close this bug now?

You tested the patch, and it works well?

Comment 34

[u294409]

I'm using trunk. Here everything works well.

Comment 35

[Reed Loden [:reed]]

(In reply to comment #34)
> I'm using trunk. Here everything works well.

The patch above hasn't landed yet. If everything is working well for you, what does this patch fix then?

Comment 36

[u294409]

I don't know. This bug is 10 days old. Something might happen in Nodoka's GIT repository.

I see the usual GTK scrollbar in Firefox too.

Comment 37

[Teune van Steeg]

(In reply to comment #35)
> (In reply to comment #34)
> > I'm using trunk. Here everything works well.
> 
> The patch above hasn't landed yet. If everything is working well for you, what
> does this patch fix then?
> 

This patch fixes the correct appearance for secondary steppers in the scrollbar (e.g. a down button just beneath the up arrow at the top of the scrollbar). Because Firefox did not give the correct information to the theme, the theming for the scrollbar buttons was disabled for Firefox in Nodoka.

But the theming for the scrollbar buttons have been enabled again in Nodoka trunk (comment #14), because I promised to fix the problems with the secondary steppers (what this patch does).

It probably seems fixed because(In reply to comment #36)
> I don't know. This bug is 10 days old. Something might happen in Nodoka's GIT
> repository.
> 
> I see the usual GTK scrollbar in Firefox too.
> 
Because you probably don't use secondary steppers, it seems fixed.

Comment 38

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.65; previous revision: 1.64
done
Checking in widget/src/gtk2/gtkdrawing.h;
/cvsroot/mozilla/widget/src/gtk2/gtkdrawing.h,v  <--  gtkdrawing.h
new revision: 1.49; previous revision: 1.48
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.135; previous revision: 1.134
done
Checking in widget/src/xpwidgets/nsNativeTheme.cpp;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.cpp,v  <--  nsNativeTheme.cpp
new revision: 1.44; previous revision: 1.43
done
Checking in widget/src/xpwidgets/nsNativeTheme.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsNativeTheme.h,v  <--  nsNativeTheme.h
new revision: 1.27; previous revision: 1.26
done