Changeset 142798 in webkit


Ignore:
Timestamp:
Feb 13, 2013 2:17:50 PM (11 years ago)
Author:
jchaffraix@webkit.org
Message:

[CSS Grid Layout] Adding or removing grid items doesn't properly recompute the track sizes
​https://bugs.webkit.org/show_bug.cgi?id=109100

Reviewed by Ojan Vafai.

Source/WebCore:

Test: fast/css-grid-layout/grid-item-removal-track-breadth-update.html

The test uncovered several bugs in our implementation that is fixed as part
of this change. They will be detailed below.

  • rendering/RenderGrid.cpp:

(WebCore::RenderGrid::logicalContentHeightForChild):
Added this function to share the code between minContentForChild and maxContentForChild.
Also forced a relayout in this case to avoid getting a wrong answer (e.g. the logical height
constrained by the previous layout's grid breadth).

(WebCore::RenderGrid::minContentForChild):
(WebCore::RenderGrid::maxContentForChild):
Updated to use logicalContentHeightForChild.

(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
Updated to match the specification and set max breadth to current breadth per the specification.
This made us over-grow some cases in the test.

(WebCore::RenderGrid::distributeSpaceToTracks):
Updated to match the specification and use an extra variable to do the intermediate spreading. Also removed
a now unneeded max. This fixes the case of multiple grid items in the same grid area that was completely broken.

(WebCore::RenderGrid::layoutGridItems):
Added a FIXME about always relaying out content sized tracks' children.

  • rendering/RenderGrid.h:

Added logicalContentHeightForChild.

LayoutTests:

  • fast/css-grid-layout/grid-item-addition-track-breadth-update-expected.txt: Added.
  • fast/css-grid-layout/grid-item-addition-track-breadth-update.html: Added.
  • fast/css-grid-layout/grid-item-removal-track-breadth-update-expected.txt: Added.
  • fast/css-grid-layout/grid-item-removal-track-breadth-update.html: Added.

New tests.

  • fast/css-grid-layout/resources/grid.css:

(.constrainedContainer):
(.unconstrainedContainer):
Added these class to share them with other tests.

  • fast/css-grid-layout/auto-content-resolution-columns.html:
  • fast/css-grid-layout/auto-content-resolution-rows.html:
  • fast/css-grid-layout/implicit-columns-auto-resolution.html:
  • fast/css-grid-layout/implicit-position-dynamic-change.html:
  • fast/css-grid-layout/implicit-rows-auto-resolution.html:
  • fast/css-grid-layout/minmax-max-content-resolution-columns.html:
  • fast/css-grid-layout/minmax-max-content-resolution-rows.html:
  • fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:
  • fast/css-grid-layout/minmax-min-content-column-resolution-rows.html:

Removed constrainedContainer definition as it was moved to grid.css.

Location:
trunk
Files:
4 added
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r142797 r142798  
     12013-02-13  Julien Chaffraix  <jchaffraix@webkit.org>
     2
     3        [CSS Grid Layout] Adding or removing grid items doesn't properly recompute the track sizes
     4        https://bugs.webkit.org/show_bug.cgi?id=109100
     5
     6        Reviewed by Ojan Vafai.
     7
     8        * fast/css-grid-layout/grid-item-addition-track-breadth-update-expected.txt: Added.
     9        * fast/css-grid-layout/grid-item-addition-track-breadth-update.html: Added.
     10        * fast/css-grid-layout/grid-item-removal-track-breadth-update-expected.txt: Added.
     11        * fast/css-grid-layout/grid-item-removal-track-breadth-update.html: Added.
     12        New tests.
     13
     14        * fast/css-grid-layout/resources/grid.css:
     15        (.constrainedContainer):
     16        (.unconstrainedContainer):
     17        Added these class to share them with other tests.
     18
     19        * fast/css-grid-layout/auto-content-resolution-columns.html:
     20        * fast/css-grid-layout/auto-content-resolution-rows.html:
     21        * fast/css-grid-layout/implicit-columns-auto-resolution.html:
     22        * fast/css-grid-layout/implicit-position-dynamic-change.html:
     23        * fast/css-grid-layout/implicit-rows-auto-resolution.html:
     24        * fast/css-grid-layout/minmax-max-content-resolution-columns.html:
     25        * fast/css-grid-layout/minmax-max-content-resolution-rows.html:
     26        * fast/css-grid-layout/minmax-min-content-column-resolution-columns.html:
     27        * fast/css-grid-layout/minmax-min-content-column-resolution-rows.html:
     28        Removed constrainedContainer definition as it was moved to grid.css.
     29
    1302013-02-13  Stephen Chenney  <schenney@chromium.org>
    231

  • trunk/LayoutTests/fast/css-grid-layout/auto-content-resolution-columns.html

    r141317 r142798  
    1010    -webkit-grid-columns: auto;
    1111    -webkit-grid-rows: 50px;
    12 }
    13 
    14 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    15 .constrainedContainer {
    16     width: 10px;
    17     height: 10px;
    1812}
    1913

  • trunk/LayoutTests/fast/css-grid-layout/auto-content-resolution-rows.html

    r141317 r142798  
    1010    -webkit-grid-columns: 50px;
    1111    -webkit-grid-rows: auto;
    12 }
    13 
    14 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    15 .constrainedContainer {
    16     width: 10px;
    17     height: 10px;
    1812}
    1913

  • trunk/LayoutTests/fast/css-grid-layout/implicit-columns-auto-resolution.html

    r141505 r142798  
    1010    /* -webkit-grid-columns is left unset so that the grid item's column is implicit. */
    1111    -webkit-grid-rows: 50px;
    12 }
    13 
    14 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    15 .constrainedContainer {
    16     width: 10px;
    17     height: 10px;
    1812}
    1913

  • trunk/LayoutTests/fast/css-grid-layout/implicit-position-dynamic-change.html

    r141963 r142798  
    1010    -webkit-grid-columns: 50px minmax(-webkit-min-content, 50px) minmax(-webkit-max-content, 50px) minmax(50px, -webkit-min-content);
    1111    -webkit-grid-rows: 70px minmax(-webkit-max-content, 70px) minmax(50px, -webkit-min-content) minmax(65px, -webkit-max-content);
    12 }
    13 
    14 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    15 .constrainedContainer {
    16     width: 10px;
    17     height: 10px;
    1812}
    1913

  • trunk/LayoutTests/fast/css-grid-layout/implicit-rows-auto-resolution.html

    r141505 r142798  
    1010    -webkit-grid-columns: 50px;
    1111    /* -webkit-grid-rows is left unset so that the grid item's row is implicit. */
    12 }
    13 
    14 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    15 .constrainedContainer {
    16     width: 10px;
    17     height: 10px;
    1812}
    1913

  • trunk/LayoutTests/fast/css-grid-layout/minmax-max-content-resolution-columns.html

    r140583 r142798  
    1515    -webkit-grid-columns: minmax(30px, -webkit-max-content);
    1616    -webkit-grid-rows: 20px;
    17 }
    18 
    19 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    20 .constrainedContainer {
    21     width: 10px;
    22     height: 10px;
    2317}
    2418

  • trunk/LayoutTests/fast/css-grid-layout/minmax-max-content-resolution-rows.html

    r140583 r142798  
    1515    -webkit-grid-columns: 20px;
    1616    -webkit-grid-rows: minmax(30px, -webkit-max-content);
    17 }
    18 
    19 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    20 .constrainedContainer {
    21     width: 10px;
    22     height: 10px;
    2317}
    2418

  • trunk/LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-columns.html

    r140480 r142798  
    1515    -webkit-grid-columns: minmax(30px, -webkit-min-content);
    1616    -webkit-grid-rows: 20px;
    17 }
    18 
    19 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    20 .constrainedContainer {
    21     width: 10px;
    22     height: 10px;
    2317}
    2418

  • trunk/LayoutTests/fast/css-grid-layout/minmax-min-content-column-resolution-rows.html

    r140480 r142798  
    1515    -webkit-grid-columns: 30px;
    1616    -webkit-grid-rows: minmax(30px, -webkit-min-content);
    17 }
    18 
    19 /* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
    20 .constrainedContainer {
    21     width: 10px;
    22     height: 10px;
    2317}
    2418

  • trunk/LayoutTests/fast/css-grid-layout/resources/grid.css

    r141787 r142798  
    4545}
    4646
     47/* This rule makes sure the container is smaller than any grid items to avoid distributing any extra logical space to them. */
     48.constrainedContainer {
     49    width: 10px;
     50    height: 10px;
     51}
     52
     53.unconstrainedContainer {
     54    width: 1000px;
     55    height: 1000px;
     56}
     57
    4758.verticalRL {
    4859    -webkit-writing-mode: vertical-rl;

  • trunk/Source/WebCore/ChangeLog

    r142796 r142798  
     12013-02-13  Julien Chaffraix  <jchaffraix@webkit.org>
     2
     3        [CSS Grid Layout] Adding or removing grid items doesn't properly recompute the track sizes
     4        https://bugs.webkit.org/show_bug.cgi?id=109100
     5
     6        Reviewed by Ojan Vafai.
     7
     8        Test: fast/css-grid-layout/grid-item-removal-track-breadth-update.html
     9
     10        The test uncovered several bugs in our implementation that is fixed as part
     11        of this change. They will be detailed below.
     12
     13        * rendering/RenderGrid.cpp:
     14        (WebCore::RenderGrid::logicalContentHeightForChild):
     15        Added this function to share the code between minContentForChild and maxContentForChild.
     16        Also forced a relayout in this case to avoid getting a wrong answer (e.g. the logical height
     17        constrained by the previous layout's grid breadth).
     18
     19        (WebCore::RenderGrid::minContentForChild):
     20        (WebCore::RenderGrid::maxContentForChild):
     21        Updated to use logicalContentHeightForChild.
     22
     23        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
     24        Updated to match the specification and set max breadth to current breadth per the specification.
     25        This made us over-grow some cases in the test.
     26
     27        (WebCore::RenderGrid::distributeSpaceToTracks):
     28        Updated to match the specification and use an extra variable to do the intermediate spreading. Also removed
     29        a now unneeded max. This fixes the case of multiple grid items in the same grid area that was completely broken.
     30
     31        (WebCore::RenderGrid::layoutGridItems):
     32        Added a FIXME about always relaying out content sized tracks' children.
     33
     34        * rendering/RenderGrid.h:
     35        Added logicalContentHeightForChild.
     36
    1372013-02-13  Adam Barth  <abarth@webkit.org>
    238

  • trunk/Source/WebCore/rendering/RenderGrid.cpp

    r141963 r142798  
    288288}
    289289
     290LayoutUnit RenderGrid::logicalContentHeightForChild(RenderBox* child, Vector<GridTrack>& columnTracks)
     291{
     292    // FIXME: We shouldn't force a layout every time this function is called but
     293    // 1) Return computeLogicalHeight's value if it's available. Unfortunately computeLogicalHeight
     294    // doesn't return if the logical height is available so would need to be changed.
     295    // 2) Relayout if the column track's used breadth changed OR the logical height is unavailable.
     296    if (!child->needsLayout())
     297        child->setNeedsLayout(true, MarkOnlyThis);
     298
     299    size_t columnTrack = resolveGridPosition(ForColumns, child);
     300    child->setOverrideContainingBlockContentLogicalWidth(columnTracks[columnTrack].m_usedBreadth);
     301    child->clearOverrideContainingBlockContentLogicalHeight();
     302    child->layout();
     303    return child->logicalHeight();
     304}
     305
    290306LayoutUnit RenderGrid::minContentForChild(RenderBox* child, TrackSizingDirection direction, Vector<GridTrack>& columnTracks)
    291307{
    … …  
    301317    }
    302318
    303     if (child->needsLayout()) {
    304         size_t columnTrack = resolveGridPosition(ForColumns, child);
    305         child->setOverrideContainingBlockContentLogicalWidth(columnTracks[columnTrack].m_usedBreadth);
    306         child->clearOverrideContainingBlockContentLogicalHeight();
    307         child->layout();
    308     }
    309     return child->logicalHeight();
     319    return logicalContentHeightForChild(child, columnTracks);
    310320}
    311321
    … …  
    323333    }
    324334
    325     if (child->needsLayout()) {
    326         size_t columnTrack = resolveGridPosition(ForColumns, child);
    327         child->setOverrideContainingBlockContentLogicalWidth(columnTracks[columnTrack].m_usedBreadth);
    328         child->clearOverrideContainingBlockContentLogicalHeight();
    329         child->layout();
    330     }
    331     return child->logicalHeight();
     335    return logicalContentHeightForChild(child, columnTracks);
    332336}
    333337
    … …  
    360364        if (maxTrackBreadth.isMaxContent())
    361365            resolveContentBasedTrackSizingFunctionsForItems(direction, columnTracks, rowTracks, i, &RenderGrid::maxContentForChild, &GridTrack::maxBreadthIfNotInfinite, &GridTrack::growMaxBreadth);
    362     }
    363 
    364     // FIXME: The spec says to update maxBreadth if it is Infinity.
     366
     367        if (track.m_maxBreadth == infinity)
     368            track.m_maxBreadth = track.m_usedBreadth;
     369    }
    365370}
    366371
    … …  
    392397
    393398    size_t tracksSize = tracks.size();
     399    Vector<LayoutUnit> updatedTrackBreadths(tracksSize);
     400
    394401    for (size_t i = 0; i < tracksSize; ++i) {
    395402        GridTrack& track = *tracks[i];
    396403        LayoutUnit availableLogicalSpaceShare = availableLogicalSpace / (tracksSize - i);
    397         // We never shrink the used breadth by clamping the difference between max and used breadth. The spec uses
    398         // 2 extra-variables and 2 extra iterations to ensure that we always grow our tracks (thus never going below
    399         // min-track). If we decide to follow it to the letter, we should remove this clamping.
    400         LayoutUnit growthShare = std::min(availableLogicalSpaceShare, std::max(LayoutUnit(0), track.m_maxBreadth - (track.*trackGetter)()));
    401         (track.*trackGrowthFunction)(growthShare);
     404        LayoutUnit trackBreadth = (tracks[i]->*trackGetter)();
     405        LayoutUnit growthShare = std::min(availableLogicalSpaceShare, track.m_maxBreadth - trackBreadth);
     406        updatedTrackBreadths[i] = trackBreadth + growthShare;
    402407        availableLogicalSpace -= growthShare;
    403408    }
    404409
    405     if (availableLogicalSpace <= 0)
    406         return;
    407 
    408     if (!tracksForGrowthAboveMaxBreadth)
    409         return;
    410 
    411     tracksSize = tracksForGrowthAboveMaxBreadth->size();
     410    if (availableLogicalSpace > 0 && tracksForGrowthAboveMaxBreadth) {
     411        tracksSize = tracksForGrowthAboveMaxBreadth->size();
     412        for (size_t i = 0; i < tracksSize; ++i) {
     413            LayoutUnit growthShare = availableLogicalSpace / (tracksSize - i);
     414            updatedTrackBreadths[i] += growthShare;
     415            availableLogicalSpace -= growthShare;
     416        }
     417    }
     418
    412419    for (size_t i = 0; i < tracksSize; ++i) {
    413         GridTrack& track = *tracksForGrowthAboveMaxBreadth->at(i);
    414         LayoutUnit growthShare = availableLogicalSpace / (tracksSize - i);
    415         (track.*trackGrowthFunction)(growthShare);
    416         availableLogicalSpace -= growthShare;
     420        LayoutUnit growth = updatedTrackBreadths[i] - (tracks[i]->*trackGetter)();
     421        if (growth >= 0)
     422            (tracks[i]->*trackGrowthFunction)(growth);
    417423    }
    418424}
    … …  
    451457        LayoutUnit oldOverrideContainingBlockContentLogicalHeight = child->hasOverrideContainingBlockLogicalHeight() ? child->overrideContainingBlockContentLogicalHeight() : LayoutUnit();
    452458
     459        // FIXME: For children in a content sized track, we clear the overrideContainingBlockContentLogicalHeight
     460        // in minContentForChild / maxContentForChild which means that we will always relayout the child.
    453461        if (oldOverrideContainingBlockContentLogicalWidth != columnTracks[columnTrack].m_usedBreadth || oldOverrideContainingBlockContentLogicalHeight != rowTracks[rowTrack].m_usedBreadth)
    454462            child->setNeedsLayout(true, MarkOnlyThis);

  • trunk/Source/WebCore/rendering/RenderGrid.h

    r141616 r142798  
    6868    size_t maximumIndexInDirection(TrackSizingDirection) const;
    6969
     70    LayoutUnit logicalContentHeightForChild(RenderBox*, Vector<GridTrack>&);
    7071    LayoutUnit minContentForChild(RenderBox*, TrackSizingDirection, Vector<GridTrack>& columnTracks);
    7172    LayoutUnit maxContentForChild(RenderBox*, TrackSizingDirection, Vector<GridTrack>& columnTracks);
Note: See TracChangeset for help on using the changeset viewer.