Closed Bug 483537 Opened 11 years ago Closed 11 years ago

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

Categories

(Toolkit :: Themes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file, 2 obsolete files)

Firefox 3.0 apparently has six uses, so you're 50% better already :-)
Flags: blocking1.9.1?
Attached patch Proposed patch (obsolete) — Splinter Review
.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-
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?
(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]...
(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.
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.
(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.
(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.
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.
(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.
Attached patch Updated for review comments (obsolete) — Splinter Review
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.
(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.
(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" ;-)
Attached patch Updated againSplinter Review
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+
Pushed changeset 62d18b867afb to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.