[CSS Grid layout] Initial position in span not correctly computed sometimes

• Component: CSS
• Status: RESOLVED FIXED

[Sergio Villar Senin]

We're using std::lower_bound to find the first line before the opposite resolved position. The returned iterator might point to a resolved position which is actually greater than the opposite position (since lower_bound returns the first non less than). In those cases we need to move the iterator to the previous line so that the span can be correctly computed.

For example, let's imagine that we try to compute the span for a given named line which is defined in these tree positions (tracks)

[2, 5, 8]

Suppose that the opposite final position to the span was computed as 7. Then std::lower_bound will return an iterator pointing to 8. We're looking for the first position before 7, and obviously 8 is after, so we have to move the iterator to the previous position which would be 5. That's because how std::lower_bound works, if the iterator points to a value <= then we use it, but if it's > then we have to use the previous one.

[Sergio Villar Senin]

Created attachment 226497 [details]
Patch

[Darin Adler]

Comment on attachment 226497 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:1035
>      const size_t* firstLineBeforeOppositePosition = std::lower_bound(gridLines.begin(), gridLines.end(), resolvedOppositePosition);
> -    if (firstLineBeforeOppositePosition != gridLines.end())
> +    if (firstLineBeforeOppositePosition != gridLines.end()) {
> +        if (*firstLineBeforeOppositePosition > resolvedOppositePosition && firstLineBeforeOppositePosition != gridLines.begin())
> +            --firstLineBeforeOppositePosition;
> +
>          firstLineBeforeOppositePositionIndex = firstLineBeforeOppositePosition - gridLines.begin();
> +    }

Awkward. Makes me think we should put this whole thing into a helper function rather than invoking lower_bound directly in-line.

> LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution-expected.txt:26
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS
> +PASS

Not great test output here. Worth thinking in the future about things we can include in tests to make it clear what we are testing rather than just a lot of PASS/FAIL. Helpful in many cases, for example, when some tests are failing it helps you see which ones they are.

[Darin Adler]

Naming that helper function would help clarify what this code is doing at a mechanical level.

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 226497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226497&action=review
> > LayoutTests/fast/css-grid-layout/grid-item-named-grid-line-resolution-expected.txt:26
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> > +PASS
> 
> Not great test output here. Worth thinking in the future about things we can include in tests to make it clear what we are testing rather than just a lot of PASS/FAIL. Helpful in many cases, for example, when some tests are failing it helps you see which ones they are.

Although I agree that the output is far from optimal, in this case the testing code is the one that is very descriptive. We're using checkLayout() to check the offsets and sizes of grid items. That allows us to test with great detail the positioning of grid items. In case of error, that JS helper prints the reason why the test fails (like expected 30 for width but found 50) and also the original testing code so it's quite easy to check the reason of the failure.

[Sergio Villar Senin]

Committed r165612: <http://trac.webkit.org/changeset/165612>

[Darin Adler]

(In reply to comment #4)
> that JS helper prints the reason why the test fails (like expected 30 for width but found 50) and also the original testing code so it's quite easy to check the reason of the failure.

If it just printed slightly more of the same when passing, I’d be happy.

When a test fails it’s good to be reminded of the similar tests that are passing.