483537 - -moz-appearance belongs in themes, not in xul.css

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[[e-mail]]

Firefox 3.0 apparently has six uses, so you're 50% better already :-)

Comment 1

[[e-mail]]

Attachment: Proposed patch

.autocomplete-richlistbox
* Moved into autocomplete.css where it belongs
radio[pane]
* Seems to be catered for by all toolkit themes already
richlistbox
* Moved into richlistbox.css where it belongs
Note: pinstripe has no changes; it seems not to use native themed listboxes.

Comment 2

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

Don't think this blocks, so minusing, but Neil, if you have rationale, please renominate.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 367525 [details] [diff] [review]
Proposed patch

> radio[pane] {
>   -moz-binding: url("chrome://global/content/bindings/preferences.xml#panebutton") !important; 
>   -moz-box-orient: vertical;
>-  -moz-appearance: none;
>   -moz-box-align: center;
> }

Does pinstripe have a replacement for this?

Comment 4

[[e-mail]]

(In reply to comment #3)
> Does pinstripe have a replacement for this?
I'm not sure what pinstripe is on; its listbox.css doesn't use -moz-appearance: listbox, but it does use -moz-appearance: listbox; four times in other files; these may have been blindly copied from winstripe.

Comment 5

[Dão Gottwald [:dao]]

Note that the cited snippet was about radio[pane]...

Comment 6

[[e-mail]]

(In reply to comment #5)
> Note that the cited snippet was about radio[pane]...
Sorry.
radio[pane] {
  -moz-appearance: none;
}
is duplicated in the following files:
toolkit/content/xul.css
toolkit/themes/gnomestripe/global/preferences.css
toolkit/themes/winstripe/global/preferences.css
browser/themes/pinstripe/browser/preferences/preferences.css

Comment 7

[Dão Gottwald [:dao]]

That looks like it needs to be moved from browser to toolkit for pinstripe.

Comment 8

[[e-mail]]

Sure, but that's a separate bug which I'm not immediately interested in ;-)

Comment 9

[Dão Gottwald [:dao]]

It seems to become necessary as soon as you remove -moz-appearance:none from xul.css. That's not just a matter of organizing that stuff nicely -- I suspect that the patch actually breaks stuff as it stands.

Comment 10

[[e-mail]]

(In reply to comment #9)
> It seems to become necessary as soon as you remove -moz-appearance:none from
> xul.css.
Oh, so for some reason the -moz-appearance: none; in pinstripe's preferences.css isn't taking effect? Since I don't have a Mac, perhaps you can look at the applied style rules in DOM Inspector to see what's conflicting.

Comment 11

[Dão Gottwald [:dao]]

I'm not sure that all documents with radio[pane] elements include browser/preferences/preferences.css, even if we ignore everything besides browser/ (which I think we shouldn't do).
What I said wasn't based on testing, and I also don't have a Mac.

Comment 12

[[e-mail]]

(In reply to comment #11)
> I'm not sure that all documents with radio[pane] elements include
> browser/preferences/preferences.css
Then they're not going to get any other radio[pane] styles...

Comment 13

[Dão Gottwald [:dao]]

Ok, since the pane attribute is preferences-specific, that won't be an issue for Firefox. That leaves us with potential breakage for other preferences.xml users.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 367525 [details] [diff] [review]
Proposed patch

>diff -r 388c17a510e8 toolkit/content/xul.css

> richlistbox {
>   -moz-binding: url('chrome://global/content/bindings/richlistbox.xml#richlistbox');
>   -moz-user-focus: normal;
>-  -moz-appearance: listbox;
>   -moz-box-orient: vertical;
> }

>diff -r 388c17a510e8 toolkit/themes/winstripe/global/richlistbox.css

> richlistbox {
>+  -moz-appearance: listbox;
>   margin: 2px 4px;
>   background-color: -moz-Field;
>   color: -moz-FieldText;

Since this was part of xul.css, it seems reasonable to me to add this to pinstripe's richlistbox.css as well (and then autocomplete.css accordingly). I'm not sure that it's necessary, but it would minimize the risk of breaking something.

Comment 15

[[e-mail]]

(In reply to comment #14)
> (From update of attachment 367525 [details] [diff] [review])
> > richlistbox {
> >+  -moz-appearance: listbox;
> >   margin: 2px 4px;
> >   background-color: -moz-Field;
> >   color: -moz-FieldText;
> Since this was part of xul.css, it seems reasonable to me to add this to
> pinstripe's richlistbox.css as well (and then autocomplete.css accordingly).
What about pinstripe's listbox.css? Should I swap border for -moz-appearance?

Comment 16

[Dão Gottwald [:dao]]

Seems like that would make sense.

Comment 17

[[e-mail]]

Attachment: Updated for review comments

It seems trees should use listbox appearance too.

I ordered the appearance lines to match the equivalent win/gnomestripe file(s).

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 389380 [details] [diff] [review]
Updated for review comments

Looks good to me. Markus, would you mind testing this on Mac?

Comment 19

[Markus Stange [:mstange]]

I've found three problems with this patch on Mac:
 - class="plain" on trees no longer turns off the border (bug 468860)
 - The border is too dark. I should fix -moz-appearance: listbox to use the
   right colors.
 - The richlistbox in the download manager no longer has a white background. I
   think we should keep the border and background styles for graceful fallback.

Comment 20

[[e-mail]]

(In reply to comment #19)
> I've found three problems with this patch on Mac:
>  - class="plain" on trees no longer turns off the border (bug 468860)
That's not Mac-specific; all our native themes have the same bug.

>  - The border is too dark. I should fix -moz-appearance: listbox to use the
>    right colors.
Beyond the scope of this bug? Or limit -moz-appearance: listbox to richlistbox?

>  - The richlistbox in the download manager no longer has a white background. I
>    think we should keep the border and background styles for graceful fallback.
Actually the previous background was -moz-Field and there was no border.

Comment 21

[[e-mail]]

(In reply to comment #19)
>  - class="plain" on trees no longer turns off the border (bug 468860)
...
>  - The richlistbox in the download manager no longer has a white background.
Aha, downloads.css reimplements class="plain" ;-)

Comment 22

[[e-mail]]

Attachment: Updated again

OK, this version should be good to go for now.

Comment 23

[[e-mail]]

Pushed changeset 62d18b867afb to mozilla-central.