394063 - Consider continuing to force <select>s to Aqua

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Stuart Morgan]

Styled selects will now fall back to gfx when styled, which look really, really bad in a way that gfx buttons and text fields do not. We could do a forms.css override/fork where we !important a bunch of rules to keep selects unstyled and Aqua-looking.

Comment 1

[philippe (part-time)]

Adding this ruleblock would do the job:

select:not([size]):not([multiple]),
select[size="0"],
select[size="1"] {
  -moz-appearance: menulist !important;
  border-color: ThreeDFace !important;
  background-color: -moz-Field !important;
  color: -moz-FieldText !important;
  font: -moz-list !important;
  border-width: 2px !important;
  border-style: inset !important;
}

This leaves listboxes ([size="5" or [multiple]) untouched.

and to my immense personal pleasure, that even works from within userContent.css :-)

Comment 2

[philippe (part-time)]

Attachment: test case

a somewhat ugly text case.

Comment 3

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

The selects on http://www.freep.com are a good example of barely-styled selects that end up looking horribly out-of-place with the gfx button.

Since this works from userContent, it sounds like we could ship another agent stylesheet (like we do for ad-blocking and flashblock) that we load all the time, which might be less fragile than forking forms.css.  OTOH, adding another sheet might increase pageload time.

Comment 4

[philippe (part-time)]

I don't think the extra sheet, by itself, would affect page load time; after all the browser doesn't need to fetch it. But it might affect painting time on pages with complex forms.
And I agree, not forking forms.css is the safer option.

My code sample above isn't complete, BTW. It misses a ruleblock for [disabled]  widgets.
And it should not contain the rule for 'font'. That prevents the user from resizing the text-label (via minimum font-size or text-resize).

Here is what I have in my usercontent.css:

select:not([size]):not([multiple]),
select[size="0"],
select[size="1"] {
  -moz-appearance: menulist !important;
  border-color: ThreeDFace !important;
  background: -moz-Field !important;
  color: -moz-FieldText !important;
  border-width: 2px !important;
  border-style: inset !important;
}
select:not([size]):not([multiple])[disabled],
select[size="0"][disabled],
select[size="1"][disabled] {
  -moz-appearance: menulist !important;
  background: ThreeDFace !important;
  color: GrayText !important;
}

I haven't seen any issues so far.

Comment 5

[Stuart Morgan]

Will a css override still work in light of bug 240117?

Comment 6

[philippe (part-time)]

Yes, it still works.
My 3 hours old home made build has all the patches from bug 240117 (I did pay attention during checkout). With my rules above, select widgets are _not_ sensitive to author styling.
Just verified with the latest Tinderbox build: 2007100917 (2.0a1pre).

Comment 7

[philippe (part-time)]

Bz has since somehow confirmed that this should work (see bug 240117 comment 18 and 19).

On the other hand, there may be a safer method by using the CSS3 'initial' keyword (fully implemented in Gecko 1.9 with the -moz prefix). Works equally well with the select widget in my testing.

select:not([size]):not([multiple]),
select[size="0"],
select[size="1"] {
  -moz-appearance: menulist !important;
  border-color: -moz-initial !important;
  background: -moz-initial !important;
  color: -moz-FieldText !important;
  border-width: -moz-initial !important;
  border-style: -moz-initial !important;
}
select:not([size]):not([multiple])[disabled],
select[size="0"][disabled],
select[size="1"][disabled] {
  -moz-appearance: menulist !important;
  background:-moz-initial !important;
  color:-moz-initial !important;
}

Comment 8

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

Another testcase are the selects at the bottom of http://trinity.neooffice.org/modules.php?name=Forums&file=viewtopic&t=849&start=15 which seem to have a 2px border that forces them into ugly-land (which is pretty standard in phpBB2-land.  

phpBB3 standard themes http://www.phpbb.com/community/viewforum.php?f=49 use 1px border and 1 px padding, which looks even worse in gfx, IMO.

Safari 3 renders the former as standard Aqua, even.

Comment 9

[Stuart Morgan]

I've been seeing a fair number of ghetto-looking selects, so we should definitely do this until widget has fancier support for selects.

Comment 10

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

The select on https://xmedia.ex.ac.uk/user/login/ is another powerful reason to do this (just focus it).

Comment 11

[Stuart Morgan]

Missed a1.

I'm now used to this enough that I no longer have a strong opinion either way.

Comment 12

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

We should be able to more-or-less copy the ad-blocking/Flashblock stylesheet loading code to do this, so it might be something that, say, Chris could do.

Doesn't need to block b2 unless we're adding some sort of *visible* pref (which I don't think we should do), but really, really want for 2.0.

Comment 13

[Chris Lawson (gone)]

Just to be clear: a "forceAqua.css" file that has the contents of comment 7, loaded via the usual stylesheet service, will solve this bug?

I can do the code if someone else can do the CSS.

Comment 14

[philippe (part-time)]

(In reply to comment #13) 
> I can do the code if someone else can do the CSS.
I'll post the stylesheet soonish.

Comment 15

[philippe (part-time)]

Attachment: the stylesheet part

I've added back the font part (when I originally tested this, that prevented minimum font-size from taking over, it now works as expected) and added text-indent (in case authors attempt to do some crazy 'image replacement method'.

The comment (* CSS rules preventing author....) can probably be written in better English. I'll leave that open to native speakers :-).

Comment 16

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

Chris said tonight he wouldn't be able to get to this until the New Year, so taking; I have this all working except 

1) watching for about:config changes of a hidden pref (for those who want to honor author styles at all times), though I'm not sure this needs to be live for that use-case, and 

2) making sure none of the code I copied is extraneous (haha!)

