469631 - Cocoa needs a correct combobox/editable menulist widget

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: NSComboBox looks like this

Currently when people try to use a combobox/editable menulist on Mac OS X, they end up with a monstrosity that is a pop-up button/menupop with part of the closed state being editable and with the up/down arrow endcap widget, rather than a Cocoa NSComboBox.

This used not to be a very commonly used widget (I'd only ever seen it in one minor extension), but recently it's becoming much more prominent in Mozilla apps, causing people like Sam to poke their eyes out:

1) attachment 352818 [details] on bug 376668 (which is Firefox putting it in web content)
2) http://www.flickr.com/photos/davidascher/3094292059/ (Thunderbird chrome, calendar tab)

By comparison, a Cocoa NSComboBox looks like the attachment here.

(Note also that the NSComboBox uses the below-the-widget menu currently erroneously seen on <select>s/menupops in Gecko (bug 402625); NSPopUpButtons use a menu positioned on top of (z) the button.  Ideally instead of having two franken-hybrids, Widget:Cocoa would support proper implementations of both pop-up buttons and comboboxen.)

Comment 1

[Markus Stange [:mstange]]

Testcase: attachment 318039 [details] (from bug 431078)

Comment 2

[Markus Stange [:mstange]]

Attachment: fix v1

Comment 3

[Markus Stange [:mstange]]

Attachment: screenshot with patch

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 360087 [details] [diff] [review]
fix v1

>diff --git a/toolkit/themes/pinstripe/global/menu.css b/toolkit/themes/pinstripe/global/menu.css

>+/* ::::: menuitems in editable menulist popups ::::: */
>+
>+menulist[editable="true"] > menupopup > menuitem {
>+  -moz-appearance: none !important;
>+}

>+menulist[editable="true"] > menupopup > menuitem[_moz-menuactive="true"] {
>+  background-color: Highlight;
>+}

>diff --git a/toolkit/themes/pinstripe/global/popup.css b/toolkit/themes/pinstripe/global/popup.css

>+menulist[editable="true"] > menupopup {
>+  -moz-appearance: none;
>+  background-color: white;
>+}
>+
>+menulist[editable="true"] > menupopup > .popup-internal-box {
>+  padding: 0 !important;
>+}

Why should the fact that you can edit the value affect the look of the menupopup this way?

>diff --git a/toolkit/themes/pinstripe/global/menulist.css b/toolkit/themes/pinstripe/global/menulist.css

