Bug 11225 – No scroll bars are displayed for standalone SVG image

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[KwisA]

In viewing this svg image it is not displaying scroll bars

http://upload.wikimedia.org/wikipedia/en/e/e5/PowerStation2.svg
the above image is locket at the top left of the image im not abe to scroll it,

http://upload.wikimedia.org/wikipedia/en/e/e5/PowerStation3.svg
the above image im able to scrole using the apple mighty mouse but olso doss not display scroll bars,

[Mark Rowe (bdash)]

Confirmed with ToT.  I see no scroll bars in the SVG given in the URL.  The second URL given in the description, PowerStation3.svg now returns a 404 when loaded.  The first time I loaded the first URL, I couldn't make it scroll via my PowerBooks two-fingered scrolling.  After reloading, it scrolled fine.  I have not been able to make it revert to its unscrollable state.  The scrollbars are still not visible.

Thanks for the bug report KwisA!

[Eric Seidel (no email)]

This may be due to the svg.css file and its use of svg { overflow: hidden !important; }  We'd have to check the spec.  This might be "intentional".

[Mark Rowe (bdash)]

It also happens with http://bdash.net.nz/files/webkit-commits-over-time.svg if you resize your browser window to be less than the SVG size (1024x768).  This one definitely has no overflow-related trickery.

[Darrik Spaude]

Vertical scrolling using the Mighty Mouse's scroll ball works, but horizontal scrolling is not possible. Without the scrollbars (vertical and horizontal) viewing larger images is not possible (especially on small displays). -- Mac Mini Core Solo 10.4.8 WebKit-SVN-r17534.

[Darin Adler]

I asked Maciej, and he said:

"I think it's actually required by the SVG spec that the root SVG element is overflow:hidden and authors can't override this. I think this is a silly rule but it might be too late to change it in the SVG spec."

[Eric Seidel (no email)]

Safari needs to either implement some sort of zoom/pan UI, or disregard the spec and allow overflow: auto on the outermost <svg>.  Otherwise there is no possible way for an SVG content author make it possible for the end user to navigate their content in Safari.

[Eric Seidel (no email)]

Ok, aside from the zoom/pan feature, this is actually a real bug.

We should be showing scrollbars when documents are larger than the window.  Example:
http://croczilla.com/svg/samples/chem2/chem2.xml

This should be relatively easy to fix, we just need to patch in to where the WebCore tells WebKit what scrollbars to show, and fix it for <svg> documents.

The question of how to correctly support zoom and pan comes later.  Sullivan suggested we just use scrolling for panning for now, I'm not sure how well that will work, but we can try.  That might again, require scroll-bar code hacks, depending on how it's implemented.

[Eric Seidel (no email)]

*** Bug 14737 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 14375 has been marked as a duplicate of this bug. ***

[Matt Lilek]

*** Bug 15957 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

*** Bug 16093 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

I'm not actively working on this.  reassigning to unassigned.

[Chris Whitney]

Another prominent example is http://en.wikipedia.org/wiki/Image:Unix_history-simple.svg

[Matthew Hutton]

^^ Fortunately in that case the diagram has been improved so you don't need to view the SVG to see the detail. But this bug does still need fixing.

[Jonathan Haas]

Created attachment 23447 [details]
proposed patch

This patch fixes the problem, as well as making sure that relatively-sized or -positioned child elements are relaidout when the SVG root size changes.

[Sam Weinig]

Comment on attachment 23447 [details]
proposed patch

+        Reviewed by Eric Seidel.
We tend not to put anything in here when submitting patches.


+        sizes or positions when an SVG document is scrolled. Added a layout test to
+        demonstrate the problem and the fix, and modified expected results of some layout
+        tests trivially impacted by this change.
This information is not really needed in the ChangeLog.  Instead, you should try and chronicle the specifics of what changed next to the files/functions that were changed below.

+        * ChangeLog:
No need to include this.
+>>>>>>> .r36446
This needs to be removed.

+        // Except in the case where the SVG element is the document; in that case we want to scroll if necessary
An element cannot be a document, so this comment seems wrong.  Perhaps you mean the svg element is the root element of a document.

