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?

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>.

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.

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!

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 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 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.

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.

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...

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?

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.