Bug 124784 – [CSS Shapes] shape-inside rectangle layout can fail

• Component: CSS
• Status: RESOLVED FIXED

[Hans Muller]

Created attachment 217701 [details]
Test case.

If the top of a rectangle is a subpixel value whose fractional part is < 0.5 and the rectangle is wide enough, then the top's Y coordinate will be returned by RectangleShape::firstIncludedIntervalLogicalTop(). Absent subpixel layout support, the value may be truncated when it is returned to RectangleShape::getIncludedIntervals(). This makes it appear that an interval whose top is the truncate value does not fit within the shape.

It's likely the that correct remedy is the one used by PolygonShape: apply ceiledLayoutUnit(Y) in RectangleShape::firstIncludedIntervalLogicalTop().

[Hans Muller]

Created attachment 217909 [details]
Patch

Apply LayoutUnit::fromFloatCeil() consistently in RectangleShape::firstIncludedIntervalLogicalTop().

[Andreas Kling]

Comment on attachment 217909 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 217909 [details]
Patch

Clearing flags on attachment: 217909

Committed r159821: <http://trac.webkit.org/changeset/159821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[zalan]

The committed test case fails when subpixel layout is enabled. By looking at the expected test result, it seems that 1.3px is supposed to get rounded to 2px. However, with subpixel on, it's never the case.
ceiledLayoutUnit() ceils to layout unit value and not to CSS pixel value (unless the 2 units are equal which is the case when subpixel is off)
subpixel off: float 1.3 -> ceilLayoutUnit() -> ceil(1.3 * 1) -> 2 layout unit value -> 2/1 -> 2px CSS pixel value
subpixel on: float 1.3 -> ceilLayoutUnit() -> ceil(1.3 * 64) -> ceil(83.2) -> 84 layout unit value -> 84 / 64 -> 1.312 -> snapped to 1px at painting time;

If the intention is really to convert 1.3px -> 2px during layout (which we tend to avoid as it defeats the purpose of using subpixel values), then you need to come up with a function that ceils your float value to CSS px. We don't have such function yet and even the existing snapping functions (roundToInt(), floorToInt()) are mostly used at painting time. I don't know enough about shapes to tell whether ceiling the layout unit is wrong, but in general it's the opposite direction of what we do at other places.
(Also, if the return value is expect to be ceiled, it's better to actually ceil right before returning as opposed to ceil earlier. Early conversion could have some unwanted side effect on other values)

[zalan]

Created attachment 221497 [details]
Patch

[zalan]

The patch assumes that we don't actually need to ceil the float value to CSS px at layout time.

[Darin Adler]

Comment on attachment 221497 [details]
Patch

Not sure I am qualified to review this. Maybe Simon Fraser or another layout expert needs to (Hyatt?).

[zalan]

(In reply to comment #8)
> (From update of attachment 221497 [details])
> Not sure I am qualified to review this. Maybe Simon Fraser or another layout expert needs to (Hyatt?).

Thanks Darin. I should probably ping some Adobe/shapes guy to see which way they wanna go.

[Hans Muller]

(In reply to comment #7)
> The patch assumes that we don't actually need to ceil the float value to CSS px at layout time.

The use LayoutUnit:::fromFloatCeil(f) was to round up to the next layout unit, not a mistaken way to compute ceil(f).  In the line that determines the result of this test,

float minY = LayoutUnit::fromFloatCeil(std::max(bounds.y(), minIntervalTop));

the objective is to ensure that the result is within the rectangle. Since minIntervalTop is already a LayoutUnit, this could have been written:

 float minY = std::max(LayoutUnit::fromFloatCeil(bounds.y()), minIntervalTop));

I think the ...test.html should contain:

    #shape-inside {
        -webkit-shape-inside: rectangle(0px, 1.3px, 200px, 200px);
        font: 100px/1 Ahem, sans-serif;
        -webkit-font-smoothing: antialiased;
        color: green;
        background-color: grey;
        background-clip: padding-box;
        width: 200px;
        height: 200px;
    }

And the ...test-expected.html should be:

    #shape-inside {
        font: 100px/1 Ahem, sans-serif;
        -webkit-font-smoothing: antialiased;
        color: green;
        background-color: grey;
        background-clip: padding-box;
        width: 200px;
        height: 198.7px;
        padding-top: 1.3px;
    }