+    bool relayoutChildren = selfNeedsLayout() || 
+        (m_width != oldWidth || m_height != oldHeight || m_viewport != oldViewport);
The parenthesis are not needed.

Can you explain why the changes to layout tests happened?  You note that it is trivial, but what caused it?

[Jonathan Haas]

Created attachment 23481 [details]
updated patch

> We tend not to put anything in here when submitting patches.

Done.

> An element cannot be a document, so this comment seems wrong.  Perhaps you mean
the svg element is the root element of a document.

Also done.

+    bool relayoutChildren = selfNeedsLayout() || 
+        (m_width != oldWidth || m_height != oldHeight || m_viewport !=
oldViewport);
> The parenthesis are not needed.

True, but I like them. They emphasize that there are two times when the children need to be relaid out: when the SVG node itself is flagged for relayout, or when the SVG node's size has changed. The parens group the conditions that indicate the latter staet.

> Can you explain why the changes to layout tests happened?  You note that it is
trivial, but what caused it?

Sure. In all four cases, dimensions changed because of the presence of scroll bars. In the case of path-bad-data.svg and focus-ring.svg, this is simple: the documents are bigger than the 800x600 test shell window, so scroll bars appear, so everything becomes one scrollbar-width narrower. In the case of foreignObject-crash-on-hover.svg and baseval-animval-equality.svg, it's a little more complicated. The short answer is: it's because of the presence of a <foreignObject>. Here's the longer answer if you're interested.

It appears that a new document gets laid out twice: once before rendering and once after, with the second layout intended to more accurately position things that are dependent on how other things are rendered. If you have the scrollbars set to anything other than "auto", this is no problem at all. If you have the scrollbars set to auto, the initial layout defaults to assuming the vertical scrollbar is visible and the horizontal scrollbar is not. Before I made the change to RenderSVGRoot, this played havoc with all the SVG layout tests: they'd get initially laid out assuming the presence of a vertical scrollbar (making the width 785px rather than 800px), but the second layout wouldn't catch the fact that the root's children needed to be relaid out, and everything stayed where it was after the initial layout. Making the SVG root relayout its children when its own size changes fixed that in most cases, except for these two, baseval-animval-equality and foreignObject-crash-on-hover.

Turns out that foreignObjects, when deciding to relayout their own contents, don't check their SVG parents but keep walking up the tree until they find a non-SVG node. Since this grandparent node does not need to be relaid out, the foreignObject node decides that it doesn't either. So everything stays where it was after the first layout with a width of 785px. This is probably a bug that should probably be fixed, but it's different from this bug and shouldn't hold off this fix.

[Eric Seidel (no email)]

Comment on attachment 23481 [details]
updated patch

+        if (!(e->hasTagName(SVGNames::svgTag) && e->parentNode() && e->parentNode()->isDocumentNode())) {


We have a function on SVGSVGElement called "isOutermostSVG" which handles the SVG in HTML case as well.  I think we should use that, and make it robust against !e->parentNode() if necessary.

So this would read then:
if (e->isSVG() && !static_cast<SVGSVGElement*>(e)->isOutermostSVG())

Why is the setTimeout(0) necessary?

+    function runTest() {
+        if (window.layoutTestController) {
+            layoutTestController.dumpAsText();
+            layoutTestController.waitUntilDone();
+        }
+        setTimeout("finishTest()", 0);
+    }

All files should have a newline at the end, or diff gets mad. :)  We wouldn't want to make diff mad...

+</svg>
\ No newline at end of file

Also, no need for this file to be executable:

Property changes on: LayoutTests/svg/custom/svg-doc-is-scrollable.svg
___________________________________________________________________
Name: svn:executable
   + *


In general this looks good.  I'm not sure if SVG inside HTML should have scrollbars.  Maybe not?  Maybe only when SVG is a root of a document?  What about the <img src="foo.svg"> case?  Seems we'll eventually need a way to turn this off in the <img> case.

I was talking with hyatt on #webkit just now, and he suggested hacking:
void FrameView::applyOverflowToViewport(RenderObject* o, ScrollbarMode& hMode, ScrollbarMode& vMode)

as an alternate way to fix this.  I'm not sure that helps with the <img> case though.

