Bug 11225

Summary: No scroll bars are displayed for standalone SVG image
Product: WebKit Reporter: KwisA <kwisa>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: adrien, astrange, bennykra, emacemac7, ggaren, good4me, jasneet, jhaas, krit, matthew, mitz, mrowe, norm, oliver, peterlaurenspublic, philcolbourn, vsatayamas, zimmermann
Priority: P2 Keywords: InRadar, SVGHitList
Version: 420+   
Hardware: Mac (Intel)   
OS: OS X 10.4   
URL: http://upload.wikimedia.org/wikipedia/commons/7/72/Mexico_COA_large.svg
Attachments:
Description Flags
proposed patch
none
updated patch
hyatt: review-
New approach krit: review+

Description KwisA 2006-10-09 04:56:36 PDT
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,
Comment 1 Mark Rowe (bdash) 2006-10-09 22:58:43 PDT
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!
Comment 2 Eric Seidel (no email) 2006-10-10 11:50:38 PDT
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".
Comment 3 Mark Rowe (bdash) 2006-10-10 12:11:05 PDT
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.
Comment 4 Darrik Spaude 2006-11-02 08:55:02 PST
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.
Comment 5 Darin Adler 2006-12-15 15:03:11 PST
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."
Comment 6 Eric Seidel (no email) 2007-01-31 05:29:28 PST
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.
Comment 7 Eric Seidel (no email) 2007-02-02 08:57:06 PST
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.
Comment 8 Eric Seidel (no email) 2007-09-24 08:11:34 PDT
*** Bug 14737 has been marked as a duplicate of this bug. ***
Comment 9 Eric Seidel (no email) 2007-09-26 07:47:09 PDT
*** Bug 14375 has been marked as a duplicate of this bug. ***
Comment 10 Matt Lilek 2007-11-12 11:48:46 PST
*** Bug 15957 has been marked as a duplicate of this bug. ***
Comment 11 Mark Rowe (bdash) 2007-11-22 16:32:44 PST
*** Bug 16093 has been marked as a duplicate of this bug. ***
Comment 12 Eric Seidel (no email) 2008-01-13 14:51:50 PST
I'm not actively working on this.  reassigning to unassigned.
Comment 13 Chris Whitney 2008-03-24 13:23:14 PDT
Another prominent example is http://en.wikipedia.org/wiki/Image:Unix_history-simple.svg

Comment 14 Matthew Hutton 2008-07-03 02:10:43 PDT
^^ 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.
Comment 15 Jonathan Haas 2008-09-15 14:24:20 PDT
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.
Comment 16 Sam Weinig 2008-09-15 22:53:30 PDT
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?
Comment 17 Jonathan Haas 2008-09-16 12:59:34 PDT
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.
Comment 18 Eric Seidel (no email) 2008-09-17 14:34:26 PDT
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.
Comment 19 Dave Hyatt 2008-09-17 14:51:55 PDT
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.

Comment 20 Dave Hyatt 2008-09-17 14:53:49 PDT
You'll just need to hack the SVG-as-image case to never have srollbars manually yeah.

Comment 21 Dave Hyatt 2008-09-17 15:04:40 PDT
I sent mail to the SVG and CSS WG about the fact that the specs make our current behavior correct.

Comment 22 Jonathan Haas 2008-09-17 15:52:13 PDT
(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.
Comment 23 Dave Hyatt 2008-09-17 15:59:54 PDT
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.)
Comment 24 Dave Hyatt 2008-09-17 16:01:28 PDT
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.


Comment 25 Dave Hyatt 2008-09-17 16:02:20 PDT
It does seem like:

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

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

Comment 26 Jonathan Haas 2008-09-17 16:05:14 PDT
(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. 
Comment 27 Dave Hyatt 2008-09-17 16:17:02 PDT
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.

Comment 28 Dave Hyatt 2008-09-17 22:35:56 PDT
Robert from Mozilla responded to my suggestion to use svg:not(:root)....

"On Thu, Sep 18, 2008 at 10:00 AM, David Hyatt <hyatt@apple.com> 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"

Comment 29 Maciej Stachowiak 2008-09-18 01:16:29 PDT
(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.
Comment 30 Dave Hyatt 2008-09-22 06:52:45 PDT
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.

Comment 31 Dave Hyatt 2008-09-23 13:33:29 PDT
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!

Comment 32 Eric Seidel (no email) 2008-10-03 23:55:59 PDT
*** Bug 20486 has been marked as a duplicate of this bug. ***
Comment 33 Jon@Chromium 2008-11-21 15:32:22 PST
This has also been reported against Chromium, see http://code.google.com/p/chromium/issues/detail?id=33
Comment 34 Brady Eidson 2009-01-19 17:17:08 PST
<rdar://problem/6508733>
Comment 35 Eric Seidel (no email) 2009-03-16 14:50:07 PDT
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...
Comment 36 Eric Seidel (no email) 2009-04-28 17:03:56 PDT
*** Bug 22669 has been marked as a duplicate of this bug. ***
Comment 37 Benjamin Kramer 2009-06-21 14:26:46 PDT
(In reply to comment #35)
> +svg:not:(:root) {
          ^

I think you've got a typo here.
Comment 38 Nikolas Zimmermann 2010-02-02 19:30:34 PST
I am working on this bug - fixed it in my tree, still needs a cleanup. Will take some days...
Comment 39 Nikolas Zimmermann 2010-02-09 07:53:19 PST
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 :-)
Comment 40 Nikolas Zimmermann 2010-02-09 07:53:40 PST
*** Bug 24448 has been marked as a duplicate of this bug. ***
Comment 41 Nikolas Zimmermann 2010-02-09 07:53:48 PST
*** Bug 24033 has been marked as a duplicate of this bug. ***
Comment 42 Dirk Schulze 2010-02-09 08:07:07 PST
Comment on attachment 48415 [details]
New approach

r=me
Comment 43 Nikolas Zimmermann 2010-02-09 08:09:18 PST
Landed in r54551. Thanks for the extra quick review :-)
Comment 44 mitz 2010-02-09 11:12:21 PST
*** Bug 31510 has been marked as a duplicate of this bug. ***