Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(373)

Issue 220853002: SVG does not respect overflow:visible. (Closed)

Created:
6 years, 4 months ago by Erik Dahlström (inactive)
Modified:
6 years, 3 months ago
CC:
blink-reviews, shans, eae+blinkwatch, fs, apavlov+blink_chromium.org, Steve Block, rwlbuis, dino_apple.com, alancutter (OOO until 2018), bemjb+rendering_chromium.org, dsinclair, Timothy Loh, dstockwell, dglazkov+blink, jchaffraix+rendering, rune+blink, Eric Willigers, rjwright, zoltan1, gyuyoung.kim_webkit.org, darktears, kouhei+svg_chromium.org, leviw+renderwatch, Mike Lawther (Google), ed+blinkwatch_opera.com, f(malita), Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 3

Patch Set 2 : review fixes #

Patch Set 3 : added missing W3C tests (relevant for overflow:auto) #

Patch Set 4 : rebase #

Total comments: 1

Patch Set 5 : overflow affects root svg's scrollbars #

Total comments: 5

Patch Set 6 : review fixes #

Patch Set 7 : fix tests, and be slightly more conservative wrt scrollbars #

Patch Set 8 : rebase #

Patch Set 9 : revert the rest of crbug.com/362510 #

Patch Set 10 : fix ref image #

Patch Set 11 : undo faulty test expectation edit #

