Bug 11225 - No scroll bars are displayed for standalone SVG image
: No scroll bars are displayed for standalone SVG image
Status: RESOLVED FIXED
: WebKit
SVG
: 420+
: Macintosh Intel Mac OS X 10.4
: P2 Normal
Assigned To:
: http://upload.wikimedia.org/wikipedia...
: InRadar, SVGHitList
:
:
  Show dependency treegraph
 
Reported: 2006-10-09 04:56 PST by
Modified: 2010-02-09 11:12 PST (History)


Attachments
proposed patch (13.98 KB, patch)
2008-09-15 14:24 PST, Jonathan Haas
no flags Review Patch | Details | Formatted Diff | Diff
updated patch (13.58 KB, patch)
2008-09-16 12:59 PST, Jonathan Haas
hyatt: review-
Review Patch | Details | Formatted Diff | Diff
New approach (334.38 KB, patch)
2010-02-09 07:53 PST, Nikolas Zimmermann
krit: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2006-10-09 04:56:36 PST
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 From 2006-10-09 22:58:43 PST -------
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 From 2006-10-10 11:50:38 PST -------
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 From 2006-10-10 12:11:05 PST -------
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 From 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 From 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 From 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 From 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 From 2007-09-24 08:11:34 PST -------
*** Bug 14737 has been marked as a duplicate of this bug. ***
------- Comment #9 From 2007-09-26 07:47:09 PST -------
*** Bug 14375 has been marked as a duplicate of this bug. ***
------- Comment #10 From 2007-11-12 11:48:46 PST -------
*** Bug 15957 has been marked as a duplicate of this bug. ***
------- Comment #11 From 2007-11-22 16:32:44 PST -------
*** Bug 16093 has been marked as a duplicate of this bug. ***
------- Comment #12 From 2008-01-13 14:51:50 PST -------
I'm not actively working on this.  reassigning to unassigned.
------- Comment #13 From 2008-03-24 13:23:14 PST -------
Another prominent example is http://en.wikipedia.org/wiki/Image:Unix_history-simple.svg
------- Comment #14 From 2008-07-03 02:10:43 PST -------
^^ 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 From 2008-09-15 14:24:20 PST -------
Created an attachment (id=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 From 2008-09-15 22:53:30 PST -------
(From update of attachment 23447 [details])
+        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 From 2008-09-16 12:59:34 PST -------
Created an attachment (id=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 From 2008-09-17 14:34:26 PST -------
(From update of attachment 23481 [details])
+        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 From 2008-09-17 14:51:55 PST -------
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 From 2008-09-17 14:53:49 PST -------
You'll just need to hack the SVG-as-image case to never have srollbars manually yeah.
------- Comment #21 From 2008-09-17 15:04:40 PST -------
I sent mail to the SVG and CSS WG about the fact that the specs make our current behavior correct.
------- Comment #22 From 2008-09-17 15:52:13 PST -------
(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 From 2008-09-17 15:59:54 PST -------
(From update of attachment 23481 [details])
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 From 2008-09-17 16:01:28 PST -------
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 From 2008-09-17 16:02:20 PST -------
It does seem like:

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

would fix the bug (aside from needing to hack the <img> case).
------- Comment #26 From 2008-09-17 16:05:14 PST -------
(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 From 2008-09-17 16:17:02 PST -------
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 From 2008-09-17 22:35:56 PST -------
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 From 2008-09-18 01:16:29 PST -------
(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 From 2008-09-22 06:52:45 PST -------
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 From 2008-09-23 13:33:29 PST -------
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 From 2008-10-03 23:55:59 PST -------
*** Bug 20486 has been marked as a duplicate of this bug. ***
------- Comment #33 From 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 From 2009-01-19 17:17:08 PST -------
<rdar://problem/6508733>
------- Comment #35 From 2009-03-16 14:50:07 PST -------
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 From 2009-04-28 17:03:56 PST -------
*** Bug 22669 has been marked as a duplicate of this bug. ***
------- Comment #37 From 2009-06-21 14:26:46 PST -------
(In reply to comment #35)
> +svg:not:(:root) {
          ^

I think you've got a typo here.
------- Comment #38 From 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 From 2010-02-09 07:53:19 PST -------
Created an attachment (id=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 From 2010-02-09 07:53:40 PST -------
*** Bug 24448 has been marked as a duplicate of this bug. ***
------- Comment #41 From 2010-02-09 07:53:48 PST -------
*** Bug 24033 has been marked as a duplicate of this bug. ***
------- Comment #42 From 2010-02-09 08:07:07 PST -------
(From update of attachment 48415 [details])
r=me
------- Comment #43 From 2010-02-09 08:09:18 PST -------
Landed in r54551. Thanks for the extra quick review :-)
------- Comment #44 From 2010-02-09 11:12:21 PST -------
*** Bug 31510 has been marked as a duplicate of this bug. ***