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

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Firefox 3.0 apparently has six uses, so you're 50% better already :-)
Flags: blocking1.9.1?
(Assignee)

Comment 1

10 years ago
Created attachment 367525 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #367525 - Flags: review?(gavin.sharp)
Don't think this blocks, so minusing, but Neil, if you have rationale, please renominate.
Flags: blocking1.9.1? → blocking1.9.1-
(Assignee)

Updated

9 years ago
Attachment #367525 - Flags: review?(gavin.sharp) → review?(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?
(Assignee)

Comment 4

9 years ago
(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.
Note that the cited snippet was about radio[pane]...
(Assignee)

Comment 6

9 years ago
(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
That looks like it needs to be moved from browser to toolkit for pinstripe.
(Assignee)

Comment 8

9 years ago
Sure, but that's a separate bug which I'm not immediately interested in ;-)
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.
(Assignee)

Comment 10

9 years ago
(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.
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.
(Assignee)

Comment 12

9 years ago
(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...
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.

Updated

9 years ago
Depends on: 504300
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.
(Assignee)

Comment 15

9 years ago
(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?
Seems like that would make sense.
(Assignee)

Comment 17

9 years ago
Created attachment 389380 [details] [diff] [review]
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).
Attachment #367525 - Attachment is obsolete: true
Attachment #389380 - Flags: review?(dao)
Attachment #367525 - Flags: review?(dao)
Comment on attachment 389380 [details] [diff] [review]
Updated for review comments

Looks good to me. Markus, would you mind testing this on Mac?
Attachment #389380 - Flags: review?(mstange)
Attachment #389380 - Flags: review?(dao)
Attachment #389380 - Flags: review+
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.
(Assignee)

Comment 20

9 years ago
(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.
(Assignee)

Comment 21

9 years ago
(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" ;-)
(Assignee)

Comment 22

9 years ago
Created attachment 389675 [details] [diff] [review]
Updated again

OK, this version should be good to go for now.
Attachment #389380 - Attachment is obsolete: true
Attachment #389675 - Flags: review?(mstange)
Attachment #389380 - Flags: review?(mstange)
Attachment #389675 - Flags: review?(mstange) → review+
(Assignee)

Comment 23

9 years ago
Pushed changeset 62d18b867afb to mozilla-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 510916
You need to log in before you can comment on or make changes to this bug.