Bug 153206 – [css-grid][css-align] justify-self stretch is not applied for img elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Created attachment 269218 [details]
Test case to reproduce the issue

When using <img> elements as grid items, the content image is not stretched even the justify-self property indicates so; it works as expected in the column axis (align-self), though.

[Javier Fernandez]

Actually this issue is affecting any Replaced Box being defined as a grid item.

[Javier Fernandez]

Created attachment 269219 [details]
Test case to reproduce the issue

[Javier Fernandez]

Created attachment 269222 [details]
Patch

[Build Bot]

Comment on attachment 269222 [details]
Patch

Attachment 269222 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/708288

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/garbage-collection.html

[Build Bot]

Created attachment 269224 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Javier Fernandez]

Created attachment 269226 [details]
Patch

[Build Bot]

Comment on attachment 269226 [details]
Patch

Attachment 269226 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/708592

New failing tests:
imported/w3c/web-platform-tests/streams-api/readable-streams/count-queuing-strategy-integration.html

[Build Bot]

Created attachment 269228 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 269226 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2525
> +// FIXME can/should move this inside specific layout classes (flex. grid)  ? Can we reactoring columnFlexItemHasStretchAlignment logic ?

Formatting errors: No spaces before "?", missing ":" after FIXME, missing capital of "can", instead of "Can we reactoring" it should be "Can we refactor".

> Source/WebCore/rendering/RenderBox.cpp:2528
> +    RenderBlock* cb = containingBlock();

What guarantees containingBlock can't be null? If we have a guarantee that it can't be null, why not have the function return a reference instead of a pointer?

Better to call this containingBlock rather than cb. Better to use a reference rather than a pointer since we don't handle null here.

    ASSERT(containingBlock()); // <comment explaining why it can't be null>
    auto& containingBlock = *this->containingBlock();

> Source/WebCore/rendering/RenderBox.cpp:2529
> +    const RenderStyle& styleToUse = style();

I think this local variable can also be named style, not sure what "to use" adds and since this is the same as the "style" function seems fine to use the name.

    auto& style = this->style();

> Source/WebCore/rendering/RenderBox.cpp:2531
> +    bool allowedToStretch = styleToUse.logicalWidth().isAuto() && !styleToUse.marginStart().isAuto() && !styleToUse.marginEnd().isAuto();

Could early exit based on this before computing containingBlock or hasPerpendicularContainingBlock.

    if (!style.logicalWidth().isAuto() || style.marginStart().isAuto() || style.marginEnd().isAuto())
        return false;

Or if you prefer the named boolean:

    if (!allowedToStretch)
        return false;

[Javier Fernandez]

Created attachment 269238 [details]
Patch

[Darin Adler]

Comment on attachment 269238 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2525
> +// FIXME: Can/Should we move this inside specific layout classes (flex. grid)? Can we reactor columnFlexItemHasStretchAlignment logic?

Typo: "reactor" here instead of "refactor".

> Source/WebCore/rendering/RenderBox.cpp:2531
> +    RenderBlock* cb = containingBlock();

As I mentioned before, in WebKit coding style the local variable should be named containingBlock, not cb.

[Javier Fernandez]

Created attachment 269260 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 269260 [details]
Patch

Clearing flags on attachment: 269260

Committed r195284: <http://trac.webkit.org/changeset/195284>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.