Synthesize preserveAspectRatio='none' for non-viewBoxed <img>

Synthesize preserveAspectRatio='none' for non-viewBoxed <img>

For SVGs embedded in an image context, we will synthesize a viewBox that
matches the intrinsic dimensions of the SVGs, if no viewBox is
specified. This would result in an image where the aspect ratio
expressed by the intrinsic dimensions would be preserved.
This does not appear to match author expectations - they rather see that
the behavior in these cases match that of a "regular" raster-based
image. To achieve this, also synthesize a preserveAspectRatio-value of
"none" in these cases. This will result in the original image being
stretched in the most common cases. Any percentage dimensions will be
resolved against the computed dimensions (replaced size), and can hence
also produce a useful viewBox.

The implemented behavior is not currently specced (neither was the old
synthesizing of a viewBox), but matches Gecko.

BUG=110195
Committed: https://crrev.com/2d9c498abd13af52fc331ff516b5826ec354801e
Cr-Commit-Position: refs/heads/master@{#416059}

[fs, 2016-08-31 13:41:24]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 13:41:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[fs, 2016-08-31 15:23:23]

The CQ bit was checked by fs@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 15:23:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-31 16:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 16:50:33]

Dry run: This issue passed the CQ dry run.

[fs, 2016-08-31 17:19:26]

fs@opera.com changed reviewers:
+ davve@opera.com, pdr@chromium.org

[pdr., 2016-09-01 17:17:19]

On 2016/08/31 at 17:19:31, fs wrote:
> 

LGTM

I checked Safari and found they do not do this yet. To prevent torturing
developers too much, can you file a bug at bugs.webkit.org asking them to switch
as well? Our code is still similar so if you add a link to this patch, they may
be willing to just apply the same logic.

[fs, 2016-09-01 17:24:02]

On 2016/09/01 at 17:17:19, pdr wrote:
> On 2016/08/31 at 17:19:31, fs wrote:
> > 
> 
> LGTM
> 
> I checked Safari and found they do not do this yet. To prevent torturing
developers too much, can you file a bug at bugs.webkit.org asking them to switch
as well? Our code is still similar so if you add a link to this patch, they may
be willing to just apply the same logic.

They have one already: https://bugs.webkit.org/show_bug.cgi?id=75970 - I'll dig
up my login and post a link to this review there.

[davve, 2016-09-01 17:46:42]

lgtm

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/as-image/synthesized-viewbox-par-width-px.html
(right):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/as-image/synthesized-viewbox-par-width-px.html:9:
doCombinationTest(
Creative use of that function... :)

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:570: FloatRect
SVGSVGElement::currentViewBoxRect() const
I bothers me a bit that currentViewBoxRect() may return something (relevant)
even though hasEmptyViewBox() is true. Makes top code in
SVGSVGElement::localCoordinateSpaceTransform look a bit odd to me. Nothing new
to this CL, however.

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:594:
synthesizedViewBoxSize.setWidth(width()->currentValue()->scaleByPercentage(currentViewportSize().width()));
So a <svg .. width="50%" height="50%"> would produce the top quarter of the
graphic that would normally be drawn with no width or height, in the same setup?
A bit strange, but oh well I see FF does the exact same thing.

https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4436 for
illustration.

[fs, 2016-09-01 17:57:24]

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/svg/as-image/synthesized-viewbox-par-width-px.html
(right):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/as-image/synthesized-viewbox-par-width-px.html:9:
doCombinationTest(
On 2016/09/01 at 17:46:42, davve wrote:
> Creative use of that function... :)

Well, I didn't want to reinvent a framework strongly resembling something
already invented =) (And I did recall it existed...)

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (left):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:570: FloatRect
SVGSVGElement::currentViewBoxRect() const
On 2016/09/01 at 17:46:42, davve wrote:
> I bothers me a bit that currentViewBoxRect() may return something (relevant)
even though hasEmptyViewBox() is true. Makes top code in
SVGSVGElement::localCoordinateSpaceTransform look a bit odd to me. Nothing new
to this CL, however.

Yeah, I noticed this too... Did feel that I should do something about it in this
CL though.

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGSVGElement.cpp (right):

https://codereview.chromium.org/2290173005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGSVGElement.cpp:594:
synthesizedViewBoxSize.setWidth(width()->currentValue()->scaleByPercentage(currentViewportSize().width()));
On 2016/09/01 at 17:46:42, davve wrote:
> So a <svg .. width="50%" height="50%"> would produce the top quarter of the
graphic that would normally be drawn with no width or height, in the same setup?
A bit strange, but oh well I see FF does the exact same thing.
> 
> https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4436 for
illustration.

Yeah, maybe a bit odd, but I guess it could make some sense in the contexts
where it applies. I also suspect it's quite the odd case that few use...

[fs, 2016-09-01 19:19:08]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-09-01 19:19:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 21:26:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-01 21:29:54]

Description was changed from

==========
Synthesize preserveAspectRatio='none' for non-viewBoxed <img>

For SVGs embedded in an image context, we will synthesize a viewBox that
matches the intrinsic dimensions of the SVGs, if no viewBox is
specified. This would result in an image where the aspect ratio
expressed by the intrinsic dimensions would be preserved.
This does not appear to match author expectations - they rather see that
the behavior in these cases match that of a "regular" raster-based
image. To achieve this, also synthesize a preserveAspectRatio-value of
"none" in these cases. This will result in the original image being
stretched in the most common cases. Any percentage dimensions will be
resolved against the computed dimensions (replaced size), and can hence
also produce a useful viewBox.

The implemented behavior is not currently specced (neither was the old
synthesizing of a viewBox), but matches Gecko.

BUG=110195
==========

to

==========
Synthesize preserveAspectRatio='none' for non-viewBoxed <img>

For SVGs embedded in an image context, we will synthesize a viewBox that
matches the intrinsic dimensions of the SVGs, if no viewBox is
specified. This would result in an image where the aspect ratio
expressed by the intrinsic dimensions would be preserved.
This does not appear to match author expectations - they rather see that
the behavior in these cases match that of a "regular" raster-based
image. To achieve this, also synthesize a preserveAspectRatio-value of
"none" in these cases. This will result in the original image being
stretched in the most common cases. Any percentage dimensions will be
resolved against the computed dimensions (replaced size), and can hence
also produce a useful viewBox.

The implemented behavior is not currently specced (neither was the old
synthesizing of a viewBox), but matches Gecko.

BUG=110195

Committed: https://crrev.com/2d9c498abd13af52fc331ff516b5826ec354801e
Cr-Commit-Position: refs/heads/master@{#416059}
==========

[commit-bot: I haz the power, 2016-09-01 21:29:55]

Patchset 2 (id:??) landed as
https://crrev.com/2d9c498abd13af52fc331ff516b5826ec354801e
Cr-Commit-Position: refs/heads/master@{#416059}