433107 - Drop down arrow for "To/CC/BCC/.." too close to the edge, widget doesn't show focus

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot

Occurred on 10.5 Leopard while testing Shredder Alpha 1 build 1, with build ID
version 3.0a1 (2008050714).

The arrow at the drop down menu of selecting "To/CC/BCC/etc." is too close to the left edge and makes it difficult to ascertain it's a drop down arrow from afar at first glance. See screenshot.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: XP SP3 screenshot

Windows XP SP3 is not affected. (Latest Thunderbird trunk 3.0a2pre build)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: proposed screenshot

This screenshot shows a proposed 5px gap between the arrow and the left edge.

Patch is upcoming.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: patch that fixes the issue.

This bug adds 5px between the left border and the arrow.

Comment 4

[[e-mail]]

May I suggest that you refrain from creating app-specific workarounds for core theme bugs in favour of getting the core bug fixed? Thanks.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Seamonkey latest 2.0a1pre nightly screenshot

Seamonkey on Mac is not affected. Build identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008060301 SeaMonkey/2.0a1pre

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Neil, SeaMonkey is not affected, nor Thunderbird on platforms other than Mac (I haven't tested Linux though). Is there a better way I should do this?

Comment 7

[[e-mail]]

Maybe you need to fix the styles that you were using in older versions?

http://mxr.mozilla.org/seamonkey/source/mail/themes/pinstripe/mail/compose/messengercompose.css#437

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

You mean add the following line in .aw-menulist instead of .addressingWidgetCell?

  padding: 0px 0px 0px 5px;

(btw, I just tested adding 5px of padding-left in .aw-menulist, removing the ones added in .addressingWidgetCell, using DOMi and it seems to have the same end-result.)

Comment 9

[[e-mail]]

(In reply to comment #8)
> You mean add the following line in .aw-menulist instead of .addressingWidgetCell?
Well, I don't actually know how you want the button to look, but iirc SeaMonkey "is not affected" because it doesn't have that style block at all.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

re: comment #4:

<NeilAway> currently you're not using the default toolkit look; is that intentional?
<nth10sd> you mean in my patch, or as TB has been all along?
<nth10sd> (i'm not sure how TB does it all along, haven't been deep in the codebase for long)
• NeilAway doesn't either
<NeilAway> before fixing something, it helps to know what's wrong

Thus, I'm not sure if my proposed patch fixes it the *right* way. Leaving it as that for now.

Comment 11

[Kai Liu]

Comment on attachment 323467 [details] [diff] [review]
patch that fixes the issue.

>-  padding: 0px;
>+  padding: 0px 0px 0px 5px;

This may cause problems in RTL; you might want to consider using start instead of left.  You can test patches for RTL compatibility using Force RTL: <http://developer.mozilla.org/en/docs/Making_Sure_Your_Theme_Works_with_RTL_Locales>

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 323467 [details] [diff] [review]
patch that fixes the issue.

Cancelling review request until we know why it's needed, best to fix the cause.

Comment 13

[Magnus Melin [:mkmelin]]

The arrow is close to the edge on linux too (though not that close).
How did mac look on 2.0? On linux the arrow is very different so I can't say if it's really a regression or not.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: screenshot of Mac TB 2.0.0.14

Comment 15

[Phil Ringnalda (:philor)]

Okay. Filed bug 437926 on having widget: cocoa actually allow drawing a menulist-compact. Filed bug 437928 on switching to using the widget's -moz-appearance when it actually has its own appearance, not that of a menulist.

Gary: can we please have a new patch, just adding 5px of -moz-padding-start to .aw-menulist (where we'll remember to remove it when we don't need it), and cleaning up the totally random indenting there?

Comment 16

[Phil Ringnalda (:philor)]

Gah. Hold off on that for another day, despite all the time I spent staring at the nine different files that do addressing widgets, I think I totally missed the obvious reason it doesn't work.

Comment 18

[Phil Ringnalda (:philor)]

Third time I've typed this, thanks to VerifiedDownloadPlugin.plugin, but at least it's getting shorter.

There are roughly three different menulist-compacts:

SM Classic on Windows and Linux, and Tb on Windows and Linux have one with a binding in toolkit/themes/(win|gnome)stripe/global/globalBindings.xml, which is a <hbox><image><label></hbox> (where the image is unused), plus CSS in toolkit/themes/(win|gnome)stripe/global/menulist.css which unsets the -moz-appearance and builds up the appearance of a (flat, Windows Classic) button that's able to show focus, with a list-style-image on the whole widget for a dropmarker.

SM Modern on all platforms uses a binding in toolkit/content/widgets/menulist.xml which is <dropmarker><image><label> (again with the unused image), plus CSS in suite/themes/modern/global/menulist.css which does roughly the same things about button borders, but since it doesn't have to mess with native theming and gets to display the dropmarker, puts the list-style-image there instead of on the widget.

SM Classic on Mac, and Tb on Mac use the same binding from content as Modern, but since dropmarker is display: none in Pinstripe, it's practically speaking just a bare <label>. There's some CSS in toolkit/themes/pinstripe/global/menulist.css which pretends that it's doing the button appearance thing, but it's pretty pointless: it doesn't unset -moz-appearance, so it's still a native-themed menulist. And since it doesn't bother pretending to set a dropmarker image, if someone actually did see that CSS by setting -moz-appearance: none in userChrome.css, it ought to look like a plain flat button with no arrow, which still magically pops up a menu. Tb on Mac overrides that with some oddly-indented CSS in mail/themes/pinstripe/mail/compose/messengercompose.css (not, oddly enough, the unused and unjarred CSS in mail/themes/pinstripe/mail/addressingWidget.css) which unsets the -moz-appearance and draws a very very flat button with a dropmarker on the left, though without any ability to show focus.

So, for probably all its life, certainly 6 years that I know of, SM Classic/Mac has had a (big, glaring, distracting, excessive) native menulist for menulist-compact, apparently without any complaints or desire to change it. For probably all it's life, certainly four years that I know of, Tb/Mac has had a subtle (but subtly broken) menulist-compact which approximates the ones used on every single other platform/theme except Classic/Mac, with no desire to change it beyond wanting to have it show focus. To me, that sounds like "doing the right thing" is one of two things:

1. Decide that a menulist-compact really truly looks like [v label] on every platform, and that Classic/Mac's native menulist is a theme-specific decision, and give toolkit/themes/pinstripe a binding like the other stripes, and CSS like the other stripes, and then give suite/themes/classic a single rule to turn menulist-compact back into -moz-appearance: menulist. There's no really good place in Classic to stick it, and it would be hard to find out why it looks like a menulist, but it would make everything else much more sensible. In fact, since the binding doesn't much matter to the native menulist, that approach could instead make the (win|gnome)stripe skin binding the default content binding, and move Modern's binding into suite/modern instead, simplifying things even more.

2. Decide that a menulist-compact really truly looks like a native-themed menulist on Mac, and that it's an app-theme decision on Tb's part to have it look like a button instead, and remove the pointless bits of the CSS in toolkit, and give Thunderbird a better binding and some (properly indented) CSS to make it look right and properly show focus.

With my Toolkit hat on, I think number 1 is the right thing to do (probably with borders and a button color in toolkit, and some Tb theming to turn it back into our nice subtle borderless white), but with my Have I Really Spent Three Days On This hat on, the twenty minutes it would take to copy-paste number 2 into existence seems tempting. Somebody want to decide?

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Deassigning myself as I don't forsee myself having the time to work on this bug in the next few weeks / months.

Comment 20

[Phil Ringnalda (:philor)]

Attachment: Cleanup and focus v.1

Well, while solution 1 from comment 18 is the right thing to do, it's currently a pain in the ass, between the hg/CVS split and nobody knowing what branch any product is going to ship from, and nobody being willing to help me figure where to put SeaMonkey's CSS. At this point, I think the off-and-on two weeks I've spent thinking about this is long enough, and it's time to just land something.

For just fixing Tb, we would actually only need a different binding to use a focus border - a negative-offset outline we can put on the whole widget, so what we've got works fine.

Comment 21

[Phil Ringnalda (:philor)]

Attachment: Screenshot with focus

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 325239 [details] [diff] [review]
Cleanup and focus v.1

My css foo isn't too good, but this looks fine to me.

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 325239 [details] [diff] [review]
Cleanup and focus v.1

looks good

Comment 24

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/comm-central/index.cgi/rev/f3cc954f1c96

Comment 25

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

The focus ring doesn't have the glow as all the other textboxes have. Is this intended? It looks a bit weird when comparing all the focus rings.

Comment 26

[Phil Ringnalda (:philor)]

Once someone makes it possible to do glow from CSS, you can bet I'll be all over glowing it up. Meantime, at least it's grey and flat in Graphite, instead of blue and flat like the type="search" textboxes.