415830 - Combobox appearance should match the native ComboBox widget and not OptionMenu

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Julien "_FrnchFrgg_" RIVAUD]

Almost all behaviour and rendering of our non editable combobox (the popup width and horizontal alignment, etc...) matches the native ComboBox widget, yet the rendering is that of an OptionMenu, which is deprecated since gtk 2.4.

Expected results: Comboboxes look like ComboBoxes
Actual results: Comboboxes look like OptionMenus
Reproducible: Always

Comment 1

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v1

This patch needs the patch in bug 416003.

Comment 2

[Teune van Steeg]

Comment on attachment 302204 [details] [diff] [review]
Patch v1

>+static gint
>+ensure_combo_box_arrow_widget()
> 
>+[..]
> 
>+static gint
>+ensure_combo_box_separator_widget()

Why not ensure both widgets in the same function, to avoid code duplication?

>+    ensure_combo_box_arrow_widget();
>+    gtk_widget_set_direction(gComboBoxButtonWidget, direction);

Nit: the direction is already set in moz_gtk_button_paint, I actually don't think it is needed to set it twice.

>+    /* Now arrow_rect contains the inner rect ; since the arrow width should
>+     * only be "arrow-size" (see gtk_combo_box_size_request), we need to
>+     * adjust it */
>+    gtk_widget_style_get (gComboBoxWidget, "arrow-size", &arrow_size, NULL);

The arrow-size property is only available since GTK 2.12, so we can not get the size this way.

