Bug 142805

Summary: Switching between two SVG images with no intrinsic sizes causes them to get the default SVG size instead of the container size
Product: WebKit Reporter: Said Abou-Hallawa <sabouhallawa>
Component: SVGAssignee: Said Abou-Hallawa <sabouhallawa>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ossy, rego, simon.fraser, webkit-bug-importer, zalan, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 142859    
Bug Blocks:    
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Said Abou-Hallawa 2015-03-17 17:43:12 PDT
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).
Comment 1 Said Abou-Hallawa 2015-03-17 18:22:03 PDT
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.
Comment 2 Said Abou-Hallawa 2015-03-18 11:51:07 PDT
Created attachment 248947 [details]
Patch
Comment 3 Said Abou-Hallawa 2015-03-18 13:06:50 PDT
Created attachment 248954 [details]
Patch
Comment 4 Darin Adler 2015-03-18 16:25:29 PDT
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
Comment 5 Said Abou-Hallawa 2015-03-18 17:07:06 PDT
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.
Comment 6 Said Abou-Hallawa 2015-03-18 17:09:33 PDT
Created attachment 248989 [details]
Patch
Comment 7 Said Abou-Hallawa 2015-03-18 17:18:51 PDT
Created attachment 248992 [details]
Patch
Comment 8 Simon Fraser (smfr) 2015-03-18 18:00:53 PDT
(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"
Comment 9 WebKit Commit Bot 2015-03-18 18:15:36 PDT
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
Comment 10 Said Abou-Hallawa 2015-03-18 18:22:29 PDT
Created attachment 249005 [details]
Patch
Comment 11 WebKit Commit Bot 2015-03-18 19:15:38 PDT
Comment on attachment 249005 [details]
Patch

Clearing flags on attachment: 249005

Committed r181720: <http://trac.webkit.org/changeset/181720>
Comment 12 WebKit Commit Bot 2015-03-18 19:15:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Manuel Rego Casasnovas 2015-03-18 23:10:39 PDT
This broke GTK+ build. It's been fixed in:
http://trac.webkit.org/changeset/181729
Comment 14 Csaba Osztrogonác 2015-03-18 23:14:03 PDT
(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. :(
Comment 15 Manuel Rego Casasnovas 2015-03-19 01:05:08 PDT
(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.
Comment 16 Csaba Osztrogonác 2015-03-19 01:14:28 PDT
(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.