Last Comment Bug 821969 - Un-prefix gradients from per-theme browser.css
: Un-prefix gradients from per-theme browser.css
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 20
Assigned To: Soumya Deb [:Debloper]
:
Mentors:
Depends on:
Blocks: 818659
  Show dependency treegraph
 
Reported: 2012-12-15 00:31 PST by Soumya Deb [:Debloper]
Modified: 2013-01-05 16:04 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP Patch (67.14 KB, patch)
2012-12-16 09:29 PST, Soumya Deb [:Debloper]
no flags Details | Diff | Review
Patch for all themes (66.70 KB, patch)
2012-12-17 09:26 PST, Soumya Deb [:Debloper]
dao+bmo: review-
Details | Diff | Review
Patch for all themes (68.27 KB, patch)
2012-12-30 05:17 PST, Soumya Deb [:Debloper]
no flags Details | Diff | Review
Patch for all themes (68.97 KB, patch)
2012-12-31 09:13 PST, Soumya Deb [:Debloper]
dao+bmo: review+
Details | Diff | Review

Description Soumya Deb [:Debloper] 2012-12-15 00:31:38 PST
Remove stray traces of -moz in browser.css from WinStripe|PinStripe|GnomeStripe themes.
Comment 1 Soumya Deb [:Debloper] 2012-12-16 09:29:33 PST
Created attachment 692745 [details] [diff] [review]
WIP Patch

-moz-linear-gradient(bottom left, <colors>) !== linear-gradient(to top right, <colors>)

Flagged the occurrences with comments - not sure how to deal with it.

http://dev.w3.org/csswg/css3-images/#linear-gradients shows the change in spec, how the standard implementation handles the gradient line differently with keyword-angle notation.

Suggestion?
Comment 2 Masatoshi Kimura [:emk] 2012-12-16 17:26:43 PST
Comment on attachment 692745 [details] [diff] [review]
WIP Patch

