Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2230963002:
SVG Image intrinsic size should be used if css style size is 'auto' (Closed)
|
Created:
4 years, 4 months ago by Shanmuga Pandi Modified:
4 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSVG Image intrinsic size should be used if css style size is 'auto'
SVG Image's width and height attributes will use intrinsic size,
if css style size is 'auto'
Spec:
https://svgwg.org/svg2-draft/embedded.html#Placement
https://svgwg.org/svg2-draft/geometry.html#Sizing
BUG=493681
Committed: https://crrev.com/3c1a3822848d986337573cfaf2817c8a69417791
Cr-Commit-Position: refs/heads/master@{#419133}
Patch Set 1 #Patch Set 2 : SVG intrinsic size should be used if css style size is 'auto' #
Total comments: 10
Patch Set 3 : applied only for SVG Image #Patch Set 4 : nits #
Total comments: 6
Patch Set 5 : Align with review comments #
Total comments: 10
Patch Set 6 : Align with review comments #Patch Set 7 : rename file #
Total comments: 21
Patch Set 8 : applied intrinsic ratio #
Total comments: 10
Patch Set 9 : Align with review comments #
Total comments: 3
Messages
Total messages: 42 (17 generated)
Message was sent while issue was closed.
Description was changed from ========== SVG intrinsic size should be used if css style size is 'auto' BUG=493681 ========== to ========== SVG intrinsic size should be used if css style size is 'auto' The following elements' width and height attributes will use intrinsic size if css style size is 'auto': * rect * image * foreignObject BUG=493681 ==========
shanmuga.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com, pdr@chromium.org, rob.buis@samsung.com
Please take a look. Thank you
The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Peer looks good to me. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:5: foreignObject {width: auto} Please add some spacing above: foreignObject { width: auto } https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:11: </foreignObject> Better to just make foreignObject closed: <foreignObject..... /> https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:12: <rect y="105" width="100" height="100" fill="green"> </rect> Ditto.
https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp:97: styleRef().width().isAuto() ? foreign->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), I guess 'auto' here would mean some form of shrink-wrap or adapting to whatever is within the fO. I'd suggest to leave it alone for now though. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73: styleRef().width().isAuto() ? image->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), No, this is not what should happen. To quote the relevant suggestion from the bug: "...I think 'auto' should mean use the intrinsic width of the referenced image." Notice that last part. So if we have an <image> referencing a bitmap with dimensions 1024x768, then 'width: auto' would mean a used value of '1024' (and similarly '768' for height.) I'm not sure if anything in particular should be done for when SVG images are referenced - maybe, maybe not. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRect.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGRect.cpp:57: styleRef().width().isAuto() ? rect->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), I'd suggest to leave this alone for now. (Not sure what makes sense here in practice.)
Description was changed from ========== SVG intrinsic size should be used if css style size is 'auto' The following elements' width and height attributes will use intrinsic size if css style size is 'auto': * rect * image * foreignObject BUG=493681 ========== to ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size if css style size is 'auto' BUG=493681 ==========
Done the changes as per review comments. Please take a look!! Thanks https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:12: <rect y="105" width="100" height="100" fill="green"> </rect> On 2016/08/10 15:25:25, rwlbuis wrote: > Ditto. Done. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGForeignObject.cpp:97: styleRef().width().isAuto() ? foreign->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), On 2016/08/10 18:01:34, fs (OoO until Aug 15) wrote: > I guess 'auto' here would mean some form of shrink-wrap or adapting to whatever > is within the fO. I'd suggest to leave it alone for now though. Acknowledged. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:73: styleRef().width().isAuto() ? image->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), On 2016/08/10 18:01:34, fs (OoO until Aug 15) wrote: > No, this is not what should happen. To quote the relevant suggestion from the > bug: "...I think 'auto' should mean use the intrinsic width of the referenced > image." Notice that last part. So if we have an <image> referencing a bitmap > with dimensions 1024x768, then 'width: auto' would mean a used value of '1024' > (and similarly '768' for height.) I'm not sure if anything in particular should > be done for when SVG images are referenced - maybe, maybe not. Thank you fs@ for explaining in detail. Done the changes. https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRect.cpp (right): https://codereview.chromium.org/2230963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGRect.cpp:57: styleRef().width().isAuto() ? rect->width()->currentValue()->value(lengthContext) : lengthContext.valueForLength(styleRef().width(), styleRef(), SVGLengthMode::Width), On 2016/08/10 18:01:34, fs (OoO until Aug 15) wrote: > I'd suggest to leave this alone for now. (Not sure what makes sense here in > practice.) Acknowledged.
https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:9: <image width="100" height="100" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAHwAAABcAgMAAADR3DkbAAAACVBMVEUisUztHCT///+Wt+Z1AAAAT0lEQVR4XmNYhR/QR35UfmooFhA1iOSBbHQwlbD8qPyo/AoGdMA1yOTR3N9Aqvyo/Kh8KAI4sEIZg0YeChDuhwK6yI/Ko4NBID842j+j8gD+5Vkn+nqy2gAAAABJRU5ErkJggg==" /> We should add tests for various configurations of an SVG image as well - with existence and non-existence of intrinsic dimensions and ratio varying. Also, this can be written as a testharness.js test (using getBBox for example), which would make that easier. https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:69: LayoutSize intrinsicSize = m_imageResource->imageSize(style()->effectiveZoom()); Probably needs to refine this a bit. (The end of) SVGImagePainter::computeImageViewportSize can probably serve as a reference.
https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:143: void LayoutSVGImage::imageChanged(WrappedImagePtr, const IntRect*) Probably need to make sure to trigger layout when the image changes and width/height is 'auto'. (More optimally only when dimensions actually end up changing.)
PTAL!! Thanks https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:9: <image width="100" height="100" xlink:href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAHwAAABcAgMAAADR3DkbAAAACVBMVEUisUztHCT///+Wt+Z1AAAAT0lEQVR4XmNYhR/QR35UfmooFhA1iOSBbHQwlbD8qPyo/AoGdMA1yOTR3N9Aqvyo/Kh8KAI4sEIZg0YeChDuhwK6yI/Ko4NBID842j+j8gD+5Vkn+nqy2gAAAABJRU5ErkJggg==" /> On 2016/08/11 15:52:57, fs wrote: > We should add tests for various configurations of an SVG image as well - with > existence and non-existence of intrinsic dimensions and ratio varying. Also, > this can be written as a testharness.js test (using getBBox for example), which > would make that easier. Added test with existence and non-existence of intrinsic dimensions https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:69: LayoutSize intrinsicSize = m_imageResource->imageSize(style()->effectiveZoom()); On 2016/08/11 15:52:57, fs wrote: > Probably needs to refine this a bit. (The end of) > SVGImagePainter::computeImageViewportSize can probably serve as a reference. Done. https://codereview.chromium.org/2230963002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:143: void LayoutSVGImage::imageChanged(WrappedImagePtr, const IntRect*) On 2016/08/11 16:39:36, fs wrote: > Probably need to make sure to trigger layout when the image changes and > width/height is 'auto'. (More optimally only when dimensions actually end up > changing.) Done.
The CQ bit was checked by shanmuga.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:2: <script src="../../resources/testharness.js"></script> I'd suggest adding a <title> ("<svg:image> 'auto' width/height" or something like that maybe). And maybe use: document.title + ', ' + testname as the testname. (Depending a bit on how the per-test description is formulated.) https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:40: new BBox(0, 0, 100, 100), Should probably add a description (string) as well - that seems more interesting and useful as a testname than the expected dimensions (alone.) Also, the 'x' and 'y' doesn't seem very relevant for this test, so could make these "dimension objects" instead. https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:72: intrinsicSize = FloatSize(cachedImage->getImage()->size()); I checked the spec, and these seem to be the relevant sections: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing (those can be added to the description) I notice that the initial value is set to '0' for the attribute (the former link) while it is 'auto' for the properties. Should probably get that cleared up in the spec. (This will affect existing content too.) I'm not 100% sure that the different descriptions of 'auto' are useful (and equivalent either), so that's one other thing to consider also filing a spec issue about. (I'd go with the description from the second link, since that seems to just refer to CSS sizing.) https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:159: if (oldBoundaries.size() != newBoundaries.size()) { Just make updateBoundingBox() return a bool instead. https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:160: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::SizeChanged); Maybe just make this setNeedsLayout and then a fall-through to mark for paint invalidation. Also, make sure there's a test for this code-path.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size if css style size is 'auto' BUG=493681 ========== to ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ==========
PTAL!! https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:2: <script src="../../resources/testharness.js"></script> On 2016/08/16 12:43:07, fs wrote: > I'd suggest adding a <title> ("<svg:image> 'auto' width/height" or something > like that maybe). > > And maybe use: > > document.title + ', ' + testname > > as the testname. (Depending a bit on how the per-test description is > formulated.) Done. https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/custom/svg-intrinsic-size-with-cssstyle-auto.html:40: new BBox(0, 0, 100, 100), On 2016/08/16 12:43:08, fs wrote: > Should probably add a description (string) as well - that seems more interesting > and useful as a testname than the expected dimensions (alone.) Also, the 'x' and > 'y' doesn't seem very relevant for this test, so could make these "dimension > objects" instead. Done. https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:72: intrinsicSize = FloatSize(cachedImage->getImage()->size()); On 2016/08/16 12:43:08, fs wrote: > I checked the spec, and these seem to be the relevant sections: > > https://svgwg.org/svg2-draft/embedded.html#Placement > https://svgwg.org/svg2-draft/geometry.html#Sizing > > (those can be added to the description) > > I notice that the initial value is set to '0' for the attribute (the former > link) while it is 'auto' for the properties. Should probably get that cleared up > in the spec. (This will affect existing content too.) I'm not 100% sure that the > different descriptions of 'auto' are useful (and equivalent either), so that's > one other thing to consider also filing a spec issue about. (I'd go with the > description from the second link, since that seems to just refer to CSS sizing.) I filed a spec bug: https://github.com/w3c/svgwg/issues/237 https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:159: if (oldBoundaries.size() != newBoundaries.size()) { On 2016/08/16 12:43:08, fs wrote: > Just make updateBoundingBox() return a bool instead. Done. https://codereview.chromium.org/2230963002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:160: setNeedsLayoutAndFullPaintInvalidation(LayoutInvalidationReason::SizeChanged); On 2016/08/16 12:43:08, fs wrote: > Maybe just make this setNeedsLayout and then a fall-through to mark for paint > invalidation. > > Also, make sure there's a test for this code-path. Done.
https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:10: async_test(function(t) { Switch the order of these two lines, and use a step_func for window.onload. (Indentation is also off-by-one here.) https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:11: var image = document.getElementsByTagName('image')[0]; querySelector('image') https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:21: }, document.title + ' and box size should be ' + '100 100'); I think you can go with the default testname here. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:2: <title>Test <svg:image>'s sizing </title> Drop "Test" and add 'auto', so maybe: <svg:image> 'auto' sizing or something. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:27: <image xlink:href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'></svg>"/> Still missing tests with no intrinsic dimensions but intrinsic ratio. (And similar variations.) https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:40: new Dimension(100, 100, "400x400 png image, with intrinsic width='100' height='100'"), You could probably drop the class, and just do: { dimensions: [100, 100], description: "..."}, ... the toString output is pretty redundant with the description. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:53: new Dimension(300, 150, "default sized svg image, without intrinsic width and height, no css width/height specified"), None these four have any intrinsic dimension. Actually, most (all?) of the statements above about 'intrinsic <something>' appear to be incorrect. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:78: styleRef().height().isAuto() ? intrinsicSize.height() : lengthContext.valueForLength(styleRef().height(), styleRef(), SVGLengthMode::Height)); This is still not using what's specced by https://svgwg.org/svg2-draft/geometry.html#Sizing (section 6.8)
https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:27: <image xlink:href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'></svg>"/> On 2016/08/19 08:59:45, fs wrote: > Still missing tests with no intrinsic dimensions but intrinsic ratio. (And > similar variations.) Could you please give one sample for that(no intrinsic dimensions but intrinsic ratio). So that I can understand better and add more variations. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:53: new Dimension(300, 150, "default sized svg image, without intrinsic width and height, no css width/height specified"), On 2016/08/19 08:59:45, fs wrote: > None these four have any intrinsic dimension. Actually, most (all?) of the > statements above about 'intrinsic <something>' appear to be incorrect. 'specified <something>' could be correct ? https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:78: styleRef().height().isAuto() ? intrinsicSize.height() : lengthContext.valueForLength(styleRef().height(), styleRef(), SVGLengthMode::Height)); On 2016/08/19 08:59:45, fs wrote: > This is still not using what's specced by > https://svgwg.org/svg2-draft/geometry.html#Sizing (section 6.8) Agree. I will study more about intrinsic dimension and intrinsic ratio.
https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:27: <image xlink:href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'></svg>"/> On 2016/08/19 at 11:56:42, Shanmuga Pandi wrote: > On 2016/08/19 08:59:45, fs wrote: > > Still missing tests with no intrinsic dimensions but intrinsic ratio. (And > > similar variations.) > > Could you please give one sample for that(no intrinsic dimensions but intrinsic ratio). > So that I can understand better and add more variations. <svg viewBox="0 0 400 200"></svg> https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:53: new Dimension(300, 150, "default sized svg image, without intrinsic width and height, no css width/height specified"), On 2016/08/19 at 11:56:42, Shanmuga Pandi wrote: > On 2016/08/19 08:59:45, fs wrote: > > None these four have any intrinsic dimension. Actually, most (all?) of the > > statements above about 'intrinsic <something>' appear to be incorrect. > > 'specified <something>' could be correct ? Maybe explicitly say "attributes width='..."' ...".
Spec has updated based on bug we raised. https://svgwg.org/svg2-draft/embedded.html#Placement To achieve the expected dimension based on the spec, could we derive LayoutReplaced for LayoutSVGImage instead of LayoutSVGModelObject ? Or we just implement only the size calculation methods if auto 'width/height' specified? Please let me know your opinion.
On 2016/08/25 at 07:42:31, shanmuga.m wrote: > Spec has updated based on bug we raised. > https://svgwg.org/svg2-draft/embedded.html#Placement > > To achieve the expected dimension based on the spec, could we derive LayoutReplaced for LayoutSVGImage instead of LayoutSVGModelObject ? > Or we just implement only the size calculation methods if auto 'width/height' specified? The latter.
Please take a look. Thanks. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:10: async_test(function(t) { On 2016/08/19 08:59:45, fs wrote: > Switch the order of these two lines, and use a step_func for window.onload. > (Indentation is also off-by-one here.) Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:11: var image = document.getElementsByTagName('image')[0]; On 2016/08/19 08:59:45, fs wrote: > querySelector('image') Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:21: }, document.title + ' and box size should be ' + '100 100'); On 2016/08/19 08:59:45, fs wrote: > I think you can go with the default testname here. Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:2: <title>Test <svg:image>'s sizing </title> On 2016/08/19 08:59:45, fs wrote: > Drop "Test" and add 'auto', so maybe: > > <svg:image> 'auto' sizing > > or something. Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:27: <image xlink:href="data:image/svg+xml,<svg xmlns='http://www.w3.org/2000/svg'></svg>"/> On 2016/08/19 12:01:31, fs wrote: > On 2016/08/19 at 11:56:42, Shanmuga Pandi wrote: > > On 2016/08/19 08:59:45, fs wrote: > > > Still missing tests with no intrinsic dimensions but intrinsic ratio. (And > > > similar variations.) > > > > Could you please give one sample for that(no intrinsic dimensions but > intrinsic ratio). > > So that I can understand better and add more variations. > > <svg viewBox="0 0 400 200"></svg> I have added more variation. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:40: new Dimension(100, 100, "400x400 png image, with intrinsic width='100' height='100'"), On 2016/08/19 08:59:45, fs wrote: > You could probably drop the class, and just do: > > { dimensions: [100, 100], description: "..."}, > ... > > the toString output is pretty redundant with the description. Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:53: new Dimension(300, 150, "default sized svg image, without intrinsic width and height, no css width/height specified"), On 2016/08/19 12:01:31, fs wrote: > On 2016/08/19 at 11:56:42, Shanmuga Pandi wrote: > > On 2016/08/19 08:59:45, fs wrote: > > > None these four have any intrinsic dimension. Actually, most (all?) of the > > > statements above about 'intrinsic <something>' appear to be incorrect. > > > > 'specified <something>' could be correct ? > > Maybe explicitly say "attributes width='..."' ...". Done. https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:78: styleRef().height().isAuto() ? intrinsicSize.height() : lengthContext.valueForLength(styleRef().height(), styleRef(), SVGLengthMode::Height)); On 2016/08/19 08:59:45, fs wrote: > This is still not using what's specced by > https://svgwg.org/svg2-draft/geometry.html#Sizing (section 6.8) Please check this.
fs@opera.com changed reviewers: + davve@opera.com
+davve https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:22: }, document.title); document.title not needed here (it's the default.) https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:47: { dimensions: [100, 100], description: "400x400 png image, with attributes width='100' height='100'"}, I think we could drop all the instances of "with" in the descriptions. Also all the commas before "and". https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:90: return FloatSize(); This is unreachable? Maybe just assert styleRef().width().isAuto() and return that size instead? https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:106: if (styleRef().width().isAuto()) This branching looks redundant - couldn't this just do: m_objectBoundingBox.setSize(calculateObjectSize()); ? (Might need to return m_objectBoundingBox.size() in the error-case in calculateObjectSize.) https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.h (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.h:71: FloatSize calculateObjectSize(); const qualify?
PTAL!! Thanks https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto-dynamic-image-change.html:22: }, document.title); On 2016/09/14 11:31:52, fs wrote: > document.title not needed here (it's the default.) Done. https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/custom/svg-image-intrinsic-size-with-cssstyle-auto.html:47: { dimensions: [100, 100], description: "400x400 png image, with attributes width='100' height='100'"}, On 2016/09/14 11:31:52, fs wrote: > I think we could drop all the instances of "with" in the descriptions. Also all > the commas before "and". Done. https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:90: return FloatSize(); On 2016/09/14 11:31:52, fs wrote: > This is unreachable? Maybe just assert styleRef().width().isAuto() and return > that size instead? Done. https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:106: if (styleRef().width().isAuto()) On 2016/09/14 11:31:52, fs wrote: > This branching looks redundant - couldn't this just do: > > m_objectBoundingBox.setSize(calculateObjectSize()); > > ? (Might need to return m_objectBoundingBox.size() in the error-case in > calculateObjectSize.) Done. https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.h (right): https://codereview.chromium.org/2230963002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.h:71: FloatSize calculateObjectSize(); On 2016/09/14 11:31:52, fs wrote: > const qualify? Done.
LGTM, but please allow some time for other reviewers to comment.
https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: bool LayoutSVGImage::updateBoundingBox() Having updateBoundingBox() both set a member variable (m_needsBoundariesUpdate) and return a value looks a bit odd to me. Especially since we now call updateBoundingBox() from outside layout(), where the m_needsBoundariesUpdate machinery lives. (I suppose m_needsBoundariesUpdate will be set until the layout() and will counteract a no-up updateBoundingBox() there?) It may be fine and dandy but the fallout isn't obvious to me.
https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: bool LayoutSVGImage::updateBoundingBox() On 2016/09/14 at 14:13:17, davve wrote: > Having updateBoundingBox() both set a member variable (m_needsBoundariesUpdate) and return a value looks a bit odd to me. Especially since we now call updateBoundingBox() from outside layout(), where the m_needsBoundariesUpdate machinery lives. (I suppose m_needsBoundariesUpdate will be set until the layout() and will counteract a no-up updateBoundingBox() there?) It may be fine and dandy but the fallout isn't obvious to me. IIRC we have a bug on file to shift some of this stuff to styleDidChange or thereabouts. I suppose that wouldn't really help with the determine-if-we-need-to-layout bit though.
The commit description looks like it could use some line-breaking at 80 cols or so. lgtm https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: bool LayoutSVGImage::updateBoundingBox() On 2016/09/14 14:37:11, fs wrote: > On 2016/09/14 at 14:13:17, davve wrote: > > Having updateBoundingBox() both set a member variable > (m_needsBoundariesUpdate) and return a value looks a bit odd to me. Especially > since we now call updateBoundingBox() from outside layout(), where the > m_needsBoundariesUpdate machinery lives. (I suppose m_needsBoundariesUpdate will > be set until the layout() and will counteract a no-up updateBoundingBox() > there?) It may be fine and dandy but the fallout isn't obvious to me. > > IIRC we have a bug on file to shift some of this stuff to styleDidChange or > thereabouts. I suppose that wouldn't really help with the > determine-if-we-need-to-layout bit though. s/no-up/no-op/. Sorry about that. After some more contemplating, I think I've managed to convince myself this is OK.
Description was changed from ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ========== to ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size, if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ==========
On 2016/09/15 12:34:43, davve wrote: > The commit description looks like it could use some line-breaking at 80 cols or > so. > > lgtm > > https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp (right): > > https://codereview.chromium.org/2230963002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/svg/LayoutSVGImage.cpp:91: bool > LayoutSVGImage::updateBoundingBox() > On 2016/09/14 14:37:11, fs wrote: > > On 2016/09/14 at 14:13:17, davve wrote: > > > Having updateBoundingBox() both set a member variable > > (m_needsBoundariesUpdate) and return a value looks a bit odd to me. Especially > > since we now call updateBoundingBox() from outside layout(), where the > > m_needsBoundariesUpdate machinery lives. (I suppose m_needsBoundariesUpdate > will > > be set until the layout() and will counteract a no-up updateBoundingBox() > > there?) It may be fine and dandy but the fallout isn't obvious to me. > > > > IIRC we have a bug on file to shift some of this stuff to styleDidChange or > > thereabouts. I suppose that wouldn't really help with the > > determine-if-we-need-to-layout bit though. > > s/no-up/no-op/. Sorry about that. After some more contemplating, I think I've > managed to convince myself this is OK. Thank you fs@ and davve@ for reviewing.
The CQ bit was checked by shanmuga.m@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size, if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ========== to ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size, if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size, if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 ========== to ========== SVG Image intrinsic size should be used if css style size is 'auto' SVG Image's width and height attributes will use intrinsic size, if css style size is 'auto' Spec: https://svgwg.org/svg2-draft/embedded.html#Placement https://svgwg.org/svg2-draft/geometry.html#Sizing BUG=493681 Committed: https://crrev.com/3c1a3822848d986337573cfaf2817c8a69417791 Cr-Commit-Position: refs/heads/master@{#419133} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3c1a3822848d986337573cfaf2817c8a69417791 Cr-Commit-Position: refs/heads/master@{#419133} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
