[webkit-reviews] review cancelled: [Bug 11225] No scroll bars are displayed for standalone SVG image : [Attachment 23447] proposed patch

[bugzilla-daemon at webkit.org]

Jonathan Haas <jhaas at chromium.org> has cancelled Jonathan Haas
<jhaas at chromium.org>'s request for review:
Bug 11225: No scroll bars are displayed for standalone SVG image
https://bugs.webkit.org/show_bug.cgi?id=11225

Attachment 23447: proposed patch
https://bugs.webkit.org/attachment.cgi?id=23447&action=edit

------- Additional Comments from Jonathan Haas <jhaas at chromium.org>
> 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.