I feared that the stylesheet loading code would be all over the app and would be non-copyable, but it's pretty self-contained (without the pref UI that ad-blocking and Flashblock use) and seemed even to me like copy/paste coding could work.

Comment 17

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

The one thing that looks worse in my limited testing[1] is the testcase for bug 322218, where we lose what little remnant of a <select> we had left (i.e., the beveled border vanishes).  I think that's a very rare case, and I wouldn't block the massive improvement we get far and wide across the web on a case where the one site on which it was found no longer exists.

[1] Sites mentioned in this bug, as well as sites/testcases in Widget:Cocoa bugs mentioning <select>s

Comment 18

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

Attachment: patch, v1.4

Here's a patch that accomplishes this and prevents me from stabbing out my eyes so much.  Big thanks to hendy for helping me debug the hidden pref part!

I decided that making the hidden pref change live would be very useful for QA purposes, and it resulted in me learning a little tiny bit more about Obj-C. ;)  I also tweaked philippe's CSS file comments, as requested.

Comment 19

[Christopher Henderson]

Comment on attachment 354028 [details] [diff] [review]
patch, v1.4

Looks good. The only necessary change I can see is
+  // the Gecko pref observer component isn't set up then.  |-xpcomTerminate:|
+  // and |-dealloc:| appear to automatically clean these up.
to
+  // the Gecko pref observer component isn't set up then.  The pref change
+  // observers are automatically cleaned up in |-xpcomTerminate:|.

Comment 20

[philippe (part-time)]

(In reply to comment #17)
> The one thing that looks worse in my limited testing[1] is the testcase for bug
> 322218, where we lose what little remnant of a <select> we had left (i.e., the
> beveled border vanishes). 

um, yeah... If you want to protect against that MS-theme thingie (a Camino user in corporate environment ?), then using the values as specified in forms.css is probably the way to go. Note that you'll still lose the drop-marker button (that doesn't happen in Minefield, but equally fails in FX 3.0.x - probably got fixed with bug 420363)

select:not([size]):not([multiple]),
select[size="0"],
select[size="1"] {
	-moz-appearance: menulist !important;
	border-color: ThreeDFace !important;
	background: -moz-Field !important;
	color: -moz-FieldText !important;
	padding: -moz-initial !important;
	font: -moz-list !important;
	border-width: 2px !important;
	border-style: inset !important;
	text-indent: 0 !important;
	}
select:not([size]):not([multiple])[disabled],
select[size="0"][disabled],
select[size="1"][disabled] {
	-moz-appearance: menulist !important;
	background: ThreeDFace !important;
 	color: GrayText !important;
	}

(In reply to comment #18)
> Created an attachment (id=354028) [details]

Thanks for making the pref 'live', that is one enhancement over using usercontent.css :-)

Comment 21

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

Comment on attachment 354028 [details] [diff] [review]
patch, v1.4

(In reply to comment #19)
> (From update of attachment 354028 [details] [diff] [review])
> Looks good. The only necessary change I can see is
> +  // the Gecko pref observer component isn't set up then.  |-xpcomTerminate:|
> +  // and |-dealloc:| appear to automatically clean these up.
> to
> +  // the Gecko pref observer component isn't set up then.  The pref change
> +  // observers are automatically cleaned up in |-xpcomTerminate:|.

Christopher and I chatted, and I reminded him we've got both the PrefChange observer and the NSNotificationCenter observer, the latter of which is cleaned up in |-dealloc:|.  I'll make the comment more clear before checkin.

Stuart, what do you think about the CSS; should we go ahead and make the testcase for bug 322218 look slightly more like something clickable?

Comment 22

[Stuart Morgan]

Comment on attachment 354028 [details] [diff] [review]
patch, v1.4

>+select:not([size]):not([multiple])[disabled],
>+select[size="0"][disabled],
>+select[size="1"][disabled] {
>+  -moz-appearance: menulist !important;
>+  background: -moz-initial !important;
>+  color: GrayText !important;
>+}

The first two rules here are repeats. Is that just because there is the possibility of a higher-specificity rule using [disabled]? If so, let's add a comment to that effect; if not, they should be removed.

Otherwise, looks good with the other comment change; sr=smorgan. Thanks!

Comment 23

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

I think that's the case, yes, but I defer to my CSS guru ;)

Comment 24

[philippe (part-time)]

(In reply to comment #22)
... 
> The first two rules here are repeats. Is that just because there is the
> possibility of a higher-specificity rule using [disabled]? If so, let's add a
> comment to that effect; if not, they should be removed.

Those can be removed.

Comment 25

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

Checked in on cvs trunk with the extra CSS removed and the comment clarified; you may now stop poking your eyes out!

Thanks philippe and hendy for the help here!

Comment 26

[Stuart Morgan]

(In reply to comment #21)
> Stuart, what do you think about the CSS; should we go ahead and make the
> testcase for bug 322218 look slightly more like something clickable?

Sorry, I meant to reply to this and forgot. Let's file a new bug if we want to look at that, so we can look at before/after and discuss whether it's worth worrying about.