SVG does not respect overflow:visible.

SVG does not respect overflow:visible.

For standalone svg documents, always clip to the viewport ('auto' acts as 'hidden').

For 'overflow: hidden | scroll' on any svg element: clip.
For 'overflow: auto | visible' on any svg element: don't clip.

New behaviours for stand-alone svg documents:
* overflow-x and overflow-y for controlling each scrollbar independently is now supported.
* overflow:scroll will display scrollbars regardless of the svg overflowing or not (as required by CSS).
* overflow:hidden will cause overflow to be clipped away and scrollbars to be hidden.

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

[krit, 2014-04-01 11:51:11]

On 2014/04/01 11:41:12, Erik Dahlström wrote:

Without looking at the patch yet, the statement of the bug is correct but still
incomplete. Beside 'overflow' we have the 'clip' property with the default value
of 'auto'. And the SVG 1.1 spec says:

""The value of auto defines a clipping path along the bounds of the viewport
created by the given element.""

This is the reason why Niko and I never cared about "fixing" it. Even if you
have 'overflow: visible', the spec required the UA to clip to the viewport
anyway.


That said, I did not move this sentence from SVG 1.1 to CSS Masking and removed
it from SVG2. So it is more of an SVG2 feature than a fix.

[1] http://www.w3.org/TR/SVG/masking.html#OverflowAndClipProperties

[fs, 2014-04-01 12:18:30]

https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
File LayoutTests/svg/animations/animate-viewport-overflow.html (right):

https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
LayoutTests/svg/animations/animate-viewport-overflow.html:21: <rect y="200"
width="100" height="100" fill="red">
Could you add a test which moves the rect too (animating 'y' perhaps).

I'm a tad worried because of:

"// Apply initial viewport clip - not affected by overflow settings"

(in RenderSVGRoot::computeFloatRectForRepaint)

[Erik Dahlström (inactive), 2014-04-01 12:20:08]

On 2014/04/01 11:51:11, krit wrote:
> On 2014/04/01 11:41:12, Erik Dahlström wrote:
> 
> Without looking at the patch yet, the statement of the bug is correct but
still
> incomplete. Beside 'overflow' we have the 'clip' property with the default
value
> of 'auto'. And the SVG 1.1 spec says:
> 
> ""The value of auto defines a clipping path along the bounds of the viewport
> created by the given element.""
> 
> This is the reason why Niko and I never cared about "fixing" it. Even if you
> have 'overflow: visible', the spec required the UA to clip to the viewport
> anyway.
> 
> 
> That said, I did not move this sentence from SVG 1.1 to CSS Masking and
removed
> it from SVG2. So it is more of an SVG2 feature than a fix.
> 
> [1] http://www.w3.org/TR/SVG/masking.html#OverflowAndClipProperties

