Bug 150904 – REGRESSION (r190883): Error calculating the tile size for an SVG with no intrinsic size but with large floating intrinsic ratio

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Said Abou-Hallawa]

Created attachment 264814 [details]
test case

1. Open the attached test case.

Results: 40% of the two <div>s rectangle are not covered by the background image.
Expected: 100% of the two  <div>s rectangle are by the background image.

The bug in drawing the background of the first <div> is a regression of r190883 which happens because of missing the following if-statemnet in resolveAgainstIntrinsicRatio() when rolling out the r184895:

   if (!resolvedSize.isEmpty())
       return resolvedSize;

The bug in drawing the background of the second <div> has been there for sometime. At least it is in Safari 9.0. The bug is in calculating the tile width given the a background SVG with non intrinsic size but with a large float intrinsic ratio.

Let's take a conceder example to show why it happens and consider the attached test case when we draw the second <div>.

1. The css of the second <div> draws a background SVG image whose width is '8000.4px' while the height is not determined.
2. The viewBox of the SVG is '0 0 8000.4 100'
3. RenderBoxModelObject::calculateFillTileSize() calls RenderBoxModelObject::calculateImageIntrinsicDimensions() to calculate the imageIntrinsicSize.
4. In RenderBoxModelObject::calculateImageIntrinsicDimensions(), because (resolvedSize.width() == 0 && resolvedSize.height() > 0) is true we return resolveAgainstIntrinsicWidthOrHeightAndRatio().
5. The input to this function is the following: intrinsicRatio = FloatSize(8000.4, 100), useWidth = LayoutUnit(512025) and useHeight = LayoutUnit(0)
6. Because useWidth is not zero, resolveHeightForRatio() is called to calculate the height.
7. resolveHeightForRatio() does its calculation in float but it then casts the result to integer. So it returns 99 instead of 100.
8. In resolveAgainstIntrinsicWidthOrHeightAndRatio(), the return of resolveHeightForRatio() is changed back to LayoutUnit but we end up returning LayoutSize(512025, 6336) instead of returning LayoutSize(512025, 6400).
9. In RenderBoxModelObject::calculateFillTileSize(), because the background-size is "auto 100px", 
        tileSize = LayoutSize(LayoutUnit(imageIntrinsicSize.width() * tileSize.height() / imageIntrinsicSize.height()), LayoutUnit(6400));
        tileSize = LayoutSize(LayoutUnit(512025 * 6400 / 6336), LayoutUnit(6400));
        tileSize = LayoutSize(LayoutUnit(517196), LayoutUnit(6400)); // == FloatSize(8081.2, 100) instead of FloatSize(8000.4, 100)

And this is the cause of not filling the whole rectangle of the second div.

[Said Abou-Hallawa]

The missing lines in resolveAgainstIntrinsicRatio():

   if (!resolvedSize.isEmpty())
       return resolvedSize;

are kind of optimization. But carrying out the imageSize calculation through resolveAgainstIntrinsicWidthOrHeightAndRatio() should have yield the same results.

[Said Abou-Hallawa]

Created attachment 264819 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/23364325>

[Said Abou-Hallawa]

Another way to see this bug is the clipped logo in http://www.wired.com.

[Said Abou-Hallawa]

Created attachment 264940 [details]
Patch

[Said Abou-Hallawa]

Zalan pointed out that resolveWidthForRatio() and resolveHeightForRatio() should return LayoutUnits instead on integers which I completely agree with. The inputs of these functions are now LayoutUnits. And the return values are passed to LayoutSizes so it does not make sense to convert from LayoutUnit to int and then to LayoutUnit only at return time. This might cause a truncation in other scenarios.

I had resolveWidthForRatio() and resolveHeightForRatio() returning LayoutUnits before submitting the original patches. But I wanted to verify the layout test would fail without the patch. So I reverted the changes manually and then added them back manually also. I must have missed adding them back especially the complier was fine without them.

[WebKit Commit Bot]

Comment on attachment 264940 [details]
Patch

Clearing flags on attachment: 264940

Committed r192161: <http://trac.webkit.org/changeset/192161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.