Bug 132132 – [CSS Shapes] shape-outside polygon can fail when line-top intersects a vertex

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

Bear reported this.

The attached test case demonstrates the problem (also: http://jsfiddle.net/52UMu/2/).

[Hans Muller]

Created attachment 230089 [details]
Test case.

The test case can also be found here: http://jsfiddle.net/52UMu/2/

[Hans Muller]

Created attachment 230210 [details]
Patch

ShapeInterval now distinguishes between x1==x2 - isEmpty() and x1,x2 haven't been set yet - isUndefined().

The polygon algorithm for computing excluded intervals now ignores horizontal edges. It also ignores edges whose lower vertex matches the top of the line, if the edge's Y direction is upwards (away from the top of the line). The rationale for this was explained here: http://hansmuller-webkit.blogspot.com/2012/11/revised-horizontal-box-algorithm.html

[Bem Jones-Bey]

Comment on attachment 230210 [details]
Patch

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

IT would nice to see a test for the case where the top of a line overlaps the bottom of the shape, if it's not too much of a pain to create.

> Source/WebCore/ChangeLog:3
> +        [CSS Shapes] shape-outside polygon fails when first vertex is 0,0

Nit: it would be nice if this was changed to better explain the issue, since it's not just when the first vertex is 0,0.

> Source/WebCore/rendering/shapes/ShapeInterval.h:43
> +        : m_x1(-1)
> +        , m_x2(-2)

I wish there was a way to make these look less like magic numbers (as far as I can tell, any numbers would work as long as x2 < x1).

> Source/WebCore/rendering/shapes/ShapeInterval.h:81
>      void setX1(T x1)
>      {
> -        ASSERT(m_x2 >= x1);
> -        m_x1 = x1;
> +        if (isUndefined()) {
> +            m_x1 = x1;
> +            m_x2 = x1;
> +        } else {
> +            ASSERT(m_x2 >= x1);
> +            m_x1 = x1;
> +        }
>      }
>  
>      void setX2(T x2)
>      {
> -        ASSERT(x2 >= m_x1);
> -        m_x2 = x2;
> +        if (isUndefined()) {
> +            m_x1 = x2;
> +            m_x2 = x2;
> +        } else {
> +            ASSERT(x2 >= m_x1);
> +            m_x2 = x2;
> +        }
>      }

With the addition of the undefined state, these two methods have become much more complex and potentially confusing (setting one can set the other?!?). It doesn't look like they're used anywhere, can we just remove them?

[Bem Jones-Bey]

Comment on attachment 230210 [details]
Patch

Looks like the test covers more than I had thought.

r=me, please to remove the setX methods before landing. :-)

[Hans Muller]

(In reply to comment #3)
> Nit: it would be nice if this was changed to better explain the issue, since it's not just when the first vertex is 0,0.

The original bug description was just a guess about the scope of the problem. 

The real source of the problem was that polygon edges that only overlapped the vertical line interval because the top of the line interval was coincident with the bottom vertex of the edge were included in the exclusion boundary.  

A simple way to visualize the problem is to consider a 100x100 square polygon at 0,0 and line-height=100.  We want the first line, whose vertical interval is 0,100 to be excluded by the polygon.  The second line, whose vertical interval is 100,200 should not exclude the polygon, despite the fact that top of the line is coincident with the bottom of the polygon.  Similarly, if the polygon's origin is 0,100, then the first line would not be excluded by the polygon, but the second line would (and the third line would not).

[Hans Muller]

Created attachment 230375 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 230375 [details]
Patch

Clearing flags on attachment: 230375

Committed r167931: <http://trac.webkit.org/changeset/167931>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.