414748 - Improve focus ring painting of native themed location bar

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Teune van Steeg]

Attachment: patch

As mentioned in bug 405210#c34
> Some themes (e.g. Glider and Crux) paint the ring outside of the dropdown

Currently only half of that ring is painted: outside the text field, the other half around the drop down button is not, because the theme's paint function lacks some information.

Attached patch fixes that.

It also fixes an issue where the painted background area is too small in the dropdown text field in the Crux Theme, and it fixes a mistake where I meant to make a gfloat a const, but made it static instead.

Comment 1

[Benjamin Berg]

If I read the patch correctly, then this will probably break the focus ring for the Industrial theme. It looks to me like you are increasing the focus size which is something the engine decides to do internally. You can't depend on every theme doing something like this, IMHO.

Comment 2

[Teune van Steeg]

(In reply to comment #1)
> If I read the patch correctly, then this will probably break the focus ring for
> the Industrial theme. It looks to me like you are increasing the focus size
> which is something the engine decides to do internally. You can't depend on
> every theme doing something like this, IMHO.
> 

The only thing this patch does for the focus ring is to set the GTK_HAS_FOCUS flag for the text field widget when the drop down button gets painted.

The size for the background of the text field (gtk_paint_flat_box) is increased by the border size to fix a glitch in the Crux Theme (a small area of missing white background between the text field and button). This fix has no effect on Industrial or any other theme I have tested.

Comment 3

[Benjamin Berg]

Ah, sorry, should have read it more carefully.

About that issue. GTK+ uses (two) X windows for the entire entry of the area (a smaller one for the text, and one for the whole area). Both of these have base[NORMAL] (or base[INSENSTIVE]) as the background. So it seems that crux in that case assumes that the whole area is already filled in with base[NORMAL]. A similar thing may also happen for spinbuttons I expect (or even any entry in general).
I'll open a bug about this whole window issue and maybe some other things soon. We need some workaround, and it will require theme/mozilla and in the long run maybe also GTK+ changes.

However, looking at it again I do see an (old) bug :-)
gtk_paint_flat_box should be passed the real state of the widget (either NORMAL or INSENSITIVE) instead of always NORMAL. (I personally would consider themes buggy if the current code works ...)

Comment 4

[Teune van Steeg]

Attachment: patch

(In reply to comment #3)
> However, looking at it again I do see an (old) bug :-)
> gtk_paint_flat_box should be passed the real state of the widget (either NORMAL
> or INSENSITIVE) instead of always NORMAL. (I personally would consider themes
> buggy if the current code works ...)
> 
Let's fix that in this bug too then... The patch is updated to contain that fix.
Chances are that this will fix bug 405458.

Comment 5

[Reed Loden [:reed]]

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

Requesting approval to land this after the tree reopens past beta 3. Improves focus ring painting of native themed location bar.

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

passing GTK_STATE_INSENSITIVE instead of GTK_STATE_NORMAL to gtk_paint_flat_box makes us draw a wrong light grey color with my version of Clearlooks (current debian testing). This is not visible in TheWidgetFactory, so we do something wrong, but I cannot tell what it is; our call now seems exactly the same as the one of gtk_entry_expose...

Comment 7

[Julien "_FrnchFrgg_" RIVAUD]

I note that if I put

style "test" {
 base[NORMAL] = "#FF0000"
 base[INSENSITIVE] = "#0000FF"
}
class "GtkEntry" style "test"

in my .gtkrc-2.0, in TheWidgetFactory entry backgrounds are wholly of the correct color, whereas in Firefox the gtk_paint_flat_box always draw a rectangle in the wrong color in the middle (apparently not the whole passed rectangle): white in the NORMAL case (so this is invisible without the custom style) and gray in the INSENSITIVE case (but lighter that the expected background, so this is visible without hack)

So our call to paint_flat_box is wrong. But how ? Benjamin, could you enlighten me a bit ?

Note: Is this call useful ? I mean, are there really themes needing this call ?

Comment 8

[Julien "_FrnchFrgg_" RIVAUD]

OK, comments 5 and 6 are completely wrong. I was testing the wrong firefox, so any change I made was useless... In fact, it seems to be an inner widget in the menulist which isn't transparent.

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

this is the html:input class="menulist-editable-input"

Comment 10

[Benjamin Berg]

Just about the paint_flat_box. I only know of one theme that uses it, and that is the Sugar GTK+ theme for the OLPC Laptop. It changes the background color for selected entries, and I have implemented that in paint_flat_box.
Most other engines will just draw the background color that is there already again.

Comment 11

[Teune van Steeg]

Attachment: patch

update to tip

Comment 12

[Damon Sicore (:damons)]

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

a1.9+=damons

Comment 13

[Dão Gottwald [:dao]]

No offense to roc, but in the future, could you please ask toolkit peers to review toolkit patches?

Comment 14

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

sorry

Comment 15

[Dão Gottwald [:dao]]

I didn't think there was a problem with this particular patch, but now that I look at it a second time: you shouldn't need the <field>, just use xbl:inherits to map "focused" to "_moz-input-focused".

Comment 16

[Teune van Steeg]

Attachment: set attribute by inheriting the focused attribute

Address comment #15, and revert a part of bug 409388 to make the focused attribute be inherit instead of setting it on the focus event.

Comment 17

[Neil Deakin]

Comment on attachment 302068 [details] [diff] [review]
set attribute by inheriting the focused attribute

>         }
>+
>+        // When the input field of the drop down button has focus, some themes
>+        // should draw focus for the drop down button as well.
>+        if (aWidgetType == NS_THEME_DROPDOWN_BUTTON && aWidgetFlags) {
>+          *aWidgetFlags = CheckBooleanAttr(aFrame, nsWidgetAtoms::mozinputfocused);
>+        }

This isn't related to this bug, but I wonder why the conditional blocks in this method aren't in 'else if' blocks.

attribute
> WIDGET_ATOM(mozinputchecked, "_moz-input-checked")
>+WIDGET_ATOM(mozinputfocused, "_moz-input-focused")

No reason to use an internal name here. You should use a real attribute such as 'parentfocused'. The other one 'mozinputchecked' doesn't seem to be used that I can see, it could be removed if so.

Comment 18

[Teune van Steeg]

Attachment: final patch

Addressing the review comments: use parentfocused instead of _moz-input-focused, and removes the unused mozinputchecked atom.

Comment 19

[Reed Loden [:reed]]

Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.114; previous revision: 1.113
done
Checking in toolkit/content/widgets/menulist.xml;
/cvsroot/mozilla/toolkit/content/widgets/menulist.xml,v  <--  menulist.xml
new revision: 1.39; previous revision: 1.38
done
Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.75; previous revision: 1.74
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.143; previous revision: 1.142
done
Checking in widget/src/xpwidgets/nsWidgetAtomList.h;
/cvsroot/mozilla/widget/src/xpwidgets/nsWidgetAtomList.h,v  <--  nsWidgetAtomList.h
new revision: 1.24; previous revision: 1.23
done