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.

This will incidentally make us pass some IE test center stuff.

We might need to fix our blur radius handling as well to match the latest group decisions.

This should block beta5 considering bug 566045 is blocking beta5.

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?

(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.)

Forget comment 6, I misunderstood was has been said in bug 566045.

(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

It's been suggested that bug 446693 block this bug.

I don't think bug 446693 should block this bug. I think we should just fix the blur radius and rename this.

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.

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?

And, actually, I may as well change the shadowBlur >= 8 back to shadowBlur > 8, since they're now equivalent.

(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.

so r=me if you change 3/2 to 1.5.

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.

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.

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 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 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.

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)

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 on attachment 474280 [details] [diff] [review] rename -moz-box-shadow to box-shadow r=me

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?

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.

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.

Can you explain the approach you're thinking of taking?

I think I answered on IRC, but in any case, we should probably discuss on bug 542595.

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;

Documentation is moved to: https://developer.mozilla.org/en/CSS/box-shadow

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.

Landed missed change from comment 33: http://hg.mozilla.org/mozilla-central/rev/a1e0e5f8e4f9 Thanks for pointing it out.