Bug 124784

Summary: [CSS Shapes] shape-inside rectangle layout can fail
Product: WebKit Reporter: Hans Muller <giles_joplin>
Component: CSSAssignee: Hans Muller <giles_joplin>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, esprehn+autocc, glenn, jonlee, kling, kondapallykalyan, rniwa, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 127404    
Bug Blocks:    
Attachments:
Description Flags
Test case.
none
Patch
none
Patch
none
Patch
commit-queue: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion none

Description Hans Muller 2013-11-22 11:02:35 PST
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().
Comment 1 Hans Muller 2013-11-26 16:10:47 PST
Created attachment 217909 [details]
Patch

Apply LayoutUnit::fromFloatCeil() consistently in RectangleShape::firstIncludedIntervalLogicalTop().
Comment 2 Andreas Kling 2013-11-27 11:28:12 PST
Comment on attachment 217909 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2013-11-27 12:14:29 PST
Comment on attachment 217909 [details]
Patch

Clearing flags on attachment: 217909

Committed r159821: <http://trac.webkit.org/changeset/159821>
Comment 4 WebKit Commit Bot 2013-11-27 12:14:31 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 zalan 2014-01-17 15:14:35 PST
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)
Comment 6 zalan 2014-01-17 15:16:55 PST
Created attachment 221497 [details]
Patch
Comment 7 zalan 2014-01-17 15:17:53 PST
The patch assumes that we don't actually need to ceil the float value to CSS px at layout time.
Comment 8 Darin Adler 2014-01-20 16:07:47 PST
Comment on attachment 221497 [details]
Patch

Not sure I am qualified to review this. Maybe Simon Fraser or another layout expert needs to (Hyatt?).
Comment 9 zalan 2014-01-20 16:11:09 PST
(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.
Comment 10 Hans Muller 2014-01-21 12:01:41 PST
(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.
Comment 11 zalan 2014-01-21 12:26:25 PST
(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.
Comment 12 Hans Muller 2014-01-21 14:25:55 PST
(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.
Comment 13 Hans Muller 2014-01-21 14:26:55 PST
(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.
Comment 14 zalan 2014-01-21 14:31:31 PST
(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.
Comment 15 zalan 2014-01-21 21:20:23 PST
Created attachment 221827 [details]
Patch
Comment 16 zalan 2014-01-21 21:26:41 PST
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)
Comment 17 Build Bot 2014-01-22 08:06:35 PST
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
Comment 18 Build Bot 2014-01-22 08:06:38 PST
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
Comment 19 zalan 2014-01-22 08:08:28 PST
(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
Comment 20 Build Bot 2014-01-22 08:28:42 PST
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
Comment 21 Build Bot 2014-01-22 08:28:45 PST
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
Comment 22 Hans Muller 2014-01-22 09:10:20 PST
(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.
Comment 23 Build Bot 2014-01-22 09:27:28 PST
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
Comment 24 Build Bot 2014-01-22 09:27:31 PST
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
Comment 25 WebKit Commit Bot 2014-01-22 14:28:22 PST
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
Comment 26 zalan 2014-01-22 14:51:49 PST
Committed r162559: <http://trac.webkit.org/changeset/162559>