Bug 126601 – [CSS Shapes] First line gets incorrectly adjusted in shape-inside due to rounding in some polygons

• Component: CSS
• Status: RESOLVED FIXED

[Zoltan Horvath]

First line gets incorrectly adjusted due to rounding in some polygons.

[Zoltan Horvath]

Created attachment 220552 [details]
proposed patch

[Zoltan Horvath]

A little explanation: 

float firstPositiveWidth ends up used in 

shape.firstIncludedIntervalLogicalTop(m_shapeLineTop, LayoutSize(minSegmentWidth, m_lineHeight), newLineTop) (ShapeInsideInfo.cpp:99)

where minSegmentWidth becomes a LayoutUnit, which clamps floats to ints in LayoutUnit.h:11, so we our segment width becomes smaller than its actual width, which leads to the layout error. Ceiling the float in firstPositiveWidth solves the problem.

A following patch could be to clarify floats/ints in/call sites of firstPositiveWidth, but I'd do that separately.

[Bem Jones-Bey]

Comment on attachment 220552 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:467
> +            return ceilf(wordMeasurements[i].width);

Have you tested this with subpixel rendering both on and off?

[Bem Jones-Bey]

Comment on attachment 220552 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review

> LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.html:14
> +<div class="shape-inside">The</div>

I am worried that this test will be flaky on different platforms with different fonts. At the very least, I think you should specify a font size and font face if you can't put this together with something like Ahem.

[Zoltan Horvath]

Created attachment 220594 [details]
Patch