The definition of a clipping path doesn't necessarily imply that it's applied to
anything. From CSS: "The 'clip' property applies to elements that have a
'overflow' property with a value other than 'visible"
(http://www.w3.org/TR/2008/REC-CSS2-20080411/visufx.html#clipping).

In any case, I think we can safely go ahead with this feature/bugfix given that
both IE and Firefox already support this.

[fs, 2014-04-01 12:23:08]

On 2014/04/01 12:18:30, fs wrote:
>
https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
> File LayoutTests/svg/animations/animate-viewport-overflow.html (right):
> 
>
https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
> LayoutTests/svg/animations/animate-viewport-overflow.html:21: <rect y="200"
> width="100" height="100" fill="red">
> Could you add a test which moves the rect too (animating 'y' perhaps).
> 
> I'm a tad worried because of:
> 
> "// Apply initial viewport clip - not affected by overflow settings"
> 
> (in RenderSVGRoot::computeFloatRectForRepaint)

...and make it not apply at 'load'.

[krit, 2014-04-01 12:23:13]

https://codereview.chromium.org/220853002/diff/1/Source/core/css/svg.css
File Source/core/css/svg.css (right):

https://codereview.chromium.org/220853002/diff/1/Source/core/css/svg.css#newc...
Source/core/css/svg.css:51: overflow: hidden;
It of course depends how the spec defines the svg root element, but it says:

""
The initial value for ‘overflow’ as defined in [CSS2-overflow] is 'visible', and
this applies also to the root ‘svg’ element; however, for child elements of an
SVG document, SVG's user agent style sheet overrides this initial value and sets
the ‘overflow’ property on elements that establish new viewports (e.g., ‘svg’
elements), ‘pattern’ elements and ‘marker’ elements to the value 'hidden'.
""

So for all SVG root elements, the value should be visible, not hidden. For all
child elements establishing a viewport the UA style sheet should be hidden.
According to the spec, this edit is wrong.

[Erik Dahlström (inactive), 2014-04-01 13:23:13]

PTAL.

https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
File LayoutTests/svg/animations/animate-viewport-overflow.html (right):

https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/a...
LayoutTests/svg/animations/animate-viewport-overflow.html:21: <rect y="200"
width="100" height="100" fill="red">
On 2014/04/01 12:18:31, fs wrote:
> Could you add a test which moves the rect too (animating 'y' perhaps).
> 
> I'm a tad worried because of:
> 
> "// Apply initial viewport clip - not affected by overflow settings"
> 
> (in RenderSVGRoot::computeFloatRectForRepaint)

Done.

[Erik Dahlström (inactive), 2014-04-05 14:22:05]

On 2014/04/01 12:23:13, krit wrote:
> https://codereview.chromium.org/220853002/diff/1/Source/core/css/svg.css
> File Source/core/css/svg.css (right):
> 
>
https://codereview.chromium.org/220853002/diff/1/Source/core/css/svg.css#newc...
> Source/core/css/svg.css:51: overflow: hidden;
> It of course depends how the spec defines the svg root element, but it says:
> 
> ""
> The initial value for ‘overflow’ as defined in [CSS2-overflow] is 'visible',
and
> this applies also to the root ‘svg’ element; however, for child elements of an
> SVG document, SVG's user agent style sheet overrides this initial value and
sets
> the ‘overflow’ property on elements that establish new viewports (e.g., ‘svg’
> elements), ‘pattern’ elements and ‘marker’ elements to the value 'hidden'.
> ""
> 
> So for all SVG root elements, the value should be visible, not hidden. For all
> child elements establishing a viewport the UA style sheet should be hidden.
> According to the spec, this edit is wrong.

Thanks for reviewing, I've removed the UA stylesheet part.

[esprehn, 2014-04-05 20:19:22]

https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGRoot.cpp:231: bool
RenderSVGRoot::applyViewportClip() const
this should be named with an "is" or "should" prefix. "apply" sounds like a verb
but this thing is just a getter.

[Erik Dahlström (inactive), 2014-04-15 15:19:22]

On 2014/04/05 20:19:22, esprehn wrote:
>
https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGRoot.cpp:231: bool
> RenderSVGRoot::applyViewportClip() const
> this should be named with an "is" or "should" prefix. "apply" sounds like a
verb
> but this thing is just a getter.

Ok, renamed to shouldApplyViewportClip().

[fs, 2014-04-15 15:43:06]

https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/StyleAdjuster.cpp:412: if (element &&
element->isSVGElement()) {
Doesn't this removal require changes elsewhere?
(SVGRenderSupport::isOverflowHidden)

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGRoot.cpp:239: return style()->overflowX() ==
OHIDDEN || style()->overflowX() == OAUTO || this->isRoot();
isRoot() was recently renamed isDocumentElement(). (See for instance use above.)

[fs, 2014-04-15 15:48:42]

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGRoot.cpp:388:
contentRepaintRect.intersect(pixelSnappedBorderBoxRect());
This is a recent addition that also clips to the viewport - you may need to test
if you need the shouldApplyViewportClip() guard here (for things like moving the
entire SVG maybe.)

[Erik Dahlström (inactive), 2014-04-16 15:53:22]

On 2014/04/15 15:48:42, fs wrote:
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGRoot.cpp:388:
> contentRepaintRect.intersect(pixelSnappedBorderBoxRect());
> This is a recent addition that also clips to the viewport - you may need to
test
> if you need the shouldApplyViewportClip() guard here (for things like moving
the
> entire SVG maybe.)

Yes, that could happen. Happy to hear any suggestions for writing a good
testcase for it though. It was failing on overflow:visible on an inline svg
element AND that element was moved by something else in the document AND the svg
was within one render tile but the overflow went outside the tile.

[Erik Dahlström (inactive), 2014-04-16 15:53:48]

https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
File Source/core/css/resolver/StyleAdjuster.cpp (left):

https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
Source/core/css/resolver/StyleAdjuster.cpp:412: if (element &&
element->isSVGElement()) {
On 2014/04/15 15:43:07, fs wrote:
> Doesn't this removal require changes elsewhere?
> (SVGRenderSupport::isOverflowHidden)

It does, SVGRenderSuppoert::isOverflowHidden should deal with this. 

The reason for removing the code here is to allow for different behaviour for
the root <svg>, for nested <svg> elements, and for any other child element of an
<svg>. And it's needed for allowing overflowX and overflowY to be handled
separately (useful in the <svg> document root case).

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGRoot.cpp (right):

https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGRoot.cpp:239: return style()->overflowX() ==
OHIDDEN || style()->overflowX() == OAUTO || this->isRoot();
On 2014/04/15 15:43:07, fs wrote:
> isRoot() was recently renamed isDocumentElement(). (See for instance use
above.)

Fixed.

[pdr., 2014-04-17 22:03:55]

On 2014/04/16 15:53:48, Erik Dahlström wrote:
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleAdjuster.cpp (left):
> 
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver...
> Source/core/css/resolver/StyleAdjuster.cpp:412: if (element &&
> element->isSVGElement()) {
> On 2014/04/15 15:43:07, fs wrote:
> > Doesn't this removal require changes elsewhere?
> > (SVGRenderSupport::isOverflowHidden)
> 
> It does, SVGRenderSuppoert::isOverflowHidden should deal with this. 
> 
> The reason for removing the code here is to allow for different behaviour for
> the root <svg>, for nested <svg> elements, and for any other child element of
an
> <svg>. And it's needed for allowing overflowX and overflowY to be handled
> separately (useful in the <svg> document root case).
> 
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGRoot.cpp (right):
> 
>
https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGRoot.cpp:239: return style()->overflowX()
==
> OHIDDEN || style()->overflowX() == OAUTO || this->isRoot();
> On 2014/04/15 15:43:07, fs wrote:
> > isRoot() was recently renamed isDocumentElement(). (See for instance use
> above.)
> 
> Fixed.

LGTM

[Erik Dahlström (inactive), 2014-04-22 07:25:18]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-04-22 07:25:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/220853002/210001

[commit-bot: I haz the power, 2014-04-22 08:33:39]

Change committed as 172126