Bug 142805 – Switching between two SVG images with no intrinsic sizes causes them to get the default SVG size instead of the container size

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Created attachment 248890 [details]
test case

Open the attached test case. 

Expected: No red color is displayed and the size of the green rectangle should be 300x100 pixels.

Switching from SVG with intrinsic size to an SVG with no intrinsic size or vice versa seems to work as expected.  The problem happens when switching between two images with no-intrisinc sizes back and forth. In this case we use the old layout of the SVG image but we set the SVG size to the default SVG intrinsic size which is 300x150 pixels. In the attached test case, the container size is 300x100 pixels. The old layout can display the SVG in 300x100 pixels. But because we change the image size to be 300x150 pixels, the SVG gets scaled down by (1,100/150).

[Said Abou-Hallawa]

Another example of this problem can be seen in http://snapsvg.io/demos/. When switching between the items in the side bar on the left. Currently we do not display the SVG with the smaller size and we leave dirt behind from the SVG with the bigger size. This bug addresses the layout issue which is responsible for not showing the smaller SVG image.

[Said Abou-Hallawa]

Created attachment 248947 [details]
Patch

[Said Abou-Hallawa]

Created attachment 248954 [details]
Patch

[Darin Adler]

Comment on attachment 248954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248954&action=review

> Source/WebCore/rendering/RenderImage.cpp:350
> +    LayoutRect repaintRect;
> +    if (rect) {
> +        // The image changed rect is in source image coordinates (pre-zooming),
> +        // so map from the bounds of the image to the contentsBox.
> +        repaintRect = enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1.0f)), contentBoxRect()));
> +        // Guard against too-large changed rects.
> +        repaintRect.intersect(contentBoxRect());
> +    } else
> +        repaintRect = contentBoxRect();
>          
> -        repaintRectangle(repaintRect);
> +    repaintRectangle(repaintRect);

I would have written this more like this:

    LayoutRect repaintRect = contentBoxRect();
    if (rect) {
        // The image changed rect is in source image coordinates (pre-zooming),
        // so map from the bounds of the image to the contentsBox.
        repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect)));
    }

But also, I don’t understand why there is a call to enclosingIntRect here.

> Source/WebCore/rendering/RenderImage.h:51
> +    ImageSizeChangeType setImageSizeForAltText(CachedImage* newImage = 0);

nullptr

> Source/WebCore/rendering/RenderImage.h:115
> +    void repaintOrMarkForLayout(ImageSizeChangeType, const IntRect* = 0);

nullptr

[Said Abou-Hallawa]

Comment on attachment 248954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=248954&action=review

>> Source/WebCore/rendering/RenderImage.cpp:350
>> +    repaintRectangle(repaintRect);
> 
> I would have written this more like this:
> 
>     LayoutRect repaintRect = contentBoxRect();
>     if (rect) {
>         // The image changed rect is in source image coordinates (pre-zooming),
>         // so map from the bounds of the image to the contentsBox.
>         repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect)));
>     }
> 
> But also, I don’t understand why there is a call to enclosingIntRect here.

Done.

I think enclosingIntRect is used because LayoutRect::interest() takes a LayoutRect& as a parameter.  And mapRect takes FloatRect& and returns also FloatRect.  There is no implicit copy constructor in LayoutRect to convert from FloatRect to LayoutRect.  But there is copy constructor to convert from IntRect to LayoutRect.  When I remove the call to enclosingIntRect(), the compiler gives the error: "No viable conversion from FloatRect to const LayoutRect"

>> Source/WebCore/rendering/RenderImage.h:51
>> +    ImageSizeChangeType setImageSizeForAltText(CachedImage* newImage = 0);
> 
> nullptr

Done.

>> Source/WebCore/rendering/RenderImage.h:115
>> +    void repaintOrMarkForLayout(ImageSizeChangeType, const IntRect* = 0);
> 
> nullptr

Done.

[Said Abou-Hallawa]

Created attachment 248989 [details]
Patch

[Said Abou-Hallawa]

Created attachment 248992 [details]
Patch

[Simon Fraser (smfr)]

(In reply to comment #5)
> Comment on attachment 248954 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248954&action=review
> 
> >> Source/WebCore/rendering/RenderImage.cpp:350
> >> +    repaintRectangle(repaintRect);
> > 
> > I would have written this more like this:
> > 
> >     LayoutRect repaintRect = contentBoxRect();
> >     if (rect) {
> >         // The image changed rect is in source image coordinates (pre-zooming),
> >         // so map from the bounds of the image to the contentsBox.
> >         repaintRect.intersect(enclosingIntRect(mapRect(*rect, FloatRect(FloatPoint(), imageResource().imageSize(1)), repaintRect)));
> >     }
> > 
> > But also, I don’t understand why there is a call to enclosingIntRect here.
> 
> Done.
> 
> I think enclosingIntRect is used because LayoutRect::interest() takes a
> LayoutRect& as a parameter.  And mapRect takes FloatRect& and returns also
> FloatRect.  There is no implicit copy constructor in LayoutRect to convert
> from FloatRect to LayoutRect.

That's because this is lossy. LayoutRect has a resolution of 1/64th pixels. You can explicitly create a LayoutRect from a FloatRect though.

> But there is copy constructor to convert from
> IntRect to LayoutRect.

Which is non-lossy.

> When I remove the call to enclosingIntRect(), the
> compiler gives the error: "No viable conversion from FloatRect to const LayoutRect"

[WebKit Commit Bot]

Comment on attachment 248992 [details]
Patch

Rejecting attachment 248992 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 248992, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4796037164171264

[Said Abou-Hallawa]

Created attachment 249005 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249005 [details]
Patch

Clearing flags on attachment: 249005

Committed r181720: <http://trac.webkit.org/changeset/181720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Manuel Rego Casasnovas]

This broke GTK+ build. It's been fixed in:
http://trac.webkit.org/changeset/181729

[Csaba Osztrogonác]

(In reply to comment #13)
> This broke GTK+ build. It's been fixed in:
> http://trac.webkit.org/changeset/181729

bug142859 fixed it too 3 hours before, but there were nobody to review it. :(

[Manuel Rego Casasnovas]

(In reply to comment #14)
> (In reply to comment #13)
> > This broke GTK+ build. It's been fixed in:
> > http://trac.webkit.org/changeset/181729
> 
> bug142859 fixed it too 3 hours before, but there were nobody to review it. :(

Sorry, didn't realize there was a fix already.
It was so simple that I just go ahead as the GTK+ EWS was getting into troubles.

[Csaba Osztrogonác]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > This broke GTK+ build. It's been fixed in:
> > > http://trac.webkit.org/changeset/181729
> > 
> > bug142859 fixed it too 3 hours before, but there were nobody to review it. :(
> 
> Sorry, didn't realize there was a fix already.
> It was so simple that I just go ahead as the GTK+ EWS was getting into
> troubles.

Not problem, thanks for the fix. :) I commented ":(" because the fix
was already done, but nobody reviewed and landed in for a long period.

To avoid parallel fixing and long build break period I suggested commenting
the original bug and pinging reviewers on #webkit until it is reviewed and
landed.