(In reply to comment #3)
> (From update of attachment 220552 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> 
> > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:467
> > +            return ceilf(wordMeasurements[i].width);
> 
> Have you tested this with subpixel rendering both on and off?

I haven't tested. As far as I know subpixel layout is enabled only on ELF and GTK. We don't have any tests which would fail due to this rounding (most of the tests uses fixed font width, or shapes without the requirement of first line adjusting, thus the new rounding won't change anything), otherwise it would have failed already.

(In reply to comment #4)
> (From update of attachment 220552 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> 
> > LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.html:14
> > +<div class="shape-inside">The</div>
> 
> I am worried that this test will be flaky on different platforms with different fonts. At the very least, I think you should specify a font size and font face if you can't put this together with something like Ahem.

Unfortunately, we can't use Ahem, since it has fixed width, thus it won't result a floating width. I specified the font in the test to make it clear.

I also added a short explanation about the problem to the changelog.

[Bem Jones-Bey]

(In reply to comment #5)
> Created an attachment (id=220594) [details]
> Patch
> 
> (In reply to comment #3)
> > (From update of attachment 220552 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> > 
> > > Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:467
> > > +            return ceilf(wordMeasurements[i].width);
> > 
> > Have you tested this with subpixel rendering both on and off?
> 
> I haven't tested. As far as I know subpixel layout is enabled only on ELF and GTK. We don't have any tests which would fail due to this rounding (most of the tests uses fixed font width, or shapes without the requirement of first line adjusting, thus the new rounding won't change anything), otherwise it would have failed already.

It is only enabled on ELF/Gtk for now, they are working on enabling it for Mac right now. Note that there is no rounding due to LayoutUnit when subpixel layout is turned on, which is why I am concerned that your fix may not be valid and your test may even fail when run on Gtk/EFL.

> 
> (In reply to comment #4)
> > (From update of attachment 220552 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=220552&action=review
> > 
> > > LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.html:14
> > > +<div class="shape-inside">The</div>
> > 
> > I am worried that this test will be flaky on different platforms with different fonts. At the very least, I think you should specify a font size and font face if you can't put this together with something like Ahem.
> 
> Unfortunately, we can't use Ahem, since it has fixed width, thus it won't result a floating width. I specified the font in the test to make it clear.

Have you tried to specify a floating point width for your Ahem characters? Or does that not work?

[Zoltan Horvath]

(In reply to comment #6)
> It is only enabled on ELF/Gtk for now, they are working on enabling it for Mac right now. Note that there is no rounding due to LayoutUnit when subpixel layout is turned on, which is why I am concerned that your fix may not be valid and your test may even fail when run on Gtk/EFL.

Good point.

> > (In reply to comment #4)
> Have you tried to specify a floating point width for your Ahem characters? Or does that not work?

I tried. I didn't work. :(

[Bem Jones-Bey]

(In reply to comment #7) 
> > > (In reply to comment #4)
> > Have you tried to specify a floating point width for your Ahem characters? Or does that not work?
> 
> I tried. I didn't work. :(

Well, even if you don't use Ahem, it would probably help if you did specify a font that you know has the metrics you want, as right now you are at the mercy of whatever default font is used by the test. I don't remember off the top of my head, but isn't there a list of fonts that one can rely on other than Ahem as being installed for the tests?

[Bear Travis]

Comment on attachment 220594 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220594&action=review

> LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit-expected.html:7
> +    height: 75px;

I am also concerned about the fragility of this test. If you need a font with characters with a specific height / width ratio, I can build one for you and you can embed it in the test as a data uri.
Will the test pass with subpixel layout enabled as well?

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:467
> +            return ceilf(wordMeasurements[i].width);

I don't think this should be adjusted here, but rather when the float -> LayoutUnit conversion takes place (in ShapeInsideInfo::adjustLogicalLineTop). Alternatively you could change adjustLogicalLineTop to take a LayoutUnit.

I think the correct behavior (including subpixel layout concerns) is to round up to the nearest value LayoutUnit can express. Without subpixel layout, this is in steps of 1 pixel. With subpixel layout, it's in steps of 1/64 of a pixel. You can use LayoutUnit::fromFloatCeil to get this value.

[Hans Muller]

(In reply to comment #2)
> A little explanation: 
> 
> float firstPositiveWidth ends up used in 
> 
> shape.firstIncludedIntervalLogicalTop(m_shapeLineTop, LayoutSize(minSegmentWidth, m_lineHeight), newLineTop) (ShapeInsideInfo.cpp:99)
> 
> where minSegmentWidth becomes a LayoutUnit, which clamps floats to ints in LayoutUnit.h:11, so we our segment width becomes smaller than its actual width, which leads to the layout error. Ceiling the float in firstPositiveWidth solves the problem.

Per our discussion on IRC: I think this problem would be better solved by making the firstIncludedIntervalLogicalTop() minLogicalIntervalSize parameter a float for all of the Shape subclasses.  The fact that it's implicitly converted to a LayoutUnit and then back to a float is just asking for trouble and appears to be unnecessary.  The minLogicalIntervalSize parameter's LayoutUnit type may just be a historical artifact.

[Zoltan Horvath]

Created attachment 220750 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 220750 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220750&action=review

> LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.html:13
> +    background-color: green;
> +    color: green;

Since the color of the text is green, and it's very tiny compared to the square, it can be hard to tell if this test fails with manual inspection. It would be nice if you could make the failure mode show some red, for example, by placing a green div on top of the div with the shape-inside so that you can make the text red. (and then put "You should not see any red" in the description)

> Source/WebCore/rendering/shapes/Shape.h:81
> +    virtual bool firstIncludedIntervalLogicalTop(LayoutUnit minLogicalIntervalTop, const float minLogicalWidth, const LayoutUnit logicalHeight, LayoutUnit& result) const = 0;

Why not have this take a FloatSize?

[Zoltan Horvath]

Created attachment 220779 [details]
Patch

(In reply to comment #12)
> (From update of attachment 220750 [details])
> Since the color of the text is green, and it's very tiny compared to the square, it can be hard to tell if this test fails with manual inspection. It would be nice if you could make the failure mode show some red, for example, by placing a green div on top of the div with the shape-inside so that you can make the text red. (and then put "You should not see any red" in the description)

I increased the size of the content, so it's much bigger now. I also painted to red and added green overlay. There is no way to miss it now, if it fails. I also updated the test description.

> > Source/WebCore/rendering/shapes/Shape.h:81
> > +    virtual bool firstIncludedIntervalLogicalTop(LayoutUnit minLogicalIntervalTop, const float minLogicalWidth, const LayoutUnit logicalHeight, LayoutUnit& result) const = 0;
> 
> Why not have this take a FloatSize?

I modified to use FloatSize.

Thanks for looking into.

[Zoltan Horvath]

Created attachment 220780 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 220780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220780&action=review

r=me, even if you don't feel like updating for the nit on the test.

> LayoutTests/fast/shapes/shape-inside/shape-inside-polygon-rounded-first-fit.html:15
> +    background-color: green;

Nit: I'd delete this line.

[Zoltan Horvath]

Created attachment 220785 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 220785 [details]
Patch

Clearing flags on attachment: 220785

Committed r161604: <http://trac.webkit.org/changeset/161604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.