[Dave Hyatt]

Try adding this to SVG's css file

replace

svg { overflow: hidden }

with

svg:not(:root) { overflow: hidden }

Note that WebKit not showing scrollbars is actually correct according to the SVG and CSS specs, so we need to raise this issue with the SVG and CSS WGs.

[Dave Hyatt]

You'll just need to hack the SVG-as-image case to never have srollbars manually yeah.

[Dave Hyatt]

I sent mail to the SVG and CSS WG about the fact that the specs make our current behavior correct.

[Jonathan Haas]

(In reply to comment #18)
> We have a function on SVGSVGElement called "isOutermostSVG" which handles the
> SVG in HTML case as well.  I think we should use that, and make it robust
> against !e->parentNode() if necessary.

The way I did it exactly parallels the way the same test is done a few lines down. If we change one, we should change the other.

> Why is the setTimeout(0) necessary?

Because the value of window.scrollbars.visible isn't updated until after the onload function is called. Without the setTimeout(), this test passes without the patch.

> All files should have a newline at the end, or diff gets mad. :)  We wouldn't
> want to make diff mad...

Duly noted.

> Also, no need for this file to be executable:

I didn't tell SVN to do that, I wonder why it did?

> In general this looks good.  I'm not sure if SVG inside HTML should have
> scrollbars.  Maybe not?

Definitely not. 

> Maybe only when SVG is a root of a document?  What
> about the <img src="foo.svg"> case?  Seems we'll eventually need a way to turn
> this off in the <img> case.

Why? The <img> case works fine, the <img> case works even without the patch. In that case, the <img> element is as large as the full extent of the SVG element (or not, depending on its own style) and it's the HTML document that the scrollbars control, not the SVG document.

> I was talking with hyatt on #webkit just now, and he suggested hacking:
> void FrameView::applyOverflowToViewport(RenderObject* o, ScrollbarMode& hMode,
> ScrollbarMode& vMode)

Hmm. I don't care for that approach. We're special-casing a style for SVG elements, and it seems that the appropriate place to put that would be in the style selector, which is where the existing special-casing already was.

[Dave Hyatt]

Comment on attachment 23481 [details]
updated patch

I don't think you can hack the overflow value in adjustRenderStyle.  The author should be able to control it.

(Also !important in a UA sheet with svg:not(:root) would be sufficient to do this anyway without having to hack CSSStyleSelector.cpp.)

[Dave Hyatt]

Yeah ignore the applyOverflowToViewport suggestion.   It would violate the CSS spec.

I think we should just sit on this bug for a bit until we hear back from the SVG/CSS WGs regarding this issue.

Our current behavior, although obviously wrong, matches the specs, so we need to figure out how the working groups are going to address the issue.

[Dave Hyatt]

It does seem like:

svg:not(:root) {
  overflow:hidden
}

would fix the bug (aside from needing to hack the <img> case).

[Jonathan Haas]