> .menulist-editable-box {
>+  padding: 2px 0 2px 4px;

Is this correct for rtl?

Comment 5

[Josh Aas]

Comment on attachment 360087 [details] [diff] [review]
fix v1

+- (void) drawWithFrame:(NSRect)rect inView:(NSView*)controlView

+static void DrawFocusRing(NSRect rect, float radius)

Does this stuff need obj-c exception guards?

Comment 6

[Markus Stange [:mstange]]

(In reply to comment #4)
> Why should the fact that you can edit the value affect the look of the
> menupopup this way?

Ask the UI designers at Apple ;)
That's just how native NSComboBoxes look like. Their dropdown lists don't have rounded corners and their menuitems' hover style is just a plain color.

Should I ask somebody who has a Mac for CSS review?

> Is this correct for rtl?

I think so. Mac OS X doesn't really care about RTL; there's no way to draw menulist buttons differently for RTL, so the dropdown marker is always on the right. And that's were no padding is necessary.


(In reply to comment #5)
> (From update of attachment 360087 [details] [diff] [review])
> +- (void) drawWithFrame:(NSRect)rect inView:(NSView*)controlView
> 
> +static void DrawFocusRing(NSRect rect, float radius)
> 
> Does this stuff need obj-c exception guards?

I don't know - does it?
Whenever these functions are called, nsNativeThemeCocoa::DrawDropdown or ::DrawSearchField are on the stack, and those already have guards.
In other words, those two new functions aren't directly called by Gecko, so guards shouldn't be necessary.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> (In reply to comment #4)
> > Why should the fact that you can edit the value affect the look of the
> > menupopup this way?
> 
> Ask the UI designers at Apple ;)
> That's just how native NSComboBoxes look like. Their dropdown lists don't have
> rounded corners and their menuitems' hover style is just a plain color.

Well, I'll leave that to you, but it looks like a quirk to me rather than by design, in which case I'd say it's not worth the code.

> > Is this correct for rtl?
> 
> I think so. Mac OS X doesn't really care about RTL; there's no way to draw
> menulist buttons differently for RTL, so the dropdown marker is always on the
> right. And that's were no padding is necessary.

Then please add a comment, saying that the dropdown is always on the right.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 360087 [details] [diff] [review]
fix v1

>+menulist[editable="true"] > menupopup {
>+  -moz-appearance: none;
>+  background-color: white;
>+}

This should probably be "Menu" instead of "white".

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>> I think so. Mac OS X doesn't really care about RTL; there's no way to draw
>> menulist buttons differently for RTL, so the dropdown marker is always on the
>> right. And that's were no padding is necessary.
>
>Then please add a comment, saying that the dropdown is always on the right.

Perhaps we should have better rtl support than OS X?  I'm not sure how we normally deal with consistency vs. doing the right thing with rtl, but it seems like this might be a case where we want to go disregard the platform.

In terms of appearance I think we want to look as much like OS X as possible.

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> In terms of appearance I think we want to look as much like OS X as possible.

Unless it's undocumented (I don't know if it is) and we consider the special look of that menupopup a bug on their side.
Are there other menupopups that look like that, e.g. autocomplete popups?

Comment 11

[Markus Stange [:mstange]]

Attachment: v2

(In reply to comment #7)
> Well, I'll leave that to you, but it looks like a quirk to me rather than by
> design, in which case I'd say it's not worth the code.

I think it's by design. And you hunch is correct; autocomplete popups look like that, too.

> Then please add a comment, saying that the dropdown is always on the right.

I had a different idea: I can just set the right numbers in GetWidgetBorder(), so there's no need to specify any paddings in the CSS.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 360889 [details] [diff] [review]
v2

>+menulist[editable="true"] > menupopup > menuitem {
>+  -moz-appearance: none !important;

Can you please remove the original !important flag that's forcing you to add another one? It's in menu.css, I think.

>+menulist[editable="true"] > menupopup > .popup-internal-box {
>+  padding: 0 !important;

here too (.popup-internal-box in popup.css)

>+menulist[editable="true"] > menupopup > menuitem[_moz-menuactive="true"] {
>+  background-color: Highlight;
>+}

Judging from you screenshot, this works, but I wonder why background-color is needed (is it?) while color isn't...

Comment 13

[Markus Stange [:mstange]]

Attachment: v3

(In reply to comment #12)
> (From update of attachment 360889 [details] [diff] [review])
> >+menulist[editable="true"] > menupopup > menuitem {
> >+  -moz-appearance: none !important;
> 
> Can you please remove the original !important flag that's forcing you to add
> another one? It's in menu.css, I think.

Done.

> >+menulist[editable="true"] > menupopup > .popup-internal-box {
> >+  padding: 0 !important;
> 
> here too (.popup-internal-box in popup.css)

Done.

> >+menulist[editable="true"] > menupopup > menuitem[_moz-menuactive="true"] {
> >+  background-color: Highlight;
> >+}
> 
> Judging from you screenshot, this works, but I wonder why background-color is
> needed (is it?) while color isn't...

Color is already set here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/menu.css?mark=161-166#150

Setting the background color is necessary because I'm turning off -moz-appearance. For normal menulist popup menuitems, -moz-appearance draws a blue gradient in the background when _moz-menuactive is set (see left screenshot).

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #13)
> Color is already set here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/pinstripe/global/menu.css?mark=161-166#150
[...]
> For normal menulist popup menuitems, -moz-appearance draws a
> blue gradient in the background when _moz-menuactive is set

We should just set the background color there as a means to make removing -moz-appearance straightforward. The same applies to the menupopup background color, I think.

Comment 15

[Markus Stange [:mstange]]

Attachment: v4

Sure.

Comment 16

[Dão Gottwald [:dao]]

(In reply to comment #11)
> I had a different idea: I can just set the right numbers in GetWidgetBorder(),
> so there's no need to specify any paddings in the CSS.

Btw, can we do the same for the search textbox?

Comment 17

[Stefan [:stefanh]]

Comment on attachment 360922 [details] [diff] [review]
v4

menupopup > menu[_moz-menuactive="true"],
 menupopup > menuitem[_moz-menuactive="true"],
 popup > menu[_moz-menuactive="true"],
 popup > menuitem[_moz-menuactive="true"] {
   color: -moz-mac-menutextselect;
+  background-color: Highlight;

I don't think you want Highlight since the color will change according to your system pref settings (try and change the selection color).

Comment 18

[Markus Stange [:mstange]]

Attachment: screenshot of a native NSComoboBox with red selection color

(In reply to comment #17)
> I don't think you want Highlight since the color will change according to your
> system pref settings (try and change the selection color).

In a native NSComboBox this color depends on the selection color, too.

Comment 19

[Stefan [:stefanh]]

Oops, sorry - I misread the patch.

Comment 20

[Markus Stange [:mstange]]

(In reply to comment #16)
> (In reply to comment #11)
> > I had a different idea: I can just set the right numbers in GetWidgetBorder(),
> > so there's no need to specify any paddings in the CSS.
> 
> Btw, can we do the same for the search textbox?

Good idea. I've filed bug 477592.

Comment 21

[Markus Stange [:mstange]]

pushed: http://hg.mozilla.org/mozilla-central/rev/cf18a03b9ca8

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090304 Minefield/3.2a1pre ID:20090304021932

Comment 23

[Shawn Wilsher :sdwilsh]

Markus - we'd like to get this polish bug fixed on 1.9.1 branch.  Could you please generate a patch for branch that also contains the regression fix for bug 479904?

Comment 24

[Markus Stange [:mstange]]

Sure, I'll make one.

Comment 25

[Markus Stange [:mstange]]

Attachment: 1.9.1 branch patch

Comment 26

[Johnathan Nightingale [:johnath]]

This is marked for approval, but doesn't have tests for the code changes (CSS stuff I can believe not needing them).  Is there a reason for that?

Comment 27

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

Comment on attachment 368123 [details] [diff] [review]
1.9.1 branch patch

a191=beltzner

Over the objections of my usual more rational fellow drivers. We need tests on Cocoa widgets in general - is there a bug on that? We'll fight that fight the other day.

Comment 28

[Markus Stange [:mstange]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a699d6f44862


(In reply to comment #26)
> This is marked for approval, but doesn't have tests for the code changes (CSS
> stuff I can believe not needing them).  Is there a reason for that?

I'm not really sure what to test here, since it's just a change in visuals.
I could add two Mac-only reftests that test whether editable menulists look different from non-editable ones and whether -moz-appearance: menulist-textfield has any effect. That's not much, but probably better than nothing.

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #27)
> We need tests on Cocoa widgets in general - is there a bug on that?

Sam filed bug 470475 about hooking up a Cocoa unit testing framework. Other than that, there's just a series of bugs on adding tests for specific changes that landed without tests, made a mess of things, and so forth (see bug 419466 and bug 471946 for two stand-alone "needs tests" bugs I can find easily).

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848

Comment 31

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day.

While not in the primary UI, this widget is pretty common so the user is likely to encounter the issue pretty often.