Last Comment Bug 709578 - Replace :-moz-system-metric with @media blocks
: Replace :-moz-system-metric with @media blocks
Status: RESOLVED FIXED
[good first bug][mentor=dao][lang=css]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Firefox 12
Assigned To: Jason Yeo[:jyeo]
:
: Dão Gottwald [:dao]
Mentors:
Depends on: 511909
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-11 06:54 PST by Dão Gottwald [:dao]
Modified: 2011-12-28 11:09 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
709578.patch (2.87 KB, patch)
2011-12-18 23:58 PST, Jason Yeo[:jyeo]
no flags Details | Diff | Splinter Review
709578v2.patch (4.22 KB, patch)
2011-12-21 09:33 PST, Jason Yeo[:jyeo]
dao+bmo: review-
Details | Diff | Splinter Review
709578v3.patch (4.23 KB, text/plain)
2011-12-22 09:37 PST, Jason Yeo[:jyeo]
no flags Details
709578v4.patch (4.23 KB, patch)
2011-12-22 09:38 PST, Jason Yeo[:jyeo]
dao+bmo: review+
Details | Diff | Splinter Review
709578v5.patch (4.24 KB, patch)
2011-12-27 10:36 PST, Jason Yeo[:jyeo]
jasonyeo88: review+
Details | Diff | Splinter Review

Comment 1 Jason Yeo[:jyeo] 2011-12-18 23:58:58 PST
Created attachment 582757 [details] [diff] [review]
709578.patch

Hi,

I'm new to moz-central code base, so this is my first patch. I'm not sure if I'm doing the right thing here. Please let me know and I will make changes to it ASAP.

There are some other css files where the -moz-system-metric(windows-default-theme) also appears. Should I change that too? They appear in:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/xul/input.css#15
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/textbox-aero.css
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser.css#1554
Comment 2 Dão Gottwald [:dao] 2011-12-19 02:24:21 PST
(In reply to Jason Yeo(:jyeo) from comment #1)
> Created attachment 582757 [details] [diff] [review]
> 709578.patch
> 
> Hi,
> 
> I'm new to moz-central code base, so this is my first patch. I'm not sure if
> I'm doing the right thing here. Please let me know and I will make changes
> to it ASAP.
> 
> There are some other css files where the
> -moz-system-metric(windows-default-theme) also appears. Should I change that
> too? They appear in:
> http://mxr.mozilla.org/mozilla-central/source/layout/reftests/editor/xul/
> input.css#15

We don't ship this code, it's only for tests, so it's probably not worth it.

> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/
> global/textbox-aero.css
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/
> browser.css#1554

Yeah, these would be fine to be changed as well.
Comment 3 Dão Gottwald [:dao] 2011-12-19 02:25:15 PST
Comment on attachment 582757 [details] [diff] [review]
709578.patch

>--- a/browser/themes/winstripe/places/places-aero.css	Sun Dec 18 18:50:13 2011 -0800
>+++ b/browser/themes/winstripe/places/places-aero.css	Mon Dec 19 15:51:27 2011 +0800

>+@media -moz-windows-default-theme {
>+  #bookmarksPanel, #history-panel {

nit: please always break the line after the comma
Comment 4 Jason Yeo[:jyeo] 2011-12-21 09:33:13 PST
Created attachment 583530 [details] [diff] [review]
709578v2.patch

Hi,

I have added a newline after the comma for browser/themes/winstripe/places/places-aero.css.

Also, I have edited 
http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser.css#1554
and
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/textbox-aero.css

Let me know if I'm doing the right thing. Thanks!
Comment 5 Dão Gottwald [:dao] 2011-12-22 01:26:51 PST
Comment on attachment 583530 [details] [diff] [review]
709578v2.patch

I wish this worked, but apparently you need to write @media (-moz-windows-default-theme) instead of @media -moz-windows-default-theme :(
Comment 6 Jason Yeo[:jyeo] 2011-12-22 09:37:20 PST
Created attachment 583830 [details]
709578v3.patch
Comment 7 Jason Yeo[:jyeo] 2011-12-22 09:38:44 PST
Created attachment 583831 [details] [diff] [review]
709578v4.patch
Comment 8 Dão Gottwald [:dao] 2011-12-23 05:19:12 PST
Comment on attachment 583831 [details] [diff] [review]
709578v4.patch

>diff -r d75ebb37080e browser/themes/winstripe/browser-aero.css
>--- a/browser/themes/winstripe/browser-aero.css	Sun Dec 18 18:50:13 2011 -0800
>+++ b/browser/themes/winstripe/browser-aero.css	Thu Dec 22 01:30:41 2011 +0800

>-  #toolbar-menubar[autohide=true]:-moz-system-metric(windows-default-theme) {
>-    background-color: transparent;

>+    #toolbar-menubar[autohide=true] {
>+      background-color: transparent;
>+    }

You need to add !important here, as this selector now loses against others.

Other than that this patch is fine.
Comment 9 Jason Yeo[:jyeo] 2011-12-27 10:36:47 PST
Created attachment 584455 [details] [diff] [review]
709578v5.patch

okay I have made the changes. So should I flag this as checkin-needed?
Comment 10 Dão Gottwald [:dao] 2011-12-27 11:01:57 PST
Yep. Thanks!
Comment 12 Matt Brubeck (:mbrubeck) 2011-12-28 11:09:54 PST
Fixed for Firefox 12.  Thanks for the patch!
https://hg.mozilla.org/mozilla-central/rev/3e315088e243

Note You need to log in before you can comment on or make changes to this bug.