(In reply to comment #19)
> Try adding this to SVG's css file
> replace
> svg { overflow: hidden }
> with
> svg:not(:root) { overflow: hidden }

We already have an svg:root selector, which is flagged as important. Making sure the overflow: hidden is not applied to the SVG root element is insufficient to fix this bug, considering the coercion that goes on in CSSStyleSelector::adjustRenderStyle().


> Note that WebKit not showing scrollbars is actually correct according to the
> SVG and CSS specs, so we need to raise this issue with the SVG and CSS WGs.

I disagree. Consider the following:

<svg xmlns:"blahblahblah" width="2000" height="2000">

The spec is pretty vague, but my interpretation of it in light of sections 7.1, 7.2, and 14.3.3 is that this code establishes a viewport with dimensions 2000x2000. Clipping should be done to that viewport, not to the dimensions of the rendering area provided by the user agent. 

[Dave Hyatt]

The specs are not vague about this.

See:

http://www.w3.org/TR/SVG11/masking.html#OverflowProperty

"The initial value for 'overflow' as defined in [CSS2-overflow] is 'visible'; however, SVG's user agent style sheet overrides this initial value and set the 'overflow' property on elements that establish new viewports (e.g., 'svg' elements), 'pattern' elements and 'marker' elements to the value 'hidden'."

Therefore the SVG spec is saying that

svg { overflow: hidden }

should be in the UA sheet.

CSS says:

http://www.w3.org/TR/CSS21/visufx.html#overflow

"UAs must apply the 'overflow' property set on the root element to the viewport. HTML UAs must instead apply the 'overflow' property from the BODY element to the viewport, if the value on the HTML element is 'visible'. The 'visible' value when used for the viewport must be interpreted as 'auto'. The element from which the value is propagated must have a used value for 'overflow' of 'visible'."

Therefore the overflow:hidden on SVG root elements is applied to the viewport and you end up not getting scrollbars.

The specs are obviously wrong, but nevertheless that's what they say right now.

[Dave Hyatt]

Robert from Mozilla responded to my suggestion to use svg:not(:root)....

"On Thu, Sep 18, 2008 at 10:00 AM, David Hyatt <[e-mail]> wrote:
It seems like SVG should possibly amend its UA sheet to:

svg:not(:root) { overflow: hidden }

What do you think?
 
I agree. That's actually what we have in our UA sheet.

Rob"

[Maciej Stachowiak]

(In reply to comment #27)

> The specs are obviously wrong, but nevertheless that's what they say right now.
> 

To be fair though, the wrongness is deliberate.

[Dave Hyatt]

This is being discussed on the mailing lists now.

Opera and Mozilla have both stated that they use:

svg:not(:root) { overflow: hidden }

to allow scrollbars to show up on the outermost SVG, so I think we should adopt that solution as well.

[Dave Hyatt]

SVG spec is having

svg:not:(:root) { overflow: hidden }

added to the errata, so it's official.  That's the path to take to fix the bug.

Thanks to the SVG WG for a quick response on this issue!

[Eric Seidel (no email)]

*** Bug 20486 has been marked as a duplicate of this bug. ***

[Jon@Chromium]

This has also been reported against Chromium, see http://code.google.com/p/chromium/issues/detail?id=33

[Brady Eidson]

<rdar://problem/6508733>

[Eric Seidel (no email)]

I tried the naive fix:
diff --git a/WebCore/css/svg.css b/WebCore/css/svg.css
index 322eda8..49e0ae4 100644
--- a/WebCore/css/svg.css
+++ b/WebCore/css/svg.css
@@ -32,8 +32,13 @@
     which does not use CSS layout [CSS2-LAYOUT] or XSL formatting [XSL], the 'overflow' property on the 
     outermost 'svg' element is ignored for the purposes of visual rendering and the initial clipping path is set 
     to the bounds of the initial viewport.
+    SVG 1.1 Errata will supposedly change this rule to only apply to :not(:root)
+    See discussion in https://bugs.webkit.org/show_bug.cgi?id=11225
 */
 svg:root {
+    overflow: auto
+}
+svg:not:(:root) {
     overflow: hidden !important
 }


But got lots of failures I didn't expect. :(  Unfortunately, I have other bugs to fix atm, so this one will have to wait...

[Eric Seidel (no email)]

*** Bug 22669 has been marked as a duplicate of this bug. ***

[Benjamin Kramer]

(In reply to comment #35)
> +svg:not:(:root) {
          ^

I think you've got a typo here.

[Nikolas Zimmermann]

I am working on this bug - fixed it in my tree, still needs a cleanup. Will take some days...

[Nikolas Zimmermann]

Created attachment 48415 [details]
New approach

After DRT has been fixed to dump precise repaint rects we can finally fix this bug in a sane way, w/o affecting hundreds of test results.
Thanks to Daves question on the SVG WG, it's now clear how overflow should be handled. Adapt to the SVG 1.1 errata changes and finally fix this bug :-)

[Nikolas Zimmermann]

*** Bug 24448 has been marked as a duplicate of this bug. ***

[Nikolas Zimmermann]

*** Bug 24033 has been marked as a duplicate of this bug. ***

[Dirk Schulze]

Comment on attachment 48415 [details]
New approach

r=me

[Nikolas Zimmermann]

Landed in r54551. Thanks for the extra quick review :-)

*** Bug 31510 has been marked as a duplicate of this bug. ***