Closed Bug 590039 Opened 14 years ago Closed 14 years ago

fix blur radius computation and rename -moz-box-shadow to box-shadow

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: dbaron, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: css3, dev-doc-complete)

Attachments

(6 files, 1 obsolete file)

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.
blocking2.0: --- → ?
This will incidentally make us pass some IE test center stuff.
Blocks: ietestcenter
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
Keywords: dev-doc-needed
Depends on: 446693
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.
blocking2.0: ? → beta6+
Attached patch fix blur radius (obsolete) — Splinter Review
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.
Attachment #473831 - Flags: review?(roc)
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.
Attached file 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.
Attached patch fix blur radiusSplinter Review
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.
Attachment #473831 - Attachment is obsolete: true
Attachment #473918 - Flags: review?(roc)
Blocks: 582277
Attachment #474212 - Flags: review? → review?(gavin.sharp)
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.
Attachment #474212 - Flags: review?(gavin.sharp) → review?(dao)
Attachment #474212 - Flags: review?(dao) → review+
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)
Attachment #474280 - Flags: review?(bzbarsky)
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.)
Attachment #474281 - Flags: review?(bzbarsky)
Comment on attachment 474280 [details] [diff] [review]
rename -moz-box-shadow to box-shadow

r=me
Attachment #474280 - Flags: review?(bzbarsky) → review+
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?
Attachment #474281 - Flags: review?(bzbarsky) → review+
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.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Summary: rename -moz-box-shadow to box-shadow → fix blur radius computation and rename -moz-box-shadow to box-shadow
Target Milestone: --- → mozilla2.0b6
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.
Blocks: 595630
Depends on: 595779
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;
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.
Depends on: 602531
Blocks: 602531
No longer depends on: 602531
Depends on: 629587
No longer depends on: 629587
Blocks: unprefix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: