Last Comment Bug 590039 - fix blur radius computation and rename -moz-box-shadow to box-shadow
: fix blur radius computation and rename -moz-box-shadow to box-shadow
Status: RESOLVED FIXED
: css3, dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b7
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 589674 589676 589677 (view as bug list)
Depends on: 446693 595779
Blocks: ietestcenter css3-background 566045 582277 595630 602531
  Show dependency treegraph
 
Reported: 2010-08-23 22:55 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2011-02-04 12:31 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
fix blur radius (5.02 KB, patch)
2010-09-09 17:18 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
Details | Diff | Review
source for graph (1.54 KB, text/plain)
2010-09-09 20:09 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
graph of Gaussian vs. triple box blur (52.69 KB, image/svg+xml)
2010-09-09 20:10 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
fix blur radius (15.68 KB, patch)
2010-09-09 21:02 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
Details | Diff | Review
audit themes (and a few tests) for blur radius changes (68.00 KB, patch)
2010-09-10 14:37 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
dao+bmo: review+
Details | Diff | Review
rename -moz-box-shadow to box-shadow (126.66 KB, patch)
2010-09-10 16:46 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review
rename -moz-box-shadow to box-shadow, manual changes (7.86 KB, patch)
2010-09-10 16:50 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-23 22:55:14 PDT
Bug 566045 is proposing adding default styles to our UA style sheet with -moz-box-shadow.  I think it's bad for authors to do this, since authors might want to disable these styles.  So I think renaming -moz-box-shadow to box-shadow blocks doing this.

However, -moz-box-shadow hasn't been in CR; it was readded to borders and backgrounds in the post-CR last call draft.

