Bug 10526

Created attachment 10180 [details] demonstrates embedded svg object doesnt scale right

Created attachment 10181 [details] better demonstration

I'm not sure I understand what you're showing here.

well just keep zooming, zoom works up until the circle touches the edge of the box then the box grows but not the circle. some kind of weird behaviour but it cant be right.

MacDome That's brilliant... another bug as there doesn't appear to be a visual interface for zooming.... eg view - zoom or ctrl - click - zoom amazing! apple + zooms so this potentially could be a really nasty bug: please compare the attachment behaviour with http://www.peepo.co.uk and http://www.peepo.com and http://www.eas-i.co.uk/ the lack of consistency is remarkable :-( what to say, sorry... should the summary be widened to SVG doesn't scale right?

apple + is labelled as Make Text Bigger under View but presumably this might be altered in webkit?

Is it possible to construct an XML/SVG document which shows HTML and SVG scaling side-by-side and showing the SVG scaling incorrectly? I'm still not quite sure what's wrong here, but maybe I just haven't looked hard enough.

#7 Eric, to be blunt this is pretty obvious stuff, you already have an html and svg test in the attachment above did you repeatedly hit the apple + buttons? unless you have an extreme setting after about 2 tries the circle stops growing thought the font size keeps increasing. note the bounding box grows. failing all else try the above attachment in Opera to see one way...

Created attachment 11453 [details] Comparing intrinsic height in SVG and PNG @#7 Eric I've uploaded a new test case which demonstrates the different way that <html:object> handles the intrinsic height of SVG and raster graphics. Both images have an intrinsic height and width of 100px, but these values should be replaced by the computed width and height of <html:object>.

#9 Ryan, well what a can of worms... is this really all one bug?

Created attachment 11461 [details] Re-illustrating the second testcase I'm not sure what's happening in the first case is a bug. When WebKit loads the SVG image, WebKit detects no intrinsic height, so it calculates the height of the box based on CSS within the page and the default width of an object element (300px). Since the height is a relative value and the width is an absolute value, as the font-size increases the object's computed height increases, as does the size of the circle. However, once the object's computed height becomes greater than the object's computed width, the circle stops scaling. The issue the user is having is an arbitrary calculation of <html:object>'s default width. I'm not sure this is actually a bug because the SVG image does not have an intrinsic width, and as such WebKit has to apply something—if you set the intrinsic width of the <object> element to 13em, the circle scales as expected. The second case, however, is a bug with the scaling of embeded SVG, similar to <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=288276">bug 288276</a> in Mozilla. Opera is the only native SVG browser to get this right. The bug is that the width and height attributes on <svg:svg> should be treated the same was as the width and height of a raster image. Perhaps this bug's title needs to be adjusted.