>-  background-image: -moz-repeating-linear-gradient(-45deg,
>+  background-image: repeating-linear-gradient(-45deg,

This is wrong. <unprefixed-degree> = 90deg - <prefixed-degree>. (135deg in this case.) Same for all occurrence of -45deg.

>@@ -2323,17 +2323,17 @@ html|*#gcli-output-frame {
>-  background-image: -moz-linear-gradient(top, #B4211B, #8A1915);
>+  background-image: linear-gradient(to top, #8A1915, #B4211B);

Why don't you just writing
  background-image: linear-gradient(#B4211B, #8A1915);
?

>@@ -3780,17 +3780,17 @@ html|*#gcli-output-frame {
>-  background-image: -moz-linear-gradient(top, #B4211B, #8A1915);
>+  background-image: linear-gradient(to top, #8A1915, #B4211B);

Same here.
Comment 3 Masatoshi Kimura [:emk] 2012-12-16 17:27:28 PST
(In reply to Soumya Deb [:Debloper] from comment #1)
> -moz-linear-gradient(bottom left, <colors>) !== linear-gradient(to top
> right, <colors>)
> 
> Flagged the occurrences with comments - not sure how to deal with it.
> 
> http://dev.w3.org/csswg/css3-images/#linear-gradients shows the change in
> spec, how the standard implementation handles the gradient line differently
> with keyword-angle notation.
> 
> Suggestion?

Did you *really* want the old behavior?
Comment 4 Soumya Deb [:Debloper] 2012-12-17 04:25:31 PST
(In reply to Masatoshi Kimura [:emk] from comment #2)
> Comment on attachment 692745 [details] [diff] [review]
> <unprefixed-degree> = 90deg - <prefixed-degree>.

Right, will fix.


> >+  background-image: linear-gradient(to top, #8A1915, #B4211B);
> 
> Why don't you just writing
>   background-image: linear-gradient(#B4211B, #8A1915);

Makes sense, will fix (actually started doing, then wasn't sure if I should, so reverted).


(In reply to Masatoshi Kimura [:emk] from comment #3)
> (In reply to Soumya Deb [:Debloper] from comment #1)
> Did you *really* want the old behavior?

I don't wanna change the behavior of the code, just standardize them with proper syntax. Altering gradient behaviour etc. might require extra UI-review and isn't the primary purpose of the bug. So preferably, yes.
Comment 5 Masatoshi Kimura [:emk] 2012-12-17 04:51:12 PST
(In reply to Soumya Deb [:Debloper] from comment #4)
> > Did you *really* want the old behavior?
> 
> I don't wanna change the behavior of the code, just standardize them with
> proper syntax. Altering gradient behaviour etc. might require extra
> UI-review and isn't the primary purpose of the bug. So preferably, yes.

New syntax does not provide a way to make an exact copy of old behavior. Perhaps you will have to file a followup bug for the behavior change.
Comment 6 Soumya Deb [:Debloper] 2012-12-17 09:26:09 PST
Created attachment 692985 [details] [diff] [review]
Patch for all themes
Comment 7 Soumya Deb [:Debloper] 2012-12-17 09:42:59 PST
(In reply to Masatoshi Kimura [:emk] from comment #5)
> (In reply to Soumya Deb [:Debloper] from comment #4)
> > > Did you *really* want the old behavior?
> > 
> > ...So preferably, yes.
> 
> Perhaps you will have to file a followup bug for the behavior change.

Done: #822322

Intentionally kept "to left", "to right" as it is, cause changing to angular notation makes it less legible. All vertical gradients are formatted to be "to bottom" & the direction keyword removed (for being default).
Comment 8 Dão Gottwald [:dao] 2012-12-28 06:17:18 PST
Comment on attachment 692985 [details] [diff] [review]
Patch for all themes

> #urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity {
>   background-color: #fff;
>   color: hsl(92,100%,30%);
>   -moz-margin-end: 4px;
>-  background-image: -moz-linear-gradient(hsla(92,81%,16%,0),
>+  background-image: linear-gradient(hsla(92,81%,16%,0),
>                                          hsla(92,81%,16%,.2) 25%,
>                                          hsla(92,81%,16%,.2) 75%,
>                                          hsla(92,81%,16%,0));

adjust indentation

> #TabsToolbar[tabsontop=false] {
>   background-image:
>-    -moz-linear-gradient(bottom, rgba(0,0,0,.3) 1px, rgba(0,0,0,.05) 1px, transparent 50%);
>+    linear-gradient(transparent 50%, rgba(0,0,0,.05) 1px, rgba(0,0,0,.3) 1px);

This results in a different gradient. (You can't go from 50% to 1px, since 50% is larger than 1px here.)
Comment 9 Soumya Deb [:Debloper] 2012-12-30 05:17:37 PST
Created attachment 696619 [details] [diff] [review]
Patch for all themes

Reverted all the vertical gradients using multi-unit, or weighted-sized values to as was in https://bugzilla.mozilla.org/attachment.cgi?id=692745&action=diff

Indentations fixed on multi-lined gradient declarations. Hadn't done so before to minimize number of lines changed.
Comment 10 Soumya Deb [:Debloper] 2012-12-30 05:22:44 PST
Also, not sure if I've done right with: https://bugzilla.mozilla.org/attachment.cgi?id=696619&action=diff#a/browser/themes/winstripe/browser.css_sec10 line #825

Making it `linear-gradient(to top, transparent, rgba(0,0,0,.15))` would surely look more homogeneous with the other lines. Please tell if I should change to that instead.
Comment 11 Dão Gottwald [:dao] 2012-12-30 06:15:26 PST
(In reply to Soumya Deb [:Debloper] from comment #10)
> Also, not sure if I've done right with:
> https://bugzilla.mozilla.org/attachment.cgi?id=696619&action=diff#a/browser/
> themes/winstripe/browser.css_sec10 line #825
> 
> Making it `linear-gradient(to top, transparent, rgba(0,0,0,.15))` would
> surely look more homogeneous with the other lines. Please tell if I should
> change to that instead.

Yep.
Comment 12 Dão Gottwald [:dao] 2012-12-30 06:17:31 PST
Comment on attachment 696619 [details] [diff] [review]
Patch for all themes

> #addon-bar[customizing] > #status-bar {
>   opacity: .5;
>-  background-image: -moz-repeating-linear-gradient(-45deg,
>-                                                   rgba(255,255,255,.3), rgba(255,255,255,.3) 5px,
>-                                                   rgba(0,0,0,.3) 5px, rgba(0,0,0,.3) 10px);
>+  background-image: repeating-linear-gradient(-45deg,
>+                                              rgba(255,255,255,.3), rgba(255,255,255,.3) 5px,
>+                                              rgba(0,0,0,.3) 5px, rgba(0,0,0,.3) 10px);
> }

Don't you need to update the angle here?
Comment 13 Soumya Deb [:Debloper] 2012-12-31 09:13:23 PST
Created attachment 696750 [details] [diff] [review]
Patch for all themes

(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 696619 [details] [diff] [review]
> ...
> >+  background-image: repeating-linear-gradient(-45deg,
> ...
> Don't you need to update the angle here?

*Facepalm* and at loss of excuses!
Comment 15 Phil Ringnalda (:philor) 2013-01-05 16:04:23 PST
https://hg.mozilla.org/mozilla-central/rev/04557efa1fd9

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