I think we should probably rename it anyway.
Comment 1 Boris Zbarsky [:bz] 2010-08-23 23:06:26 PDT
This will incidentally make us pass some IE test center stuff.
Comment 2 Boris Zbarsky [:bz] 2010-08-23 23:07:44 PDT
*** Bug 589676 has been marked as a duplicate of this bug. ***
Comment 3 Boris Zbarsky [:bz] 2010-08-23 23:07:48 PDT
*** Bug 589677 has been marked as a duplicate of this bug. ***
Comment 4 Boris Zbarsky [:bz] 2010-08-23 23:08:13 PDT
*** Bug 589674 has been marked as a duplicate of this bug. ***
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-08-23 23:24:00 PDT
We might need to fix our blur radius handling as well to match the latest group decisions.
Comment 6 Mounir Lamouri (:mounir) 2010-08-23 23:27:46 PDT
This should block beta5 considering bug 566045 is blocking beta5.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-24 03:45:21 PDT
I've lost track of where the discussion on www-style went. Fantasai, is the text at http://dev.w3.org/csswg/css3-background/#the-box-shadow going to change?
Comment 8 Tab Atkins Jr. 2010-08-24 06:15:32 PDT
(In reply to comment #7)
> I've lost track of where the discussion on www-style went. Fantasai, is the
> text at http://dev.w3.org/csswg/css3-background/#the-box-shadow going to
> change?

It should - the current text says to approximate a gaussian with a stdev equal to the blur radius, which would generate *enormous* shadows.  The stdev should be equal to half the specified blur radius.

(As far as I can tell, this mistake has to be accidental.  There were two schools of thought regarding what the "blur radius" should mean, and neither would result in anything close to the current spec wording.)
Comment 9 Mounir Lamouri (:mounir) 2010-08-24 13:55:33 PDT
Forget comment 6, I misunderstood was has been said in bug 566045.
Comment 10 Tab Atkins Jr. 2010-08-24 15:34:06 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > I've lost track of where the discussion on www-style went. Fantasai, is the
> > text at http://dev.w3.org/csswg/css3-background/#the-box-shadow going to
> > change?
> 
> It should - the current text says to approximate a gaussian with a stdev equal
> to the blur radius, which would generate *enormous* shadows.  The stdev should
> be equal to half the specified blur radius.
> 
> (As far as I can tell, this mistake has to be accidental.  There were two
> schools of thought regarding what the "blur radius" should mean, and neither
> would result in anything close to the current spec wording.)

All right, current editor's draft has the correct text now.

http://dev.w3.org/csswg/css3-background/#the-box-shadow
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-05 08:04:31 PDT
It's been suggested that bug 446693 block this bug.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-07 21:18:23 PDT
I don't think bug 446693 should block this bug. I think we should just fix the blur radius and rename this.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 17:18:19 PDT
Created attachment 473831 [details] [diff] [review]
fix blur radius

I think this is what the relevant specs say, but I have rather low confidence in this patch since I don't have any testcases written by others.  I'll try posting to www-style after I land it.

This does appear to increase our large canvas blurs, which does appear to match WebKit for canvas, and the quirk in the spec was definitely reverse-engineered from WebKit.

See the long comment in the commit message for a more detailed explanation of the changes.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 17:21:09 PDT
Also, post-bug 467518, does the radius measure in our box blur implementation still roughly match what the SVG spec describes as a box blur?
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 17:22:01 PDT
And, actually, I may as well change the shadowBlur >= 8 back to shadowBlur > 8, since they're now equivalent.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-09 17:35:25 PDT
(In reply to comment #14)
> Also, post-bug 467518, does the radius measure in our box blur implementation
> still roughly match what the SVG spec describes as a box blur?

Hmm. Actually I think that factor of 3/2 is needed to make that work out right. But of course it should be 1.5 so it's not broken.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-09 17:35:37 PDT
so r=me if you change 3/2 to 1.5.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-09 19:35:35 PDT
OK, so here's the situation assuming we take your patch with 3/2 changed to 1.5.

gfxAlphaBoxBlur::Init's aBlurRadius is the radius of pixels affected by the overall blur. That means that the sum of the left lobes of the three box blurs is aBlurRadius, and the sum of the right lobes of the three box blurs is aBlurRadius. (ComputeLobes guarantees that.)

The SVG 1.1 spec says that the box blur size d should be defined by d = floor(s * 3*sqrt(2*pi)/4 + 0.5) where s is the standard deviation. Our box-blur sizes are approximately 2/3 of aBlurRadius (two lobes divided by three box-blurs), so gfxAlphaBoxBlur::CalculateBlurRadius should return a radius that's approximately 3/2 of s*3*sqrt(2*pi)/4.

So basically that means we should be doing the right things. The confusing bit is that aBlurRadius is not the same as the blur radius defined in the CSS spec.
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 20:09:38 PDT
Created attachment 473907 [details]
source for graph

This is an R program that's the source for a graph showing the difference between a triple box blur and a Gaussian.  (Gaussians are nice for blurring, I suppose, because they allow X and Y to be done independently such that the blur is still circular, and behaves just as it would if the axes were rotated 45 degrees.)

I worked out the distribution function for a triple box blur; it's piecewise quadratic.  That bit is on paper, so I'm not attaching it here.  But it's in the code here (especially if you can read R).

In any case, I'd note that the formula in the SVG spec, http://www.w3.org/TR/SVG11/filters.html#feGaussianBlurElement , gives the diameter of the individual box blurs (the width of the kernel for each individual box blur), whereas our code expects the *radius* of the kernel for the *entire* triple box blur (so that each individual box blur has a kernel whose radius is 1/3 that value, or whose *diameter* is 2/3 that value).  That's where the factor of 1.5 comes from.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 20:10:04 PDT
Created attachment 473908 [details]
graph of Gaussian vs. triple box blur
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-09 21:02:10 PDT
Created attachment 473918 [details] [diff] [review]
fix blur radius

Revised with new math, new comments, and fixes for rect inflation/deflation.

text-shadow and box-shadow reftests pass locally; I'll see what try says overnight.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-10 14:37:08 PDT
Created attachment 474212 [details] [diff] [review]
audit themes (and a few tests) for blur radius changes
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-10 14:42:07 PDT
Comment on attachment 474212 [details] [diff] [review]
audit themes (and a few tests) for blur radius changes

This is really repetitive, but basically it's a replacement of the blur radius values according to the following table:

from to
1px 1px
2px 1.5px
3px 2px
4px 3px
5px 3.5px
6px 4px
7px 5px
8px 5.5px
9px 6.5px
10px 7px
11px 8px
12px 8.5px
13px 9px
14px 10px
15px 10.5px
16px 11.5px
17px 12px
18px 13px
19px 13.5px
20px 14px
21px 15px
22px 15.5px
23px 16px
24px 17px
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-10 14:48:29 PDT
Comment on attachment 474212 [details] [diff] [review]
audit themes (and a few tests) for blur radius changes

These changes are to preserve behavior in the presence of the previous patch.  This attempts to follow the table in the previous comment.  The commit message for the previous patch explains (in slightly more detail than in the version above):

This changes text-shadow and -moz-box-shadow handling to use
CalculateBlurRadius on half of the value given instead of passing the
value through directly.  This means that text-shadow and box-shadow
blurs are multiplied by 1.410 relative to their old sizes.  It also
means that we round rather than floor, so that the effect that used to
be drawn by a blur in the range 1px to 1.99px is now drawn by a blur
anywhere in the range 0.36px to 1.05px, the effect that used to be drawn
by a blur in the range 2px to 2.99px is now drawn by a blur anywhere in
the range 1.06px to 1.77px, what used to be a drawn by a blur in the
range 3px to 3.99px is now drawn by a blur anywhere in the range 1.78px
to 2.47px, etc.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-10 16:46:30 PDT
Created attachment 474280 [details] [diff] [review]
rename -moz-box-shadow to box-shadow

This patch was written with sed; see the commit message.  (There's one correction to what this did in the next patch, in nsCSSPropList.h)
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-10 16:50:45 PDT
Created attachment 474281 [details] [diff] [review]
rename -moz-box-shadow to box-shadow, manual changes

These are the manual changes.

Note that it is technically a little early to be doing this renaming.  However, (a) I really want this for the reasons described in bug 566045 comment 23 -- if we're going to have a box-shadow style in our UA style sheet that authors are reasonably likely to want to override, we should be using the standard property and (b) it's pretty likely to be in CR by the time we ship.

(I am, however, somewhat concerned about the overflow issue.)
Comment 27 Boris Zbarsky [:bz] 2010-09-10 19:26:36 PDT
Comment on attachment 474280 [details] [diff] [review]
rename -moz-box-shadow to box-shadow

r=me
Comment 28 Boris Zbarsky [:bz] 2010-09-10 19:28:30 PDT
Comment on attachment 474281 [details] [diff] [review]
rename -moz-box-shadow to box-shadow, manual changes

I agree about the overflow issue... I assume there's no way we can fix that before 2.0?

r=me on this, though; the UA stylesheet thing is a bigger problem, I think.  Unless we want to make some hack where box-shadow:none works but nothing else?
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-11 10:09:19 PDT
Ah, and one other note about being CR (I'd forgotten whether this actually happened or whether it was about to happen):  the CSS WG did resolve to publish a CR: http://lists.w3.org/Archives/Public/www-style/2010Sep/0002.html so hopefully the CR will be out within the next few weeks.
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-11 10:11:19 PDT
http://hg.mozilla.org/mozilla-central/rev/830111e10951
http://hg.mozilla.org/mozilla-central/rev/7e330021ce68
http://hg.mozilla.org/mozilla-central/rev/11cf38adabf3
http://hg.mozilla.org/mozilla-central/rev/83a79e1e035b

I'm going to try working on the overflow issue and see how hard it is.  I actually think it won't be too bad.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-09-11 14:35:39 PDT
Can you explain the approach you're thinking of taking?
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-09-11 20:18:50 PDT
I think I answered on IRC, but in any case, we should probably discuss on bug 542595.
Comment 33 Philip Chee 2010-09-13 08:44:49 PDT
The following comment in https://bugzilla.mozilla.org/show_bug.cgi?id=595630#c5
was misfiled under the wrong bug. I think it was meant to be put here:

> but I
> think you missed this one at least:
> 
>  #identity-box:-moz-focusring {
> -  -moz-box-shadow: 0 0 3px 1px -moz-mac-focusring inset,
> +  -moz-box-shadow: 0 0 2px 1px -moz-mac-focusring inset,
>                     0 0 3px 2px -moz-mac-focusring;
Comment 34 Eric Shepherd [:sheppy] 2010-10-04 14:00:19 PDT
Documentation is moved to:

https://developer.mozilla.org/en/CSS/box-shadow
Comment 35 Eric Shepherd [:sheppy] 2010-10-04 14:04:09 PDT
I should add that I made no changes to the text, other than finishing the renames of -moz-box-shadow to box-shadow; if the blur radius computation change requires other changes (it doesn't look like it does to me), let me know.
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-10-09 09:40:35 PDT
Landed missed change from comment 33:
http://hg.mozilla.org/mozilla-central/rev/a1e0e5f8e4f9

Thanks for pointing it out.

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