I'm still confused. This bug is unfortunately seems to be being ignored due to being 1. unconfirmed and 2. confusing. http://bugs.webkit.org/attachment.cgi?id=11453 looks to be correct. http://bugs.webkit.org/attachment.cgi?id=11461 also looks to be correct. (even when increasing/decreasing the font scaling) In response to the final comment: The second case, however, is a bug with the scaling of embeded SVG, similar to <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=288276">bug 288276</a> in Mozilla. Opera is the only native SVG browser to get this right. The bug is that the width and height attributes on <svg:svg> should be treated the same was as the width and height of a raster image. It's true that the width/height on an svg should be treated as the intrinsic size when the SVG is referenced by an <img> tag (although this is not written down anywhere, it's just the consensus hyatt, tor and I came to a while back). I'm not sure about an <object> tag however. Hyatt would have to comment on that. So is your complaint that the width/height on the SVG is not treated as the intrinsic size for an <object> tag? Thus overriding any default width/height that an <object> tag w/o specified dimensions or content with intrinsic size should use. Am i understanding that correctly?

Ok. I finally understand. The bug you're pointing out is that for PNGs, <object> tags scale the PNG to fit in the <object> tag, where as for SVGs <object> tags merely define the size of the drawing area for the SVG and the SVG is drawn w/o any scaling. I agree, that's probably wrong. I bet some CDF specification somewhere has defined what we're actually supposed to do here. In fact, there is. I think you're asking for us to support this: http://www.w3.org/TR/WICD/#scalable-child-layout

A note to implementors: - This will require HTMLObjectElement (or RenderReplaced) to detect that the embedded type is an SVGDocument (or possibly more generally "scalable" content (i.e. a PDF), and adjust the layout behavior accordingly. - We already do "is this an SVG" detection in getSVGDocument() (sorta) The current WebKit behavior is to treat embedded SVG files the same as HTML or XHTML files would be treated. Simply giving them a width and allowing them to adjust their hight accordingly. Sorta "xMinYMin slice" in SVG vernacular, however with the special case of never allowing the X direction to scale beyond the viewport (and no flow behavior like HTML, only whatever fixed width/height was specified in the SVG).

Here is another test case which demonstrates the problem clearly. http://clientside.net.au/code/safari/svg/ This really sucks as there appears no way to include SVG images in an HTML page and have them scale correctly with the HTML content. I could'nt find a work around for it. Tested with WebKit r25353 on Mac OS X 10.4.10 ; Firefox 2.0.0.6 (Mac) and Opera 9.22 (Mac). Both Firefox and Opera behave as expected.

The feature-branch of WebKit contains numerous fixes to SVG. This may already be fixed there: http://lists.macosforge.org/pipermail/webkit-dev/2007-May/001803.html http://trac.webkit.org/projects/webkit/browser/branches/feature-branch http://trac.webkit.org/projects/webkit/log/branches/feature-branch Unfortunately, there is no "nightly" build of this branch, so it's difficult to test if you're not comfortable compiling your own copy of WebKit to test. You might try the #webkit IRC channel on irc.freenode.net to see if any of the SVG hackers can test this for you.

You can test the feature branch with: http://nightly.webkit.org/builds/overview/feature-branch

If I'm interpreting the test cases correctly, this issue is still present in ToT.

Version 3.2.1 (5525.27.1) ppc 10.5 yep AP looks pretty ugly on zoom, viewing box: width is not growing at all height is not growing at a proportionate rate to content. mozilla have now fixed this bug, or try Opera either have attractive zoom options mozilla offer text only zoom as well...

it appears this bug may be a crash bug, as three times the application fell over after opening the attachment. However this may be hard to reproduce... hence have not change status http://www.peepo.com and http://www.peepo.co.uk zoom on both sites, one is pure svg the other embedded resize windows now try in opera and mozilla plenty to cut ones teeth on!

I have a fix for embedded <embed> and <object> scaling. Preparing a patch.

Created attachment 95159 [details] Patch Full support of CSS 2.1 inline replaced width/height, computing intrinsic ratio, width/height for embedded SVG documents. This fixes this bug, and dozens of others, including the WICD Core 1.0 'rightsizing' tests. The code changes are small, though the patch is large, because I added dozens of new testcases, including pixel test results.

Created attachment 95161 [details] Patch v2 The patch doesn't seem to apply, try marking svg/in-html/by-reference-expected.png as binary, before creating the patch...

Comment on attachment 95161 [details] Patch v2 Attachment 95161 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8747051

Comment on attachment 95161 [details] Patch v2 Attachment 95161 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8739501

Created attachment 95165 [details] Patch v3 Remove unused variable, fix build.

Comment on attachment 95165 [details] Patch v3 Attachment 95165 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8747056 New failing tests: svg/zoom/page/zoom-foreignObject.svg svg/zoom/page/zoom-svg-through-object-with-override-size.html svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml svg/custom/object-sizing-explicit-height.xhtml svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml svg/custom/object-sizing-explicit-width.xhtml svg/zoom/page/zoom-svg-through-object-with-text.xhtml svg/custom/immutable-properties.html svg/custom/object-sizing-no-width-height.xhtml http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml svg/custom/object-sizing-explicit-width-height.xhtml svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml svg/zoom/page/zoom-coords-viewattr-01-b.svg svg/custom/object-sizing.xhtml svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml svg/custom/object-no-size-attributes.xhtml

Created attachment 95166 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Adam might be interested in seeing this go by (from a layout perspective) since he toyed with seemless iframes, which will have to do similar content size detection and displayed size adjustment. Not directly related, but in the same area.

Comment on attachment 95161 [details] Patch v2 Attachment 95161 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8748019

Comment on attachment 95165 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=95165&action=review This is really a great area of SVG to fix! r- because of some minor things, also I need to get the overall picture before r+. > Source/WebCore/rendering/RenderPart.cpp:151 > + if (widthIsAuto) { Could move the above bools (apart from widthIsAuto obviously) into the block. > Source/WebCore/rendering/RenderPart.cpp:172 > + if (heightIsAuto && !hasIntrinsicWidth && !hasIntrinsicHeight && containingBlock) Isn't heightIsAuto && hasIntrinsicWidth already tested above? > Source/WebCore/rendering/RenderPart.cpp:199 > + if (heightIsAuto) { Could move the above bools (apart from heightIsAuto obviously) into the block. > Source/WebCore/rendering/RenderPart.cpp:210 > + return logicalHeight; Could omit the variable, but I can see it is clearer to use it. > Source/WebCore/rendering/RenderReplaced.h:35 > + virtual void computePreferredLogicalWidths(); Should no be needed to make public. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:87 > + if (svg->useCurrentView()) { Getting a viewBox from the svg could be put into SVGSVGElement helper function ; internally we already have that code.

(In reply to comment #31) > (From update of attachment 95165 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95165&action=review > > This is really a great area of SVG to fix! r- because of some minor things, also I need to get the overall picture before r+. > > > Source/WebCore/rendering/RenderPart.cpp:151 > > + if (widthIsAuto) { > > Could move the above bools (apart from widthIsAuto obviously) into the block. Fixed. > > > Source/WebCore/rendering/RenderPart.cpp:172 > > + if (heightIsAuto && !hasIntrinsicWidth && !hasIntrinsicHeight && containingBlock) > > Isn't heightIsAuto && hasIntrinsicWidth already tested above? Yeah, but this is heightIsAuto && ! hasIntrinsicWidth? > > > Source/WebCore/rendering/RenderPart.cpp:199 > > + if (heightIsAuto) { > > Could move the above bools (apart from heightIsAuto obviously) into the block. Fixed. > > > Source/WebCore/rendering/RenderPart.cpp:210 > > + return logicalHeight; > > Could omit the variable, but I can see it is clearer to use it. Heh debuggin leftover, where I had a fprintf before that line... fixed. > > > Source/WebCore/rendering/RenderReplaced.h:35 > > + virtual void computePreferredLogicalWidths(); > > Should no be needed to make public. Right. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:87 > > + if (svg->useCurrentView()) { > > Getting a viewBox from the svg could be put into SVGSVGElement helper function ; internally we already have that code. Ouch yeah, I forgot to refactor that part, will fix.

Created attachment 95174 [details] Patch v4 Fix Robs comments.

Comment on attachment 95174 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=95174&action=review I trust you enough to fix my remarks before landing, no new iteration needed. You could consider adding some copyrights since you touch most of the time more than 10 lines of code. r=me > Source/WebCore/rendering/RenderPart.h:62 > + mutable bool m_preferredWidthsDependOnContent : 1; Not used? > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:29 > +#include "FrameTree.h" Are you sure this include is needed here? > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:188 > + return computeIntrinsicWidth(replacedWidth); You may want to skip the early return style here, as two code paths at the start of the method end up with the same return statement. > LayoutTests/svg/wicd/resources/test-svg-child-object-rightsizing.svg:2340 > +<hkern g1="Ccaron" g2="Scedilla" k="0" /> I doubt all this stuff is needed for the test :) Please strip it down as much as you can before landing.

Comment on attachment 95174 [details] Patch v4 Attachment 95174 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8747103 New failing tests: svg/zoom/page/zoom-foreignObject.svg svg/zoom/page/zoom-svg-through-object-with-override-size.html svg/zoom/page/zoom-svg-through-object-with-percentage-size.xhtml svg/custom/object-sizing-explicit-height.xhtml svg/zoom/page/zoom-svg-through-object-with-absolute-size.xhtml svg/custom/object-sizing-explicit-width.xhtml svg/zoom/page/zoom-svg-through-object-with-text.xhtml svg/custom/immutable-properties.html svg/custom/object-sizing-no-width-height.xhtml http/tests/misc/object-embedding-svg-delayed-size-negotiation.xhtml svg/custom/object-sizing-explicit-width-height.xhtml svg/zoom/page/zoom-svg-through-object-with-huge-size.xhtml svg/zoom/page/zoom-coords-viewattr-01-b.svg svg/custom/object-sizing.xhtml svg/zoom/page/zoom-svg-through-object-with-absolute-size-2.xhtml svg/custom/object-no-size-attributes.xhtml

Created attachment 95185 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #34) > (From update of attachment 95174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95174&action=review > > I trust you enough to fix my remarks before landing, no new iteration needed. You could consider adding some copyrights since you touch most of the time more than 10 lines of code. r=me Thanks a lot for the quick review. > > > Source/WebCore/rendering/RenderPart.h:62 > > + mutable bool m_preferredWidthsDependOnContent : 1; > > Not used? Oops, removed. > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:29 > > +#include "FrameTree.h" > > Are you sure this include is needed here? Not anymore. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:188 > > + return computeIntrinsicWidth(replacedWidth); > > You may want to skip the early return style here, as two code paths at the start of the method end up with the same return statement. Right, fixed. > > > LayoutTests/svg/wicd/resources/test-svg-child-object-rightsizing.svg:2340 > > +<hkern g1="Ccaron" g2="Scedilla" k="0" /> > > I doubt all this stuff is needed for the test :) Please strip it down as much as you can before landing. Stripped down everything except the S, V, G glyphs. Landed in r87526. Let's wait for the build bots...

Landed a follow-up crash fix for svg/custom/immutable-properties.html in r87534.

Landed mac-leopard/qt/win rebaselines in r87535. Closing this bug now, as it should be done.

Had to fix one wrong win result in r87545. Hopefully we're done now.

On Chromium, I'm seeing a bunch of failures where scrollbars are added around the embedded object: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size-2.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-huge-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-override-size.html%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-percentage-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-text.xhtml&showExpectations=true&group=%40ToT%20-%20chromium.org Is this expected?

(In reply to comment #41) > On Chromium, I'm seeing a bunch of failures where scrollbars are added around the embedded object: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size-2.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-huge-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-override-size.html%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-percentage-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-text.xhtml&showExpectations=true&group=%40ToT%20-%20chromium.org > > Is this expected? And another failure, this one not related to scrollbars: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fhixie%2Ftext%2F003.html&group=%40ToT%20-%20chromium.org Having spent much of the day chasing down rebaselines after this change, I'm inclined to think there might be something wrong with it.

(In reply to comment #42) > (In reply to comment #41) > > On Chromium, I'm seeing a bunch of failures where scrollbars are added around the embedded object: > > > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size-2.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-huge-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-override-size.html%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-percentage-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-text.xhtml&showExpectations=true&group=%40ToT%20-%20chromium.org > > > > Is this expected? > > And another failure, this one not related to scrollbars: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=svg%2Fhixie%2Ftext%2F003.html&group=%40ToT%20-%20chromium.org > > Having spent much of the day chasing down rebaselines after this change, I'm inclined to think there might be something wrong with it. Spoke too soon at that last one, looks like the one weird failure was flakiness.

(In reply to comment #41) > On Chromium, I'm seeing a bunch of failures where scrollbars are added around the embedded object: > > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=svg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size-2.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-absolute-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-huge-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-override-size.html%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-percentage-size.xhtml%2Csvg%2Fzoom%2Fpage%2Fzoom-svg-through-object-with-text.xhtml&showExpectations=true&group=%40ToT%20-%20chromium.org > > Is this expected? Hi Adam, thanks for the notice. This is definately not expected, and it wonders me a lot. The content looks correctly scaled, so there's no need at all for the scrollbars. Do you think you could find a chromium guy who could debug this? I don't have a chromium environment to test, and this is really important...

svg/zoom/page/zoom-svg-through-object-with-text.xhtml is still failing on Mac: <http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r87627%20(29838)/svg/zoom/page/zoom-svg-through-object-with-text-pretty-diff.html>.

(In reply to comment #45) > svg/zoom/page/zoom-svg-through-object-with-text.xhtml is still failing on Mac: <http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r87627%20(29838)/svg/zoom/page/zoom-svg-through-object-with-text-pretty-diff.html>. Yeah, I just added this testcase to show that SVG text inside objects are still broken, when scaling, see the inline comment in the file: <!-- FIXME: This is broken, text doesn't correctly zoom, its font size remains the same upon scaling --> I'll take care of fixing this soon. Thanks Alexey for pointing this out, I obviously forgot to mention it in the ChangeLog.

Comment on attachment 95174 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=95174&action=review No offense Rob, but are you sure you know this code well enough to review this? I don't feel like I do. I would have wanted to loop simon or hyatt to make sure the width/height calcs make sense with the rest of the rendering tree. Alone the code is probably fine, but how much are we duplicating with other logic in the rendering tree which we should be re-using or augmenting here? Some of this is SVG specific, sure, but the general idea of having a frame auto-negotiate with its parent for sizing seems useful (and certainly necessary for seemless iframes, which abarth played with a little a couple weeks back). In general this seems OK. My only real questions are with regards to the integartion with the rest of the rendering tree. It's unclear if all of these tests needed to be pixel tests, but I think you just imported most of them. > Source/WebCore/platform/Length.h:176 > + bool isSpecified() const { return type() == Fixed || type() == Percent; } So this is just the opposite of isIntrinsicOrAuto? > Source/WebCore/rendering/RenderPart.cpp:99 > + if (!node() || !widget() || !widget()->isFrameView()) When can a RenderPart be anonymous? Does it not have a widget during teardown? > Source/WebCore/rendering/RenderPart.cpp:111 > + return toRenderSVGRoot(svgRootRenderer); I'm surprised we don't share more with getSVGDocument(). They seem very similar. > Source/WebCore/rendering/RenderPart.cpp:118 > + int maxLogicalWidth = !includeMaxWidth || contentRenderStyle->logicalMaxWidth().isUndefined() ? logicalWidth : computeReplacedLogicalWidthUsing(contentRenderStyle->logicalMaxWidth()); ? : has strange precedence. Are we sure that it isn't higher priority than ||. We've been bitten by similar in the past. > Source/WebCore/rendering/RenderPart.cpp:120 > + int width = max(minLogicalWidth, min(logicalWidth, maxLogicalWidth)); > + return width; I wouldn't have bothered with a local here. > Source/WebCore/rendering/RenderPart.cpp:127 > + int maxLogicalHeight = contentRenderStyle->logicalMaxHeight().isUndefined() ? logicalHeight : computeReplacedLogicalHeightUsing(contentRenderStyle->logicalMaxHeight()); Again, same precedence concern. > Source/WebCore/rendering/RenderPart.cpp:129 > + int height = max(minLogicalHeight, min(logicalHeight, maxLogicalHeight)); > + return height; Again, no local. > Source/WebCore/rendering/RenderPart.cpp:141 > + if (style()->width().isAuto()) { I would have probably made this an early return for the !isAuto case. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:69 > + // SVG from an âobjectâ element in HTML styled with CSS. It is possible (indeed, common) for an SVG Encoding troubles with " "? > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:129 > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; I don't think we can have an annoymous root node either, but caution doesn't really hurt. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:133 > + // If our frame has an owner renderer, we're embedded through eg. object/embed. I'm not sure what "embedded through" means. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:148 > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; Seems like we might want a frame helper, since we do this a few times. I'm surprised one doesn't already exist for RenderObject or RenderBoxModelObject. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:165 > + // or provide hints about the dimensions of the viewport for the SVG content. SVG content itself optionally can provide I'm not sure "hints" is what you mean. I expect there is an explicit algorithm for computing the height/width based on the information provided. :) But the comment is fine too. > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:184 > + int maxLogicalWidth = !includeMaxWidth || ownerRendererStyle->logicalMaxWidth().isUndefined() ? logicalWidth : ownerRenderer->computeReplacedLogicalWidthUsing(ownerRendererStyle->logicalMaxWidth()); Again, possible ? : ordering trouble. I'll have to check the c++ spec to rest my paranoia here. :) > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:195 > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; Again, frame accessor would have been helpful, or in this case even an ownerRenderer(). > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:236 > + negotiateSizeWithHostDocumentIfNeeded(); I'm surprised this logic goes on SVGRoot. I would think CDF or WICD or whatever spec covers this to be more general. But I guess this is just SVG specific for now? > Source/WebCore/svg/SVGSVGElement.cpp:203 > + if (!inDocument()) > + return 1; We still have a frame, even if we're not in a document, right? This is a change in behavior, or did the old code crash? > Source/WebCore/svg/SVGSVGElement.cpp:237 > + // Calling setCurrentScale() on the outermost <svg> element in a standalone SVG document > + // is allowed to change the page zoom factor, influencing the document size, scrollbars etc. > + if (!hasFrameParent && isOutermostSVG()) { > + frame->setPageZoomFactor(scale); > + m_scale = 1; I assume we have test coverage for this?

Looks like my operator precedence concerns are unfounded: http://en.wikipedia.org/wiki/Operators_in_C_and_C%2B%2B#Operator_precedence My precedence fears came from this post: http://www.viva64.com/en/a/0074/ V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '-' operator. views custom_frame_view.cc 400 But now that I read the wikipedia guide, I think the post is wrong.

wfm, but can someone point me to, or advise why this is marked as fixed? ie whilst the patch appears subject to change.... Its not clear to me that I wont need to check for regression due to the 'final' patch. ie as a bug reporter, I only want to check once, when the bug is fixed, not to be involved in the arbitration, unless there is a problem with the testcase. anyway 5 years down the line it's great to see some of my bugs receiving attention ~:"

(In reply to comment #47) > (From update of attachment 95174 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95174&action=review > > No offense Rob, but are you sure you know this code well enough to review this? I don't feel like I do. I would have wanted to loop simon or hyatt to make sure the width/height calcs make sense with the rest of the rendering tree. They certainly do, if you compare to the other computeReplacedLogicalWidth implementations. There is some code to share though, between RenderPart/RenderReplaced/RenderBox variants of that method, that I plan to adress in a follow-up patch. >Alone the code is probably fine, but how much are we duplicating with other logic in the rendering tree which we should be re-using or augmenting here? Some of this is SVG specific, sure, but the general idea of having a frame auto-negotiate with its parent for sizing seems useful (and certainly necessary for seemless iframes, which abarth played with a little a couple weeks back). Well this is exactly the starting point for further generaliziation of the size negotiation code. The idea was to fix it for <object>/<iframe>/<embed> first that host SVG documents, then I want to generalize it to support SVGImages, and then we can think about further usecases... > > In general this seems OK. My only real questions are with regards to the integartion with the rest of the rendering tree. It's unclear if all of these tests needed to be pixel tests, but I think you just imported most of them. I hope the integration is well, at least that's how I understood how our rendering model works :-) > > > Source/WebCore/platform/Length.h:176 > > + bool isSpecified() const { return type() == Fixed || type() == Percent; } > > So this is just the opposite of isIntrinsicOrAuto? Well I refactored this method from RenderPart, to Length, to be able to reuse it. It's not related to isIntrinsicOrAuto at all: bool isIntrinsicOrAuto() const { return type() == Auto || type() == MinIntrinsic || type() == Intrinsic; } It isn't the opposite, that we're dealing with here. > > > Source/WebCore/rendering/RenderPart.cpp:99 > > + if (!node() || !widget() || !widget()->isFrameView()) > > When can a RenderPart be anonymous? Does it not have a widget during teardown? Easy answer: safety first. Other places do the widget()/node() check as well, see requiresAcceleratedCompositing(). > > Source/WebCore/rendering/RenderPart.cpp:111 > > + return toRenderSVGRoot(svgRootRenderer); > > I'm surprised we don't share more with getSVGDocument(). They seem very similar. I'm confused? > > > Source/WebCore/rendering/RenderPart.cpp:118 > > + int maxLogicalWidth = !includeMaxWidth || contentRenderStyle->logicalMaxWidth().isUndefined() ? logicalWidth : computeReplacedLogicalWidthUsing(contentRenderStyle->logicalMaxWidth()); > > ? : has strange precedence. Are we sure that it isn't higher priority than ||. We've been bitten by similar in the past. That code is a copied from RenderReplaced. As I said above, I'll unify all the users of exactly that code (there are 4 other places in rendering/ that do the same) in a follow-up patch. > > > Source/WebCore/rendering/RenderPart.cpp:120 > > + int width = max(minLogicalWidth, min(logicalWidth, maxLogicalWidth)); > > + return width; > > I wouldn't have bothered with a local here. I think I resolved that before landing? Oops, I didn't, thanks for noticing. I'll adress that as well soon. > > > Source/WebCore/rendering/RenderPart.cpp:127 > > + int maxLogicalHeight = contentRenderStyle->logicalMaxHeight().isUndefined() ? logicalHeight : computeReplacedLogicalHeightUsing(contentRenderStyle->logicalMaxHeight()); > > Again, same precedence concern. No worriess. > > > Source/WebCore/rendering/RenderPart.cpp:129 > > + int height = max(minLogicalHeight, min(logicalHeight, maxLogicalHeight)); > > + return height; > > Again, no local. Right, will fix. > > > Source/WebCore/rendering/RenderPart.cpp:141 > > + if (style()->width().isAuto()) { > > I would have probably made this an early return for the !isAuto case. Sure I could have done that, though that makes the spec comments harder to follow, no? > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:69 > > + // SVG from an âobjectâ element in HTML styled with CSS. It is possible (indeed, common) for an SVG > > Encoding troubles with " "? No the usual trouble with UTF8 and patches... Copy anything from the SVG spec containing quotes, place it in the ChangeLog, upload the patch, and this is how it looks like on bugs.webkit.org > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:129 > > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; > > I don't think we can have an annoymous root node either, but caution doesn't really hurt. I don't think it's possible, but I'm rather following safety first practices here. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:133 > > + // If our frame has an owner renderer, we're embedded through eg. object/embed. > > I'm not sure what "embedded through" means. <object data="foo.svg"/> then the frame belonging to the foo.svg document, has an ownerRenderer the RenderPart renderer for <object> and a contentRenderer(), its own RenderView. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:148 > > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; > > Seems like we might want a frame helper, since we do this a few times. I'm surprised one doesn't already exist for RenderObject or RenderBoxModelObject. Right, seems helpful. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:165 > > + // or provide hints about the dimensions of the viewport for the SVG content. SVG content itself optionally can provide > > I'm not sure "hints" is what you mean. I expect there is an explicit algorithm for computing the height/width based on the information provided. :) But the comment is fine too. Copied from the spec.. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:184 > > + int maxLogicalWidth = !includeMaxWidth || ownerRendererStyle->logicalMaxWidth().isUndefined() ? logicalWidth : ownerRenderer->computeReplacedLogicalWidthUsing(ownerRendererStyle->logicalMaxWidth()); > > Again, possible ? : ordering trouble. I'll have to check the c++ spec to rest my paranoia here. :) :-) > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:195 > > + Frame* frame = node() && node()->document() ? node()->document()->frame() : 0; > > Again, frame accessor would have been helpful, or in this case even an ownerRenderer(). Yop. > > > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:236 > > + negotiateSizeWithHostDocumentIfNeeded(); > > I'm surprised this logic goes on SVGRoot. I would think CDF or WICD or whatever spec covers this to be more general. But I guess this is just SVG specific for now? Exactly, given the complexity involved with gettin' sizing right for all kind of content, we have to start at some point: my starting point is only <object>/<embed>/<iframe> embedding SVG. That now works like a charme. > > > Source/WebCore/svg/SVGSVGElement.cpp:203 > > + if (!inDocument()) > > + return 1; > > We still have a frame, even if we're not in a document, right? This is a change in behavior, or did the old code crash? This is a change in behaviour, currentScale() behaviour is undefined in such cases, according to SVG. I choose to return the default, 1. Are you okay with the outcome? I'll prepare a patch that fixes the locals, and refactors some of the code to deduplicate...

(In reply to comment #50) > Are you okay with the outcome? I'll prepare a patch that fixes the locals, and refactors some of the code to deduplicate... I filed 52045 for the code deduplication, uploading a patch soon.

(In reply to comment #51) > (In reply to comment #50) > > Are you okay with the outcome? I'll prepare a patch that fixes the locals, and refactors some of the code to deduplicate... > > I filed 52045 for the code deduplication, uploading a patch soon. Erm, bug 61860 sorry.

Comment on attachment 95174 [details] Patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=95174&action=review > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:142 > + if (RenderPart* ownerRenderer = frame->ownerRenderer()) { > + ownerRenderer->setNeedsLayoutAndPrefWidthsRecalc(); > + m_didNegotiateSize = true; > + } You can't do this! A significant amount of layout code depends on layout within a FrameView only causing invalidations within that FrameView.