This produces an off-by one when subpixel layout is turned off, because the padding-top value causes the single Ahem character to be rendered at 1, but our "keep it within the rectangle" code puts it at 2, as you've pointed out.

I'll be happy to fix this, roughly along the lines you've suggested, if you want.

[zalan]

(In reply to comment #10)
> (In reply to comment #7)
> > The patch assumes that we don't actually need to ceil the float value to CSS px at layout time.
> 
> The use LayoutUnit:::fromFloatCeil(f) was to round up to the next layout unit, not a mistaken way to compute ceil(f).  In the line that determines the result of this test,
> 
> float minY = LayoutUnit::fromFloatCeil(std::max(bounds.y(), minIntervalTop));
> 
> the objective is to ensure that the result is within the rectangle. Since minIntervalTop is already a LayoutUnit, this could have been written:
> 
>  float minY = std::max(LayoutUnit::fromFloatCeil(bounds.y()), minIntervalTop));
> 
> I think the ...test.html should contain:
> 
>     #shape-inside {
>         -webkit-shape-inside: rectangle(0px, 1.3px, 200px, 200px);
>         font: 100px/1 Ahem, sans-serif;
>         -webkit-font-smoothing: antialiased;
>         color: green;
>         background-color: grey;
>         background-clip: padding-box;
>         width: 200px;
>         height: 200px;
>     }
> 
> And the ...test-expected.html should be:
> 
>     #shape-inside {
>         font: 100px/1 Ahem, sans-serif;
>         -webkit-font-smoothing: antialiased;
>         color: green;
>         background-color: grey;
>         background-clip: padding-box;
>         width: 200px;
>         height: 198.7px;
>         padding-top: 1.3px;
>     }
> 
> This produces an off-by one when subpixel layout is turned off, because the padding-top value causes the single Ahem character to be rendered at 1, but our "keep it within the rectangle" code puts it at 2, as you've pointed out.
> 
> I'll be happy to fix this, roughly along the lines you've suggested, if you want.

So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that.

[Hans Muller]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #7)

> So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that.

Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test.

[Hans Muller]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> 
> > So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that.
> 
> Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test.

Sorry, I also meant to say: yes that sounds like all that's needed.

[zalan]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #7)
> 
> > So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that.
> 
> Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test.

That's there to trigger sync layout.

[zalan]

Created attachment 221827 [details]
Patch

[zalan]

This is blocked by bug 127404. The expected test case goes through the SimpleLineLayout code path, which needs some extra rounding. I'll cq the patch soon after I have a fix for the blocking bug.

(the expected test case needs the '2px' adjustment as the 1.3px padding gets floored to 1px as the result of integer arithmetic. normal 'subpixel off' padding/margin/border behavior)

[Build Bot]

Comment on attachment 221827 [details]
Patch

Attachment 221827 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6427164685631488

New failing tests:
fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

[Build Bot]

Created attachment 221866 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[zalan]

(In reply to comment #17)
> (From update of attachment 221827 [details])
> Attachment 221827 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/6427164685631488
> 
> New failing tests:
> fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

bug 127404

[Build Bot]

Comment on attachment 221827 [details]
Patch

Attachment 221827 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5153810498453504

New failing tests:
fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

[Build Bot]

Created attachment 221867 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Hans Muller]

(In reply to comment #16)
> This is blocked by bug 127404. The expected test case goes through the SimpleLineLayout code path, which needs some extra rounding. I'll cq the patch soon after I have a fix for the blocking bug.
> 
> (the expected test case needs the '2px' adjustment as the 1.3px padding gets floored to 1px as the result of integer arithmetic. normal 'subpixel off' padding/margin/border behavior)

Excellent, I'd noticed this as well. Glad to hear that a fix is coming.

[Build Bot]

Comment on attachment 221827 [details]
Patch

Attachment 221827 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6262856416755712

New failing tests:
fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

[Build Bot]

Created attachment 221869 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[WebKit Commit Bot]

Comment on attachment 221827 [details]
Patch

Rejecting attachment 221827 [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', 'apply-attachment', '--no-update', '--non-interactive', 221827, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
cceeded at 1 with fuzz 3.
patching file LayoutTests/fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top-expected.html
patching file LayoutTests/platform/mac/TestExpectations
Original content of LayoutTests/platform/mac/fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top-expected.png mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 265.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

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

[zalan]

Committed r162559: <http://trac.webkit.org/changeset/162559>