>+    }
>+    break;
>+  case NS_THEME_DROPDOWN:
>+    {
>+      moz_gtk_get_combo_box_size(&aResult->width, &aResult->height);
>       *aIsOverridable = PR_FALSE;

Interestingly enough HTML ComboBoxes consider GetMinimumWidgetSize as being the absolute maximum size: with this patch the ComboBoxes in HTML contain no text and are only as big as the arrow and the separator (the minimum size).
Setting *aIsOverridable to PR_TRUE seems to help.

Further more I think we should get rid of all code involving gOptionMenuWidget, therefore we should use gComboBoxWidget to return the border sizes in moz_gtk_get_widget_border. 
A quick look at the GTK+ code tells me that for the gComboBoxWidget we probably need to return the combobox's border-width, focus-padding and focus-line-width and the button's xthickness and ythickness. And I guess we need to add the separator width and the requisition of the arrow widget to the border left value or right value, depending on the text direction, in order to not overflow text in the arrow's region

Comment 3

[Julien "_FrnchFrgg_" RIVAUD]

Yeah, I forgot to change that last use of OptionMenu.

Since all code in GetWidgetMinimumSize set aIsOverridable to PR_FALSE, and I didn't notice anything wrt maximum size (as when I enlarged the editable menulist button via CSS), I'm a little puzzled by your findings. I think we need the help of a guru here. roc, what is aIsOverridable for ?

Regarding the arrow size, I'll look to see what is done in gtk < 2.12 (probably an hardcoded value)

I'll merge the two ensure functions; ensure_combo_box_separator_and_arrow_widgets() ?

Comment 4

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2

I addressed comments, and suppressed the problem of aIsOverridable by letting dropdown use the same generic codepath as button (it works perfectly, so I don't think special casing is needed)

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

Note that in moz_gtk_get_widget_border, I can check for inhtml to avoid adding too much border for HTML combos, as it done for the MOZ_GTK_BUTTON

Comment 6

[Julien "_FrnchFrgg_" RIVAUD]

Sorry, the OS widget is the one I was playing with to see if the border was enough to make the text not approach the separator and arrow.

Comment 7

[Teune van Steeg]

Comment on attachment 302450 [details] [diff] [review]
Patch v2

>+    inner_rect->x = rect->x + XTHICKNESS(style) + focus_width + focus_pad;
>+    inner_rect->x += direction = GTK_TEXT_DIR_LTR ?

You copied over a mistake I made in an attachment of bug 415163, the current attachment there has the correct code (should be: direction == GTK_TEXT_DIR_LTR)

>+    inner_rect->y = rect->y + inner_border.top + YTHICKNESS(style) +
>+                   focus_width + focus_pad;
Fix the alignment  

>+    gtk_widget_set_direction(gComboBoxArrowWidget, direction);
>+    gtk_widget_set_direction(gComboBoxSeparatorWidget, direction);
This is actually not needed: when the direction is set on the button, the direction is inherited by it's children. (Except for QtCurve, since those widgets are not children of the button there. But that doesn't matter anyhow, because QtCurve paints its own arrow internally, ignoring the paint_arrow call from here).

>+            *left = *top = focus_width + focus_pad
>+                         + GTK_CONTAINER(gComboBoxButtonWidget)->border_width;
Operators shouldn't be at the beginning of a line; move the '+' to the end of the previous line.  

>-    gOptionMenuWidget = NULL;
Don't forget to cleanup gComboBoxWidget, gComboBoxButtonWidget,  gComboBoxArrowWidget and gComboBoxSeparatorWidget.


(In reply to comment #3)
> Since all code in GetWidgetMinimumSize set aIsOverridable to PR_FALSE,
Note that the way you choose to go now doesn't set aIsOverridable to PR_FALSE either.

> didn't notice anything wrt maximum size (as when I enlarged the editable
> menulist button via CSS), I'm a little puzzled by your findings.
The problem was that non styled HTML comboboxes were to small to contain text, and are only the minimum size. Did this not happen for you?

> what is aIsOverridable for ?
AFAIK aIsOverridable is for when there is no room to paint the widget in it's minimum size, the code is allowed to shrink the widget, or not when aIsOverridable = PR_FALSE.

(In reply to comment #5)
> Note that in moz_gtk_get_widget_border, I can check for inhtml to avoid adding
> too much border for HTML combos, as it done for the MOZ_GTK_BUTTON
> 
That is not necessary in my opinion, the current size is small enough.

Comment 8

[Julien "_FrnchFrgg_" RIVAUD]

(In reply to comment #7)
> (From update of attachment 302450 [details] [diff] [review])
> >+    inner_rect->x = rect->x + XTHICKNESS(style) + focus_width + focus_pad;
> >+    inner_rect->x += direction = GTK_TEXT_DIR_LTR ?
> 
> You copied over a mistake I made in an attachment of bug 415163, the current
> attachment there has the correct code (should be: direction ==
> GTK_TEXT_DIR_LTR)

Corrected.

> >+    inner_rect->y = rect->y + inner_border.top + YTHICKNESS(style) +
> >+                   focus_width + focus_pad;
> Fix the alignment

Done.

> >+    gtk_widget_set_direction(gComboBoxArrowWidget, direction);
> >+    gtk_widget_set_direction(gComboBoxSeparatorWidget, direction);
> This is actually not needed: when the direction is set on the button, the
> direction is inherited by it's children. (Except for QtCurve, since those
> widgets are not children of the button there. But that doesn't matter anyhow,
> because QtCurve paints its own arrow internally, ignoring the paint_arrow call
> from here).

I'll remove them if you're sure. Note that ThinIce also draws its arrow as part of the button paint, and ignores completely the separator and the arrow paint (IIRC).

> >+            *left = *top = focus_width + focus_pad
> >+                         + GTK_CONTAINER(gComboBoxButtonWidget)->border_width;
> Operators shouldn't be at the beginning of a line; move the '+' to the end of
> the previous line.  

OK.

> >-    gOptionMenuWidget = NULL;
> Don't forget to cleanup gComboBoxWidget, gComboBoxButtonWidget, 
> gComboBoxArrowWidget and gComboBoxSeparatorWidget.

Ah, yeah. Bad _FrnchFrgg_, no cookie.


> The problem was that non styled HTML comboboxes were to small to contain text,
> and are only the minimum size. Did this not happen for you?

Yes, it happened, I just didn't see at first that you had the bug only in HTML.

> > what is aIsOverridable for ?
> AFAIK aIsOverridable is for when there is no room to paint the widget in it's
> minimum size, the code is allowed to shrink the widget, or not when
> aIsOverridable = PR_FALSE.

So the bug would indeed be a bug in existing code, and not in my use of it...

> (In reply to comment #5)
> > Note that in moz_gtk_get_widget_border, I can check for inhtml to avoid adding
> > too much border for HTML combos, as it done for the MOZ_GTK_BUTTON
> > 
> That is not necessary in my opinion, the current size is small enough.

I think too it's not necessary (or else I'd have put it in the patch), but I know that since it enlarges the layout (visible in bugzilla for instance), some wouldn't like it.

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2.1

With comments addressed, unnecessary gtk_widget_set_direction removed, and a comment put instead.

Comment 10

[Reed Loden [:reed]]

Just so I know what is being changed here, would you mind attaching a screenshot for my own personal benefit of what Firefox currently does versus what it should be doing?

Comment 11

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Difference between OptionMenus and ComboBoxes

The difference is generally minimal (I'd even say OptionMenus are somewhat prettier for some themes). The thing is there is room for difference (an engine could theme completely differently one from another), and since OptionMenus are deprecated, engines are bound to care less about them on the not-so-long term, even if I suppose they'll still render them correctly for compatibility purposes.

(I also think this patch improves the maintainability of this code, but it was not your question.)

Comment 12

[Reed Loden [:reed]]

Cool, thanks!

Comment 13

[Teune van Steeg]

Comment on attachment 302639 [details] [diff] [review]
Patch v2.1

>+    calculate_arrow_rect(gComboBoxEntryArrowWidget,
>+                         &arrow_rect, &real_arrow_rect, direction);
> 
>     style = gComboBoxEntryArrowWidget->style;
>     TSOffsetStyleGCs(style, real_arrow_rect.x, real_arrow_rect.y);
 
This patch indicates that the last two lines above should be already present in the code, but they are not and they are needed to make this patch work. 

At first I thought the patch not applying cleanly for me had something to do with my own patches in my tree, but these two lines are also missing in the patch from bug 416003, that this patch should be applied on top of.

The r=me only counts when you correct this :)

Comment 14

[Julien "_FrnchFrgg_" RIVAUD]

The second line is added in by your patch (attachment 302458 [details] [diff] [review] from bug 415163); the first line should be modified by the patch of bug 416003 from

style = gArrowWidget->style; (added by your patch)

to

style = gComboBoxEntryArrowWidget->style;

I'll attach new versions updated to tip (and on top of bug 415163); it's strange since in my repository those line are correctly added, and I just rebased with git... I must have done something wrong while posting my patches.

Comment 15

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2.2

Can you verify that this one is correct ?

Comment 16

[Teune van Steeg]

Comment on attachment 302838 [details] [diff] [review]
Patch v2.2

OK, no problems now :)

Comment 17

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v3

This patch has been overhauled to be rock-solid against crash possibilities as was done for bug 415163. It is to be applied on top of both bug 415163 and bug 416003 patches.

Comment 18

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v3.1

Small change to make bug 416868 independent from this patch

Comment 19

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v3.5

Patch updated to latest trunk and to reflect what has been done for the ComboBoxEntry.

Comment 20

[Julien "_FrnchFrgg_" RIVAUD]

Note that this patch is to be applied on top of the approved patch of bug 416003.

Comment 21

[Michael Ventnor]

Comment on attachment 308060 [details] [diff] [review]
Patch v3.5


>             if (direction == GTK_TEXT_DIR_RTL)
>-                *left += indicator_spacing.left + indicator_size.width + indicator_spacing.right;
>+                *left += separator_width + arrow_req.width;
>             else
>-                *right += indicator_spacing.left + indicator_size.width + indicator_spacing.right;
>+                *right += separator_width + arrow_req.width;
>+

This worries me a tiny bit, are you sure the text will stop as soon as it reaches the separator before the arrow? Did you test this in Clearlooks and on a widget with text overflow, like the country selector at the bottom right corner of http://store.apple.com/1-800-MY-APPLE/WebObjects/AppleStore ?

Comment 22

[Michael Ventnor]

The reason I'm suspicious is because I'm not sure whether that arrow_req.with includes the spacing.

Comment 23

[Julien "_FrnchFrgg_" RIVAUD]

The text doesn't overwrite the arrow, nor the separator (it seems to stop exactly at the separator). In fact, I'm sure that arrow_req.width includes all I need, since I draw it that way (and gtk does the same). I ask the arrow to paint itself in a rect of "arrow_rect.width" width, and I draw the separator just next to it.

Comment 24

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

(In reply to comment #3)
> Yeah, I forgot to change that last use of OptionMenu.
> 
> Since all code in GetWidgetMinimumSize set aIsOverridable to PR_FALSE, and I
> didn't notice anything wrt maximum size (as when I enlarged the editable
> menulist button via CSS), I'm a little puzzled by your findings. I think we
> need the help of a guru here. roc, what is aIsOverridable for ?

Sorry I missed this question. When aIsOverridable is set to false, the value you return is the only size the widget can have. When aIsOverridable is set to true, the value you return is only the minimum size, layout can make it bigger.

There may be a bug in combobox layout that means aIsOverridable==false does something weird with comboboxes. No platform has ever tested that, nor should they!

Comment 25

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

Comment on attachment 308060 [details] [diff] [review]
Patch v3.5

+    if (!gComboBoxButtonWidget ||
+        !gComboBoxArrowWidget) {

Turn this into "if (... && ...) return" to save indenting

The rest looks OK but Michael will do a better review than I can

Comment 26

[Julien "_FrnchFrgg_" RIVAUD]

I'll post an updated patch when Michael posts his review, to address all comments at once.

Comment 27

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: patch v3.6

With review comments addressed.

Comment 28

[Julien "_FrnchFrgg_" RIVAUD]

For drivers:

This patch changes the rendering of non-editable menulists; currently Gecko uses an "OptionMenu" widget (deprecated in recent GTKs), the patch replaces it by a "ComboBox" widget.

The difference in rendering is generally minimal (I'd even say OptionMenus are somewhat prettier for some themes). The thing is there is room for difference (an engine could theme one completely differently from another), and since OptionMenus are deprecated, engines are bound to care less about them on the not-so-long term, even if I suppose they'll still render them correctly for compatibility purposes.

This patch also improves the maintainability of this code.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 309297 [details] [diff] [review]
patch v3.6

a1.9=beltzner

Comment 30

[Reed Loden [:reed]]

Checking in widget/src/gtk2/gtk2drawing.c;
/cvsroot/mozilla/widget/src/gtk2/gtk2drawing.c,v  <--  gtk2drawing.c
new revision: 1.97; previous revision: 1.96
done
Checking in widget/src/gtk2/nsNativeThemeGTK.cpp;
/cvsroot/mozilla/widget/src/gtk2/nsNativeThemeGTK.cpp,v  <--  nsNativeThemeGTK.cpp
new revision: 1.154; previous revision: 1.153
done

Comment 31

[Ginn Chen]

FWIW:
It looks very bad with GNOME Nimbus theme.
Other themes are fine.

Comment 32

[Julien "_FrnchFrgg_" RIVAUD]

Ginn: How do ComboBoxes look in this theme ? Can you post a screenshot of twf for example ?

Comment 33

[Ginn Chen]

Attachment: screenshot with nimbus theme

I don't know whether it is a bug of nimbus or Firefox.

The arrow is not rendered at first. (like the Status combobox in screenshot)
If I focus or dropdown the combobox, the arrow is rendered at middle and top of the combobox. (like the Product combobox in screenshot)

Comment 34

[Marcus Husar]

I (respectively Bluefang from MozillaZine Forums) discovered that there were some recent changes to the code of the combobox that perhaps caused some problems. They occur with the appearances "Clearlooks" and "Glossy" under Gnome (Debian testing). The text in the combobox is a little cut.

a screenshot of the german news site "heise.de":
http://img137.imageshack.us/my.php?image=heiserw9.png

a native combobox from the preferences of Rhythmbox:
http://img139.imageshack.us/my.php?image=rhythmboxcm7.png

The nightly build of Minefield three days ago (2008-03-16) didn't have any problems with the combobox.