Bug 153206 - [css-grid][css-align] justify-self stretch is not applied for img elements
Summary: [css-grid][css-align] justify-self stretch is not applied for img elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords: BlinkMergeCandidate
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2016-01-18 06:56 PST by Javier Fernandez
Modified: 2016-01-19 03:36 PST (History)
10 users (show)

See Also:


Attachments
Test case to reproduce the issue (230 bytes, text/html)
2016-01-18 06:56 PST, Javier Fernandez
no flags Details
Test case to reproduce the issue (332 bytes, text/html)
2016-01-18 07:00 PST, Javier Fernandez
no flags Details
Patch (9.79 KB, patch)
2016-01-18 07:53 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-yosemite (773.91 KB, application/zip)
2016-01-18 08:55 PST, Build Bot
no flags Details
Patch (9.79 KB, patch)
2016-01-18 09:28 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (767.63 KB, application/zip)
2016-01-18 10:26 PST, Build Bot
no flags Details
Patch (9.89 KB, patch)
2016-01-18 15:03 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2016-01-19 01:32 PST, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2016-01-18 06:56:34 PST
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.
Comment 1 Javier Fernandez 2016-01-18 06:57:13 PST
Actually this issue is affecting any Replaced Box being defined as a grid item.
Comment 2 Javier Fernandez 2016-01-18 07:00:59 PST
Created attachment 269219 [details]
Test case to reproduce the issue
Comment 3 Javier Fernandez 2016-01-18 07:53:16 PST
Created attachment 269222 [details]
Patch
Comment 4 Build Bot 2016-01-18 08:55:48 PST
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
Comment 5 Build Bot 2016-01-18 08:55:51 PST
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
Comment 6 Javier Fernandez 2016-01-18 09:28:59 PST
Created attachment 269226 [details]
Patch
Comment 7 Build Bot 2016-01-18 10:26:41 PST
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
Comment 8 Build Bot 2016-01-18 10:26:43 PST
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
Comment 9 Darin Adler 2016-01-18 14:32:03 PST
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;
Comment 10 Javier Fernandez 2016-01-18 15:03:31 PST
Created attachment 269238 [details]
Patch
Comment 11 Darin Adler 2016-01-18 18:04:13 PST
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.
Comment 12 Javier Fernandez 2016-01-19 01:32:09 PST
Created attachment 269260 [details]
Patch
Comment 13 WebKit Commit Bot 2016-01-19 03:36:23 PST
Comment on attachment 269260 [details]
Patch

Clearing flags on attachment: 269260

Committed r195284: <http://trac.webkit.org/changeset/195284>
Comment 14 WebKit Commit Bot 2016-01-19 03:36:27 PST
All reviewed patches have been landed.  Closing bug.