Support subpixel values for text-shadow and box-shadow

Support subpixel values for text-shadow and box-shadow

This converts the int members of ShadowData to LayoutUnit and chases
them through the callstack. The actual calls on the context were already
using float.

There are two caveats:
- When border-radius is used, we don't support subpixel spread values. Subpixel blurs and offsets are still supported
- For inset shadows, we don't support subpixel values

Both limitations come from RoundedRect only supporting integer values.

R=leviw@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165173

[cbiesinger, 2013-08-21 22:09:54]

I can't figure out how to write a test beyond computedstyle... suggestions
welcome!

[eae, 2013-08-24 00:01:11]

On 2013/08/21 22:09:54, cbiesinger wrote:
> I can't figure out how to write a test beyond computedstyle... suggestions
> welcome!

A ref test using non-shadow for the expected.html file perhaps?

[cbiesinger, 2013-08-27 21:57:21]

On 2013/08/24 00:01:11, eae wrote:
> On 2013/08/21 22:09:54, cbiesinger wrote:
> > I can't figure out how to write a test beyond computedstyle... suggestions
> > welcome!
> 
> A ref test using non-shadow for the expected.html file perhaps?

Is that useful? Hmm, I guess I could do a reftest with an integer shadow...

I guess my point is that I don't see a rendering difference from this change,
yet the person who reported this seems to be expecting one.

[eae, 2013-08-27 21:58:40]

On 2013/08/27 21:57:21, cbiesinger wrote:
> On 2013/08/24 00:01:11, eae wrote:
> > On 2013/08/21 22:09:54, cbiesinger wrote:
> > > I can't figure out how to write a test beyond computedstyle... suggestions
> > > welcome!
> > 
> > A ref test using non-shadow for the expected.html file perhaps?
> 
> Is that useful? Hmm, I guess I could do a reftest with an integer shadow...
> 
> I guess my point is that I don't see a rendering difference from this change,
> yet the person who reported this seems to be expecting one.

If there is no rendering difference then why make the change?

[cbiesinger, 2013-10-23 23:01:14]

On 2013/08/27 21:58:40, eae wrote:
> On 2013/08/27 21:57:21, cbiesinger wrote:
> > On 2013/08/24 00:01:11, eae wrote:
> > > On 2013/08/21 22:09:54, cbiesinger wrote:
> > > > I can't figure out how to write a test beyond computedstyle...
suggestions
> > > > welcome!
> > > 
> > > A ref test using non-shadow for the expected.html file perhaps?
> > 
> > Is that useful? Hmm, I guess I could do a reftest with an integer shadow...
> > 
> > I guess my point is that I don't see a rendering difference from this
change,
> > yet the person who reported this seems to be expecting one.
> 
> If there is no rendering difference then why make the change?

To update on this, apparently there is a rendering difference on retina (only).
I don't have a retina display... and http://crbug.com/310839 makes testing this
tricky.

[mithro-old, 2013-10-24 00:45:41]

Random minor comments;

Should there be a pixel test for this too?

https://codereview.chromium.org/23241010/diff/4001/LayoutTests/animations/res...
File LayoutTests/animations/resources/animation-test-helpers.js (right):

https://codereview.chromium.org/23241010/diff/4001/LayoutTests/animations/res...
LayoutTests/animations/resources/animation-test-helpers.js:383: // Shadow
cssText looks like "rgb(0, 0, 255) 0px -3px 10px 0px"
Comment needs to be updated with the change to the regex.

[cbiesinger, 2013-10-26 01:26:05]

I have good news and bad news:

I figured out how to test this correctly (this is the good news). When testing,
I found other pieces of code that I missed. They are easy to fix for text-shadow
(more good news).

But for box-shadow, there's lots of code I don't really understand that seems
hard to fix. I think I'll start with making this work for text-shadow only...

https://codereview.chromium.org/23241010/diff/4001/LayoutTests/animations/res...
File LayoutTests/animations/resources/animation-test-helpers.js (right):

https://codereview.chromium.org/23241010/diff/4001/LayoutTests/animations/res...
LayoutTests/animations/resources/animation-test-helpers.js:383: // Shadow
cssText looks like "rgb(0, 0, 255) 0px -3px 10px 0px"
On 2013/10/24 00:45:41, mithro wrote:
> Comment needs to be updated with the change to the regex.

I guess... will do.

[cbiesinger, 2013-10-26 01:36:48]

Levi, Emil: Do you know if there's a reason why RoundedRect uses integer sizes
instead of layout units? This is making this implementation harder due to
GraphicsContext::drawInnerShadow

(I have since figured out that I can test this by setting
testRunner.setBackingScaleFactor which does make a visual difference)

[cbiesinger, 2013-11-22 01:55:19]

This is 90% complete, I just need to verify that subpixel spread values work
correctly... I'm not sure that they do in this version of the patch. But it's
getting late here on the east coast.

[cbiesinger, 2013-11-23 00:23:57]

