Last Comment Bug 730483 - Remove dropped prefixes from comm-central
: Remove dropped prefixes from comm-central
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Theme (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 14.0
Assigned To: Richard Marti (:Paenglab)
:
Mentors:
: 731999 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-24 15:55 PST by Richard Marti (:Paenglab)
Modified: 2012-03-27 01:08 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
remove the prefixes (8.57 KB, patch)
2012-02-24 16:01 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Review
remove the prefixes - full patch (27.06 KB, patch)
2012-02-25 00:44 PST, Richard Marti (:Paenglab)
no flags Details | Diff | Review
remove the prefixes (26.19 KB, patch)
2012-03-02 09:32 PST, Richard Marti (:Paenglab)
bwinton: review-
Details | Diff | Review
remove the prefixes (19.95 KB, patch)
2012-03-06 13:27 PST, Richard Marti (:Paenglab)
bwinton: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
Details | Diff | Review
Badge Comparison. Left 100% right 10000px (3.96 KB, image/png)
2012-03-20 14:53 PDT, Richard Marti (:Paenglab)
no flags Details

Description Richard Marti (:Paenglab) 2012-02-24 15:55:23 PST
Bug 693510 dropped the support of -moz-border-radius and -moz-box-shadow. Comm-central has still some of this properties.
Comment 1 Richard Marti (:Paenglab) 2012-02-24 16:01:32 PST
Created attachment 600559 [details] [diff] [review]
remove the prefixes

This patch removes the prefixes. It has still some parts with the prefixes: testpilot, venkman, inspector and irc. Also mozmill uses this prefixes.

I wasn't sure if I should also remove them so I leaved them. Should I expand the patch and fix also these parts?
Comment 2 Richard Marti (:Paenglab) 2012-02-24 16:02:51 PST
I have also a small part of calendar in this patch. I hope it's okay like this.
Comment 3 Richard Marti (:Paenglab) 2012-02-25 00:44:56 PST
Created attachment 600631 [details] [diff] [review]
remove the prefixes - full patch

This is a full patch with testpilot and mozmill. The in mxr listed entries under /mozilla (venkman, irc and inspector) aren't in my clone.

I don't know what happens with the tests when I change the properties in them.

I leave the previous patch active. Then you can decide which one should be used (if one would be used)
Comment 4 Philip Chee 2012-02-28 02:41:30 PST
> (venkman, irc and inspector) aren't in my clone
They are separate nested repositories. You'll need to file separate bugs for these, if there aren't already.
Comment 5 Richard Marti (:Paenglab) 2012-03-01 07:12:09 PST
*** Bug 731999 has been marked as a duplicate of this bug. ***
Comment 6 Richard Marti (:Paenglab) 2012-03-02 09:32:01 PST
Created attachment 602393 [details] [diff] [review]
remove the prefixes

Patch updated after landing of Bug 730930
Comment 7 Blake Winton (:bwinton) (:☕️) 2012-03-06 12:12:24 PST
Comment on attachment 602393 [details] [diff] [review]
remove the prefixes

You missed the following:

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/content/screen.css
196:	-moz-border-radius: 0.25em;

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/skin/all/css/screen-standalone-mobile.css
249:	-moz-border-radius: 0.25em;

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/skin/all/css/screen-standalone.css
232:	-moz-border-radius: 0.25em;

mail/test/resources/mozmill/mozmill/extension/content/css/mozmill.css
42:  -moz-border-radius: 1px;

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/content/screen.css
197:	-webkit-border-radius: 0.25em;

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/skin/all/css/screen-standalone-mobile.css
250:	-webkit-border-radius: 0.25em;

mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/skin/all/css/screen-standalone.css
233:	-webkit-border-radius: 0.25em;

> +++ b/mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/content/browser.css
> @@ -214,7 +214,7 @@
> -    -moz-border-radius: 100%;
> +    border-radius: 10000px;

Why did you change this from 100% to 10000px?

I don't think we should change "jquery-ui-1.7.1.custom.css", anything in the "bespin" directory, or anything in "virtualenv/docs".

So given all of that, I'm gonna say r- for now, but I look forward to the next version.

Thanks,
Blake.
Comment 8 Richard Marti (:Paenglab) 2012-03-06 13:27:03 PST
Created attachment 603435 [details] [diff] [review]
remove the prefixes

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #7)
> Comment on attachment 602393 [details] [diff] [review]
> remove the prefixes
> 
The missed -moz-border-radius and -webkit-border-radius where in not used parts (commented out) -> fixed.

> > +++ b/mail/app/profile/extensions/tbtestpilot@labs.mozilla.com/content/browser.css
> > @@ -214,7 +214,7 @@
> > -    -moz-border-radius: 100%;
> > +    border-radius: 10000px;
> 
> Why did you change this from 100% to 10000px?

I thought this was missed when the behavior of border-radius was changed. But also Firefox has this. So I let the 100% now.

> I don't think we should change "jquery-ui-1.7.1.custom.css", anything in the
> "bespin" directory, or anything in "virtualenv/docs".

Removed from patch
Comment 9 neil@parkwaycc.co.uk 2012-03-06 15:40:53 PST
There used to be a bug in the computation of percentage border radius. The old computation calculated the percentage of the width of the border box, and applied this to the height too. If the height was smaller than the width, then it would apply the rules for radii which are too big, which is to reduce them in proportion, thus it would create a radius using the height instead. The net effect (assuming that the border radius is applied to all corners) is that you would get a rectangle with semicircular end caps.

The new computation calculates the percentage of the width and height independently, resulting in an ellipse (again assuming that the border radius is applied to all corners).
Comment 10 Blake Winton (:bwinton) (:☕️) 2012-03-20 14:21:02 PDT
Comment on attachment 603435 [details] [diff] [review]
remove the prefixes

Okay, I like it.  But I'm also convinced by Neil's explanation, so we should probably re-add that 10000px.  (Or show me a screenshot of each of the options, and I'll pick.  Or you can pick.  :)

Thanks,
Blake.
Comment 11 Richard Marti (:Paenglab) 2012-03-20 14:53:09 PDT
Created attachment 607731 [details]
Badge Comparison. Left 100% right 10000px

This is the comparison between border-radius: 100% and 10000px. I would say the left badge with small number looks with small numbers than the right with radius 10000px. With bigger numbers (here 20) almost no difference.

I propose to leave 100%.
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-03-20 15:02:12 PDT
Works for me!
Comment 13 Richard Marti (:Paenglab) 2012-03-21 01:52:29 PDT
Comment on attachment 603435 [details] [diff] [review]
remove the prefixes

[Approval Request Comment]
The prefix drop was before the merge and it would be good if this can land in aurora. It's all CSS, ergo low risk. If landed we can prevent bug reports about missing radii.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-03-21 16:42:18 PDT
http://hg.mozilla.org/comm-central/rev/a3d01125cea4
Comment 15 Mark Banner (:standard8) 2012-03-27 01:08:27 PDT
Checked in:

http://hg.mozilla.org/releases/comm-aurora/rev/af3f77dda5cf

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