118294 - NS_THEME_DROPDOWN* implementations (GTK)

Status: RESOLVED FIXED

(Core Graveyard :: Skinability, enhancement)

Description

[Hixie (not reading bugmail)]

This covers the implementation of NS_THEME_DROPDOWN and NS_THEME_DROPDOWN_BUTTON
on GTK.

Comment 1

[Pierre Chanial]

taking

Comment 2

[Pierre Chanial]

Attachment: toolkit/skin/gtk2/menulist.css

Comment 3

[Pierre Chanial]

Attachment: patch v1.0

This patch implements NS_THEME_DROPDOWN and NS_THEME_DROPDOWN_TEXT.
The latter is just there in case a background is set on the label box.
We can also decide not to support it since now, we have gtk2 skin files that
are not contaminated by non native css rules. It's up to you.

For gtk, combo box are an entry and arrow widget side by side, not a arrow
inside an entry as we are currently displaying. The css file attached above
corrects it for menulists. I'll update the location bar after.

Comment 4

[Pierre Chanial]

there was two gcc warnings that I corrected: I "constify" indicator_size and
indicator_spacing in moz_gtk_option_menu_get_metrics calling sequence and in its
callers.

Also, to ease the review: the menulist appearance stands for a dropdown
(menulist non editable). Its gtk equivalent is an option_menu.
The editable menulist  consists of a gtk_entry and a gtk arrow that are
specified resp. by the menulist-textfield and menulist-button appearance.

Comment 5

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

Comment on attachment 142990 [details] [diff] [review]
patch v1.0

+    if (!interior_focus)
+	 *interior_focus = default_interior_focus;

er ... this looks like an instant seg fault if interior_focus is null. I'm not
sure what you're trying to do here. Probably should just get rid of all this
default stuff? sr=roc if you do that

Comment 6

[chris hofmann]

think this has been landed. reopen if its not the case.  thanks chofmann

Comment 7

[Ganesh]

I just pulled the latest firefox source and verified . But I don't see this
patch in that. Any specific reason?

Comment 8

[Pierre Chanial]

Right, I have an updated patch, but I have been *really* busy these days.
I'll upload it hopefully this week.

Comment 9

[Ganesh]

Any chance you have the latest patch for this. If not can you put your updated
patch with appropriate comments so that I can work on it. Thanks..

Comment 10

[Pierre Chanial]

Attachment: patch v2.0

Here is the patch that I lamely kept for so long...

Comment 11

[Pierre Chanial]

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

Re-requesting reviews, since I've been away from a while.

Comment 12

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

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

++  skin/classic/global/menulist.css

What's this about? You didn't change that file int the patch.

Comment 13

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

Oh I see it, never mind. 

Comment 14

[Pierre Chanial]

-> fixed.
I added the menulist-description style in the menulist.css file attached below
that was introduced after the first version of this patch (and wasn't in the new
toolkit skins neither).

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

a=asa for branch checkin.

Comment 16

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

what about 1.7?

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

a=asa for 1.7 checkin too, if applicable.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

needs re-approval now that we're past 1.0 RC. setting back to request.

Comment 19

[Asa Dotzler [:asa]]

Comment on attachment 155878 [details] [diff] [review]
patch v2.0

no longer branch material