Patch Set 12 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+902 lines, -92 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +7 lines, -0 lines 0 comments Download
D LayoutTests/fast/svg/different-overflow-values.html View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -22 lines 0 comments Download
D LayoutTests/fast/svg/different-overflow-values-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -12 lines 0 comments Download
A LayoutTests/svg/W3C-SVG-1.1-SE/painting-marker-05-f.svg View 1 2 1 chunk +240 lines, -0 lines 0 comments Download
A LayoutTests/svg/W3C-SVG-1.1-SE/painting-marker-05-f-expected.txt View 1 2 1 chunk +102 lines, -0 lines 0 comments Download
A LayoutTests/svg/W3C-SVG-1.1-SE/painting-marker-06-f.svg View 1 2 4 1 chunk +159 lines, -0 lines 0 comments Download
A + LayoutTests/svg/W3C-SVG-1.1-SE/painting-marker-06-f-expected.txt View 1 2 4 chunks +20 lines, -17 lines 0 comments Download
A LayoutTests/svg/animations/animate-viewport-overflow.html View 1 chunk +27 lines, -0 lines 0 comments Download
A LayoutTests/svg/animations/animate-viewport-overflow-2.html View 1 1 chunk +29 lines, -0 lines 0 comments Download
A LayoutTests/svg/animations/animate-viewport-overflow-2-expected.html View 1 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/svg/animations/animate-viewport-overflow-expected.html View 1 chunk +15 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-repaint.html View 1 chunk +23 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-repaint-expected.html View 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root.html View 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root-attr.html View 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root-attr-expected.html View 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root-expected.html View 1 chunk +17 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root-style.html View 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/svg/in-html/overflow-svg-root-style-expected.html View 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-on-outermost-svg-element-horizontal-auto.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-on-outermost-svg-element-horizontal-auto-expected.html View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
M LayoutTests/svg/overflow/overflow-on-outermost-svg-element-ignore-attribute-1-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/svg/overflow/overflow-on-outermost-svg-element-ignore-attribute-3-expected.txt View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M LayoutTests/svg/overflow/overflow-on-outermost-svg-element-in-xhtml-visible.xhtml View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/svg/overflow/overflow-on-outermost-svg-element-vertical-auto.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-on-outermost-svg-element-vertical-auto-expected.html View 1 2 3 4 5 6 7 8 9 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-scroll-on-outermost-svg-element.svg View 1 2 3 4 5 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-scroll-on-outermost-svg-element-expected.html View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-visible-repaint-expected.txt View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-x-hidden-on-outermost-svg-element.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-x-hidden-on-outermost-svg-element-expected.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-y-hidden-on-outermost-svg-element.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/svg/overflow/overflow-y-hidden-on-outermost-svg-element-expected.svg View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/css/resolver/StyleAdjuster.cpp View 1 2 3 4 1 chunk +0 lines, -21 lines 0 comments Download
M Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -5 lines 0 comments Download
M Source/core/rendering/svg/RenderSVGRoot.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/rendering/svg/RenderSVGRoot.cpp View 1 2 3 4 5 6 7 8 9 10 11 5 chunks +26 lines, -5 lines 0 comments Download
M Source/core/rendering/svg/SVGRenderSupport.cpp View 1 2 3 4 5 1 chunk +1 line, -7 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
Erik Dahlström (inactive)
6 years, 4 months ago (2014-04-01 11:41:12 UTC) #1
krit
On 2014/04/01 11:41:12, Erik Dahlström wrote: Without looking at the patch yet, the statement of ...
6 years, 4 months ago (2014-04-01 11:51:11 UTC) #2
fs
https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html File LayoutTests/svg/animations/animate-viewport-overflow.html (right): https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html#newcode21 LayoutTests/svg/animations/animate-viewport-overflow.html:21: <rect y="200" width="100" height="100" fill="red"> Could you add a ...
6 years, 4 months ago (2014-04-01 12:18:30 UTC) #3
Erik Dahlström (inactive)
On 2014/04/01 11:51:11, krit wrote: > On 2014/04/01 11:41:12, Erik Dahlström wrote: > > Without ...
6 years, 4 months ago (2014-04-01 12:20:08 UTC) #4
fs
On 2014/04/01 12:18:30, fs wrote: > https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html > File LayoutTests/svg/animations/animate-viewport-overflow.html (right): > > https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html#newcode21 > ...
6 years, 4 months ago (2014-04-01 12:23:08 UTC) #5
krit
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#newcode51 Source/core/css/svg.css:51: overflow: hidden; It of course depends how the spec ...
6 years, 4 months ago (2014-04-01 12:23:13 UTC) #6
Erik Dahlström (inactive)
PTAL. https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html File LayoutTests/svg/animations/animate-viewport-overflow.html (right): https://codereview.chromium.org/220853002/diff/1/LayoutTests/svg/animations/animate-viewport-overflow.html#newcode21 LayoutTests/svg/animations/animate-viewport-overflow.html:21: <rect y="200" width="100" height="100" fill="red"> On 2014/04/01 12:18:31, ...
6 years, 4 months ago (2014-04-01 13:23:13 UTC) #7
Erik Dahlström (inactive)
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#newcode51 > ...
6 years, 4 months ago (2014-04-05 14:22:05 UTC) #8
esprehn
https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/svg/RenderSVGRoot.cpp File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/svg/RenderSVGRoot.cpp#newcode231 Source/core/rendering/svg/RenderSVGRoot.cpp:231: bool RenderSVGRoot::applyViewportClip() const this should be named with an ...
6 years, 4 months ago (2014-04-05 20:19:22 UTC) #9
Erik Dahlström (inactive)
On 2014/04/05 20:19:22, esprehn wrote: > https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/svg/RenderSVGRoot.cpp > File Source/core/rendering/svg/RenderSVGRoot.cpp (right): > > https://codereview.chromium.org/220853002/diff/50001/Source/core/rendering/svg/RenderSVGRoot.cpp#newcode231 > ...
6 years, 4 months ago (2014-04-15 15:19:22 UTC) #10
fs
https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp File Source/core/css/resolver/StyleAdjuster.cpp (left): https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp#oldcode412 Source/core/css/resolver/StyleAdjuster.cpp:412: if (element && element->isSVGElement()) { Doesn't this removal require ...
6 years, 4 months ago (2014-04-15 15:43:06 UTC) #11
fs
https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/svg/RenderSVGRoot.cpp File Source/core/rendering/svg/RenderSVGRoot.cpp (right): https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/svg/RenderSVGRoot.cpp#newcode388 Source/core/rendering/svg/RenderSVGRoot.cpp:388: contentRepaintRect.intersect(pixelSnappedBorderBoxRect()); This is a recent addition that also clips ...
6 years, 4 months ago (2014-04-15 15:48:42 UTC) #12
Erik Dahlström (inactive)
On 2014/04/15 15:48:42, fs wrote: > https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/svg/RenderSVGRoot.cpp > File Source/core/rendering/svg/RenderSVGRoot.cpp (right): > > https://codereview.chromium.org/220853002/diff/70001/Source/core/rendering/svg/RenderSVGRoot.cpp#newcode388 > ...
6 years, 3 months ago (2014-04-16 15:53:22 UTC) #13
Erik Dahlström (inactive)
https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp File Source/core/css/resolver/StyleAdjuster.cpp (left): https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp#oldcode412 Source/core/css/resolver/StyleAdjuster.cpp:412: if (element && element->isSVGElement()) { On 2014/04/15 15:43:07, fs ...
6 years, 3 months ago (2014-04-16 15:53:48 UTC) #14
pdr.
On 2014/04/16 15:53:48, Erik Dahlström wrote: > https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp > File Source/core/css/resolver/StyleAdjuster.cpp (left): > > https://codereview.chromium.org/220853002/diff/70001/Source/core/css/resolver/StyleAdjuster.cpp#oldcode412 ...
6 years, 3 months ago (2014-04-17 22:03:55 UTC) #15
Erik Dahlström (inactive)
The CQ bit was checked by ed@opera.com
6 years, 3 months ago (2014-04-22 07:25:18 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/220853002/210001
6 years, 3 months ago (2014-04-22 07:25:32 UTC) #17
commit-bot: I haz the power
6 years, 3 months ago (2014-04-22 08:33:39 UTC) #18
Message was sent while issue was closed.
Change committed as 172126

Powered by Google App Engine
This is Rietveld 408576698