Bug 150904 - REGRESSION (r190883): Error calculating the tile size for an SVG with no intrinsic size but with large floating intrinsic ratio
Summary: REGRESSION (r190883): Error calculating the tile size for an SVG with no intr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
Keywords: InRadar, Regression
Depends on:
Reported: 2015-11-04 14:25 PST by Said Abou-Hallawa
Modified: 2015-11-09 10:10 PST (History)
6 users (show)

See Also:

test case (1.26 KB, text/html)
2015-11-04 14:25 PST, Said Abou-Hallawa
no flags Details
Patch (8.58 KB, patch)
2015-11-04 15:09 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (8.57 KB, patch)
2015-11-06 09:42 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2015-11-04 14:25:35 PST
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.
Comment 1 Said Abou-Hallawa 2015-11-04 14:29:11 PST
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.
Comment 2 Said Abou-Hallawa 2015-11-04 15:09:49 PST
Created attachment 264819 [details]
Comment 3 Said Abou-Hallawa 2015-11-04 15:13:54 PST
Comment 4 Said Abou-Hallawa 2015-11-04 15:15:06 PST
Another way to see this bug is the clipped logo in http://www.wired.com.
Comment 5 Said Abou-Hallawa 2015-11-06 09:42:42 PST
Created attachment 264940 [details]
Comment 6 Said Abou-Hallawa 2015-11-06 09:56:50 PST
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.
Comment 7 WebKit Commit Bot 2015-11-09 10:10:28 PST
Comment on attachment 264940 [details]

Clearing flags on attachment: 264940

Committed r192161: <http://trac.webkit.org/changeset/192161>
Comment 8 WebKit Commit Bot 2015-11-09 10:10:32 PST
All reviewed patches have been landed.  Closing bug.