On 2013/11/22 01:55:19, cbiesinger wrote:
> This is 90% complete, I just need to verify that subpixel spread values work
> correctly... I'm not sure that they do in this version of the patch. But it's
> getting late here on the east coast.

This is all ready for review now! Changed quite a bit from the initial version.

[eae, 2013-11-27 12:02:27]

On 2013/11/23 00:23:57, cbiesinger wrote:
> On 2013/11/22 01:55:19, cbiesinger wrote:
> > This is 90% complete, I just need to verify that subpixel spread values work
> > correctly... I'm not sure that they do in this version of the patch. But
it's
> > getting late here on the east coast.
> 
> This is all ready for review now! Changed quite a bit from the initial
version.

I see that you have added a high-dpi test for it (yay, thank you!), could you
please explain what it is supposed to show though, it renders nearly identical
for me with and without the patch (with fake high-dpi). I.e. what is the problem
the patch solves?

[cbiesinger, 2013-11-27 22:57:18]

On 2013/11/27 12:02:27, eae wrote:
> On 2013/11/23 00:23:57, cbiesinger wrote:
> > On 2013/11/22 01:55:19, cbiesinger wrote:
> > > This is 90% complete, I just need to verify that subpixel spread values
work
> > > correctly... I'm not sure that they do in this version of the patch. But
> it's
> > > getting late here on the east coast.
> > 
> > This is all ready for review now! Changed quite a bit from the initial
> version.
> 
> I see that you have added a high-dpi test for it (yay, thank you!), could you
> please explain what it is supposed to show though, it renders nearly identical
> for me with and without the patch (with fake high-dpi). I.e. what is the
problem
> the patch solves?

Isn't that the case for all subpixel layout? It's all "nearly identical"... But
I can ask for more information.

[cbiesinger, 2013-11-27 22:58:26]

On 2013/11/27 22:57:18, cbiesinger wrote:
> On 2013/11/27 12:02:27, eae wrote:
> > On 2013/11/23 00:23:57, cbiesinger wrote:
> > > On 2013/11/22 01:55:19, cbiesinger wrote:
> > > > This is 90% complete, I just need to verify that subpixel spread values
> work
> > > > correctly... I'm not sure that they do in this version of the patch. But
> > it's
> > > > getting late here on the east coast.
> > > 
> > > This is all ready for review now! Changed quite a bit from the initial
> > version.
> > 
> > I see that you have added a high-dpi test for it (yay, thank you!), could
you
> > please explain what it is supposed to show though, it renders nearly
identical
> > for me with and without the patch (with fake high-dpi). I.e. what is the
> problem
> > the patch solves?
> 
> Isn't that the case for all subpixel layout? It's all "nearly identical"...
But
> I can ask for more information.

Oh, right, basically the idea is to be able to display hairline strokes and
small shadows on hi-dpi displays that otherwise would get rounded away.

[eae, 2013-11-27 23:45:27]

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2550: // XXX is this IntRect
conversion correct?
Why not use pixelSnappedIntRect instead?

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2568: // inset shadow
Update comment to be more descriptive or remove it.

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2583:
context->drawInnerShadow(border, shadowColor, IntSize(shadowOffset.width(),
shadowOffset.height()), shadowBlur, shadowSpread, clippedEdges);
do you want to floor or round the offset here? Use flooredIntSize or
roundedIntSize to be explicit.

[dstockwell, 2013-11-27 23:49:30]

https://codereview.chromium.org/23241010/diff/230001/LayoutTests/animations/i...
File LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/23241010/diff/230001/LayoutTests/animations/i...
LayoutTests/animations/interpolation/resources/interpolation-test.js:197: //
Round numbers to one decimal place. Our layout units are not exact
This shouldn't be necessary and isn't really appropriate. This is used in more
than the comparisons of just layout units.

Are the test results you are producing not stable?

[Timothy Loh, 2013-12-02 07:03:09]

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/st...
File Source/core/rendering/style/ShadowData.h (right):

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/st...
Source/core/rendering/style/ShadowData.h:62: LayoutPoint m_location;
I don't think we store LayoutUnits/etc. in style data? Is using
FloatPoint/float/float more appropriate here (this will also probably resolve
dstockwell's concern)?

[cbiesinger, 2013-12-18 02:09:11]

All comments fixed, please take another look.

https://codereview.chromium.org/23241010/diff/230001/LayoutTests/animations/i...
File LayoutTests/animations/interpolation/resources/interpolation-test.js
(right):

https://codereview.chromium.org/23241010/diff/230001/LayoutTests/animations/i...
LayoutTests/animations/interpolation/resources/interpolation-test.js:197: //
Round numbers to one decimal place. Our layout units are not exact
On 2013/11/27 23:49:31, dstockwell wrote:
> This shouldn't be necessary and isn't really appropriate. This is used in more
> than the comparisons of just layout units.
> 
> Are the test results you are producing not stable?

Reverted, see below.

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2550: // XXX is this IntRect
conversion correct?
On 2013/11/27 23:45:27, eae wrote:
> Why not use pixelSnappedIntRect instead?

Good idea, thanks.

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2568: // inset shadow
On 2013/11/27 23:45:27, eae wrote:
> Update comment to be more descriptive or remove it.

Done.

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2583:
context->drawInnerShadow(border, shadowColor, IntSize(shadowOffset.width(),
shadowOffset.height()), shadowBlur, shadowSpread, clippedEdges);
On 2013/11/27 23:45:27, eae wrote:
> do you want to floor or round the offset here? Use flooredIntSize or
> roundedIntSize to be explicit.

Ah, thanks - done. Switched to floored because that's what we used to do.

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/st...
File Source/core/rendering/style/ShadowData.h (right):

https://codereview.chromium.org/23241010/diff/230001/Source/core/rendering/st...
Source/core/rendering/style/ShadowData.h:62: LayoutPoint m_location;
On 2013/12/02 07:03:10, Timothy Loh wrote:
> I don't think we store LayoutUnits/etc. in style data? Is using
> FloatPoint/float/float more appropriate here (this will also probably resolve
> dstockwell's concern)?

Ah, looks like you're right - we store Lengths or floats.

I turned these into floats, and that did indeed solve dstockwell's issue as
well! Thanks for pointing that out!

[cbiesinger, 2014-01-09 02:18:17]

Happy new year, everyone!

Levi, Emil - could you take another look?

[leviw_travelin_and_unemployed, 2014-01-14 22:28:34]

2 Questions: why is the png a green box with no shadows and do we need both
versions of adjustRectForShadow in ShadowList.

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/shadows-computed-style.html (right):

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/shadows-computed-style.html:23: style =
window.getComputedStyle(box, null);
Maybe use js-test shouldBe's?

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/shadows-hidpi.html (right):

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/shadows-hidpi.html:11: setTimeout(function() {
testRunner.notifyDone() }, 0);
Is this setTimeout necessary?

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2578: // TODO: support
non-integer shadows
Can we get a bug?

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/st...
File Source/core/rendering/style/ShadowList.cpp (right):

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/st...
Source/core/rendering/style/ShadowList.cpp:63: rect.setWidth(rect.width() -
shadowLeft + shadowRight);
This seems to just treat the LayoutUnits like floats (implicitly). If that's the
behavior we want, shouldn't we just have this call through to the float version?

[cbiesinger, 2014-01-15 21:27:39]

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/shadows-computed-style.html (right):

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/shadows-computed-style.html:23: style =
window.getComputedStyle(box, null);
On 2014/01/14 22:28:35, Levi wrote:
> Maybe use js-test shouldBe's?

Done.

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
File LayoutTests/fast/sub-pixel/shadows-hidpi.html (right):

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/shadows-hidpi.html:1: <!DOCTYPE html>
Not sure what was going on with the -expected.png for this test but I ran git cl
upload again and it's there now, looking correct.

https://codereview.chromium.org/23241010/diff/310001/LayoutTests/fast/sub-pix...
LayoutTests/fast/sub-pixel/shadows-hidpi.html:11: setTimeout(function() {
testRunner.notifyDone() }, 0);
On 2014/01/14 22:28:35, Levi wrote:
> Is this setTimeout necessary?

Apparently not - removed.

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/Re...
File Source/core/rendering/RenderBoxModelObject.cpp (right):

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/Re...
Source/core/rendering/RenderBoxModelObject.cpp:2578: // TODO: support
non-integer shadows
On 2014/01/14 22:28:35, Levi wrote:
> Can we get a bug?

Done.

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/st...
File Source/core/rendering/style/ShadowList.cpp (right):

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/st...
Source/core/rendering/style/ShadowList.cpp:54: void
ShadowList::adjustRectForShadow(LayoutRect& rect, LayoutUnit
additionalOutlineSize) const
Turns out we do need both versions; rendering/svg/RenderSVGText.cpp uses the
FloatRect one and the two other callers use LayoutRects.

However, the one caller that passes a nonzero additionalOutlineSize passes an
integer. I've changed the type to a float.

https://codereview.chromium.org/23241010/diff/310001/Source/core/rendering/st...
Source/core/rendering/style/ShadowList.cpp:63: rect.setWidth(rect.width() -
shadowLeft + shadowRight);
On 2014/01/14 22:28:35, Levi wrote:
> This seems to just treat the LayoutUnits like floats (implicitly). If that's
the
> behavior we want, shouldn't we just have this call through to the float
version?

Changed.

[leviw_travelin_and_unemployed, 2014-01-15 21:33:52]

Okay. LGTM.

[commit-bot: I haz the power, 2014-01-15 23:45:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cbiesinger@chromium.org/23241010/500001

[commit-bot: I haz the power, 2014-01-15 23:54:04]

Change committed as 165173