Changeset 180623 in webkit


Ignore:
Timestamp:
Feb 25, 2015 2:53:57 AM (9 years ago)
Author:
svillar@igalia.com
Message:

[CSS Grid Layout] Tracks growing beyond limits when they should not
​https://bugs.webkit.org/show_bug.cgi?id=140883

Reviewed by Darin Adler.

Source/WebCore:

Our track sizing algorithm implementation incorrectly grew some
tracks beyond limits when handling min-content and max-content
base sizes. In those cases we should only grow "any affected track
that happens to also have an intrinsic max track sizing function"
or all of them if there are none.

The problem was that we're using a vector of indexes pointing to
the vector with the list of affected tracks. Those indexes become
easily incorrect because the first thing we do in
distributeSpaceToTracks() is to sort the vector with the affected
tracks by growth potential.

Instead of that we now pass a vector with pointers to the list of
tracks allowed to grow over the limits. The size changes are now
directly stored in the GridTracks (it's called plannedSize)
instead of in a separate vector, so we don't need to worry about
tracks being rearranged (and we also get rid of the ugly vector of
indexes pointing to another vector).

  • rendering/RenderGrid.cpp:

(WebCore::GridTrack::plannedSize): New member with getter/setter.
(WebCore::RenderGrid::computeUsedBreadthOfGridTracks):
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
Pass a setXXX() function instead of a growXXX() one to ForItems().
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForItems):
Renamed additionalBreadthSpace to extraSpace which is the term
used by specs.
(WebCore::RenderGrid::distributeSpaceToTracks): Use GridTrack's
plannedSize instead of the old distribution vector.
(WebCore::GridTrack::growBaseSize): Deleted.
(WebCore::GridTrack::growGrowthLimit): Deleted.

  • rendering/RenderGrid.h:

LayoutTests:

  • fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt:
  • fast/css-grid-layout/grid-content-sized-columns-resolution.html:
Location:
trunk
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r180621 r180623  
     12015-02-25  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [CSS Grid Layout] Tracks growing beyond limits when they should not
     4        https://bugs.webkit.org/show_bug.cgi?id=140883
     5
     6        Reviewed by Darin Adler.
     7
     8        * fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt:
     9        * fast/css-grid-layout/grid-content-sized-columns-resolution.html:
     10
    1112015-02-25  Joanmarie Diggs  <jdiggs@igalia.com>
    212

  • trunk/LayoutTests/fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt

    r180142 r180623  
    4343PASS window.getComputedStyle(gridMinContentFixedAndFixedFixedAndAuto, '').getPropertyValue('-webkit-grid-template-columns') is "20px 20px 60px"
    4444PASS window.getComputedStyle(gridAutoAndFixedFixedAndMaxContentFixed, '').getPropertyValue('-webkit-grid-template-columns') is "70px 20px 60px"
     45PASS window.getComputedStyle(gridMaxContentAndMaxContentFixedAndMaxContent, '').getPropertyValue('-webkit-grid-template-columns') is "70px 20px 50px"
     46PASS window.getComputedStyle(gridAutoAndMinContentFixedAndMinContent, '').getPropertyValue('-webkit-grid-template-columns') is "55px 30px 65px"
    4547PASS successfullyParsed is true
    4648
    … …  
    130132XXXX XXXX
    131133XXXXXXXXXXXXXXX
     134X X X
     135X X
     136XX XX XX XX XX
     137XX
     138XXXXXXXXXXXXXXX
     139XXX XXX

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

    r180142 r180623  
    4848    -webkit-grid-template-columns: auto minmax(20px, 30px) minmax(-webkit-max-content, 20px);
    4949}
     50.gridMaxContentAndMaxContentFixedAndMaxContent {
     51    -webkit-grid-template-columns: -webkit-max-content minmax(-webkit-max-content, 20px) -webkit-max-content;
     52}
     53.gridAutoAndMinContentFixedAndMinContent {
     54    -webkit-grid-template-columns: auto minmax(-webkit-min-content, 30px) -webkit-min-content;
     55}
     56
    5057</style>
    5158<script src="../../resources/js-test.js"></script>
    … …  
    293300        <div class="firstRowBothColumn">XXXXXXXXXXXXXXX</div>
    294301    </div>
     302</div>
     303
     304<div class="grid gridMaxContentAndMaxContentFixedAndMaxContent" id="gridMaxContentAndMaxContentFixedAndMaxContent">
     305    <div class="firstRowFirstColumn">X X X</div>
     306    <div style="-webkit-grid-row: 1; -webkit-grid-column: 3;">X X</div>
     307    <div class="firstRowBothColumn">XX XX XX XX XX</div>
     308</div>
     309
     310<div class="grid gridAutoAndMinContentFixedAndMinContent" id="gridAutoAndMinContentFixedAndMinContent">
     311    <div class="firstRowFirstColumn">XX</div>
     312    <div class="firstRowBothColumn">XXXXXXXXXXXXXXX</div>
     313    <div style="-webkit-grid-row: 1; -webkit-grid-column: 3;">XXX XXX</div>
    295314</div>
    296315
    … …  
    348367testGridColumnsValues("gridMinContentFixedAndFixedFixedAndAuto", "20px 20px 60px");
    349368testGridColumnsValues("gridAutoAndFixedFixedAndMaxContentFixed", "70px 20px 60px");
     369testGridColumnsValues("gridMaxContentAndMaxContentFixedAndMaxContent", "70px 20px 50px");
     370testGridColumnsValues("gridAutoAndMinContentFixedAndMinContent", "55px 30px 65px");
    350371</script>
    351372</body>

  • trunk/Source/WebCore/ChangeLog

    r180621 r180623  
     12015-02-25  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [CSS Grid Layout] Tracks growing beyond limits when they should not
     4        https://bugs.webkit.org/show_bug.cgi?id=140883
     5
     6        Reviewed by Darin Adler.
     7
     8        Our track sizing algorithm implementation incorrectly grew some
     9        tracks beyond limits when handling min-content and max-content
     10        base sizes. In those cases we should only grow "any affected track
     11        that happens to also have an intrinsic max track sizing function"
     12        or all of them if there are none.
     13
     14        The problem was that we're using a vector of indexes pointing to
     15        the vector with the list of affected tracks. Those indexes become
     16        easily incorrect because the first thing we do in
     17        distributeSpaceToTracks() is to sort the vector with the affected
     18        tracks by growth potential.
     19
     20        Instead of that we now pass a vector with pointers to the list of
     21        tracks allowed to grow over the limits. The size changes are now
     22        directly stored in the GridTracks (it's called plannedSize)
     23        instead of in a separate vector, so we don't need to worry about
     24        tracks being rearranged (and we also get rid of the ugly vector of
     25        indexes pointing to another vector).
     26
     27        * rendering/RenderGrid.cpp:
     28        (WebCore::GridTrack::plannedSize): New member with getter/setter.
     29        (WebCore::RenderGrid::computeUsedBreadthOfGridTracks):
     30        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
     31        Pass a setXXX() function instead of a growXXX() one to ForItems().
     32        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForItems):
     33        Renamed additionalBreadthSpace to extraSpace which is the term
     34        used by specs.
     35        (WebCore::RenderGrid::distributeSpaceToTracks): Use GridTrack's
     36        plannedSize instead of the old distribution vector.
     37        (WebCore::GridTrack::growBaseSize): Deleted.
     38        (WebCore::GridTrack::growGrowthLimit): Deleted.
     39        * rendering/RenderGrid.h:
     40
    1412015-02-25  Joanmarie Diggs  <jdiggs@igalia.com>
    242

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

    r180567 r180623  
    6969    }
    7070
    71     void growBaseSize(LayoutUnit growth)
    72     {
    73         ASSERT(growth >= 0);
    74         m_baseSize += growth;
    75         ensureGrowthLimitIsBiggerThanBaseSize();
    76     }
    77 
    78     void growGrowthLimit(LayoutUnit growth)
    79     {
    80         ASSERT(growth >= 0);
    81         if (m_growthLimit == infinity)
    82             m_growthLimit = m_baseSize + growth;
    83         else
    84             m_growthLimit += growth;
    85 
    86         ASSERT(m_growthLimit >= m_baseSize);
    87     }
    88 
    8971    bool growthLimitIsInfinite() const
    9072    {
    … …  
    9880    }
    9981
     82    LayoutUnit& plannedSize()
     83    {
     84        return m_plannedSize;
     85    }
     86
    10087private:
    10188    bool isGrowthLimitBiggerThanBaseSize() const { return growthLimitIsInfinite() || m_growthLimit >= m_baseSize; }
    … …  
    10996    LayoutUnit m_baseSize { 0 };
    11097    LayoutUnit m_growthLimit { 0 };
     98    LayoutUnit m_plannedSize { 0 };
    11199};
    112100
    … …  
    220208
    221209    // Performance optimization: hold onto these Vectors until the end of Layout to avoid repeated malloc / free.
    222     Vector<LayoutUnit> distributeTrackVector;
    223210    Vector<GridTrack*> filteredTracks;
    224     Vector<unsigned> growAboveMaxBreadthTrackIndexes;
     211    Vector<GridTrack*> growBeyondGrowthLimitsTracks;
    225212    Vector<GridItemWithSpan> itemsSortedByIncreasingSpan;
    226213};
    … …  
    379366            tracksForDistribution[i] = tracks.data() + i;
    380367
    381         distributeSpaceToTracks(tracksForDistribution, nullptr, &GridTrack::baseSize, &GridTrack::growBaseSize, sizingData, availableLogicalSpace);
     368        distributeSpaceToTracks(tracksForDistribution, nullptr, &GridTrack::baseSize, &GridTrack::setBaseSize, availableLogicalSpace);
    382369    } else {
    383370        for (auto& track : tracks)
    … …  
    638625
    639626    for (auto& itemWithSpan : sizingData.itemsSortedByIncreasingSpan) {
    640         resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::growBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth);
    641         resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMaxContentMinTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::baseSize, &GridTrack::growBaseSize, &GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth);
    642         resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMinOrMaxContentMaxTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::growthLimitIfNotInfinite, &GridTrack::growGrowthLimit);
    643         resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMaxContentMaxTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::growthLimitIfNotInfinite, &GridTrack::growGrowthLimit);
     627        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMinOrMaxContentMinTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth);
     628        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMaxContentMinTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::baseSize, &GridTrack::setBaseSize, &GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth);
     629        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMinOrMaxContentMaxTrackBreadth, &RenderGrid::minContentForChild, &GridTrack::growthLimitIfNotInfinite, &GridTrack::setGrowthLimit);
     630        resolveContentBasedTrackSizingFunctionsForItems(direction, sizingData, itemWithSpan, &GridTrackSize::hasMaxContentMaxTrackBreadth, &RenderGrid::maxContentForChild, &GridTrack::growthLimitIfNotInfinite, &GridTrack::setGrowthLimit);
    644631    }
    645632
    … …  
    675662
    676663    sizingData.filteredTracks.shrink(0);
    677     sizingData.growAboveMaxBreadthTrackIndexes.shrink(0);
     664    sizingData.growBeyondGrowthLimitsTracks.shrink(0);
    678665    for (GridResolvedPosition trackIndex = initialTrackPosition; trackIndex <= finalTrackPosition; ++trackIndex) {
    679666        const GridTrackSize& trackSize = gridTrackSize(direction, trackIndex.toInt());
    … …  
    684671        sizingData.filteredTracks.append(&track);
    685672
    686         if (growAboveMaxBreadthFilterFunction && (trackSize.*growAboveMaxBreadthFilterFunction)())
    687             sizingData.growAboveMaxBreadthTrackIndexes.append(sizingData.filteredTracks.size() - 1);
     673        if (!growAboveMaxBreadthFilterFunction || (trackSize.*growAboveMaxBreadthFilterFunction)())
     674            sizingData.growBeyondGrowthLimitsTracks.append(&track);
    688675    }
    689676
    … …  
    691678        return;
    692679
    693     LayoutUnit additionalBreadthSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks);
     680    LayoutUnit extraSpace = (this->*sizingFunction)(gridItemWithSpan.gridItem(), direction, sizingData.columnTracks);
    694681    for (GridResolvedPosition trackPositionForSpace = initialTrackPosition; trackPositionForSpace <= finalTrackPosition; ++trackPositionForSpace) {
    695682        GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackPositionForSpace.toInt()] : sizingData.rowTracks[trackPositionForSpace.toInt()];
    696         additionalBreadthSpace -= (track.*trackGetter)();
    697     }
    698 
    699     // Specs mandate to floor additionalBreadthSpace (extra-space in specs) to 0. Instead we directly avoid the function
    700     // call in those cases as it will be a noop in terms of track sizing.
    701     if (additionalBreadthSpace > 0)
    702         distributeSpaceToTracks(sizingData.filteredTracks, &sizingData.growAboveMaxBreadthTrackIndexes, trackGetter, trackGrowthFunction, sizingData, additionalBreadthSpace);
     683        extraSpace -= (track.*trackGetter)();
     684    }
     685
     686    // Specs mandate to floor extraSpace to 0. Instead we directly avoid the function call in those cases as it will be
     687    // a noop in terms of track sizing.
     688    if (extraSpace > 0) {
     689        auto* tracksToGrowBeyondGrowthLimits = sizingData.growBeyondGrowthLimitsTracks.isEmpty() ? &sizingData.filteredTracks : &sizingData.growBeyondGrowthLimitsTracks;
     690        distributeSpaceToTracks(sizingData.filteredTracks, tracksToGrowBeyondGrowthLimits, trackGetter, trackGrowthFunction, extraSpace);
     691    }
    703692}
    704693
    … …  
    716705}
    717706
    718 void RenderGrid::distributeSpaceToTracks(Vector<GridTrack*>& tracks, Vector<unsigned>* growAboveMaxBreadthTrackIndexes, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, GridSizingData& sizingData, LayoutUnit& availableLogicalSpace)
     707void RenderGrid::distributeSpaceToTracks(Vector<GridTrack*>& tracks, const Vector<GridTrack*>* growBeyondGrowthLimitsTracks, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, LayoutUnit& availableLogicalSpace)
    719708{
    720709    ASSERT(availableLogicalSpace > 0);
    … …  
    722711
    723712    unsigned tracksSize = tracks.size();
    724     sizingData.distributeTrackVector.resize(tracksSize);
    725713
    726714    for (unsigned i = 0; i < tracksSize; ++i) {
    727715        GridTrack& track = *tracks[i];
    728         const LayoutUnit& trackBreadth = (tracks[i]->*trackGetter)();
     716        const LayoutUnit& trackBreadth = (track.*trackGetter)();
    729717        bool infiniteGrowthPotential = track.growthLimitIsInfinite();
    730718        LayoutUnit trackGrowthPotential = infiniteGrowthPotential ? track.growthLimit() : track.growthLimit() - trackBreadth;
    731         sizingData.distributeTrackVector[i] = trackBreadth;
     719        track.plannedSize() = trackBreadth;
    732720        // Let's avoid computing availableLogicalSpaceShare as much as possible as it's a hot spot in performance tests.
    733721        if (trackGrowthPotential > 0 || infiniteGrowthPotential) {
    … …  
    735723            LayoutUnit growthShare = infiniteGrowthPotential ? availableLogicalSpaceShare : std::min(availableLogicalSpaceShare, trackGrowthPotential);
    736724            ASSERT_WITH_MESSAGE(growthShare >= 0, "We should never shrink any grid track or else we can't guarantee we abide by our min-sizing function. We can still have 0 as growthShare if the amount of tracks greatly exceeds the availableLogicalSpace.");
    737             sizingData.distributeTrackVector[i] += growthShare;
     725            track.plannedSize() += growthShare;
    738726            availableLogicalSpace -= growthShare;
    739727        }
    740728    }
    741729
    742     if (availableLogicalSpace > 0 && growAboveMaxBreadthTrackIndexes) {
    743         unsigned indexesSize = growAboveMaxBreadthTrackIndexes->size();
    744         unsigned tracksGrowingAboveMaxBreadthSize = indexesSize ? indexesSize : tracksSize;
    745         // If we have a non-null empty vector of track indexes to grow above max breadth means that we should grow all
    746         // affected tracks.
    747         for (unsigned i = 0; i < tracksGrowingAboveMaxBreadthSize; ++i) {
    748             LayoutUnit growthShare = availableLogicalSpace / (tracksGrowingAboveMaxBreadthSize - i);
    749             unsigned distributeTrackIndex = indexesSize ? growAboveMaxBreadthTrackIndexes->at(i) : i;
    750             sizingData.distributeTrackVector[distributeTrackIndex] += growthShare;
     730    if (availableLogicalSpace > 0 && growBeyondGrowthLimitsTracks) {
     731        unsigned tracksGrowingBeyondGrowthLimitsSize = growBeyondGrowthLimitsTracks->size();
     732        for (unsigned i = 0; i < tracksGrowingBeyondGrowthLimitsSize; ++i) {
     733            GridTrack* track = growBeyondGrowthLimitsTracks->at(i);
     734            LayoutUnit growthShare = availableLogicalSpace / (tracksGrowingBeyondGrowthLimitsSize - i);
     735            track->plannedSize() += growthShare;
    751736            availableLogicalSpace -= growthShare;
    752737        }
    753738    }
    754739
    755     for (unsigned i = 0; i < tracksSize; ++i) {
    756         LayoutUnit growth = sizingData.distributeTrackVector[i] - (tracks[i]->*trackGetter)();
    757         if (growth >= 0)
    758             (tracks[i]->*trackGrowthFunction)(growth);
    759     }
     740    for (auto* track : tracks)
     741        (track->*trackGrowthFunction)(track->plannedSize());
    760742}
    761743

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

    r180142 r180623  
    9494    void resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection, const GridCoordinate&, RenderBox& gridItem, GridTrack&, Vector<GridTrack>& columnTracks);
    9595    void resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection, GridSizingData&, GridItemWithSpan&, FilterFunction, SizingFunction, AccumulatorGetter, AccumulatorGrowFunction, FilterFunction growAboveMaxBreadthFilterFunction = nullptr);
    96     void distributeSpaceToTracks(Vector<GridTrack*>&, Vector<unsigned>* growAboveMaxBreadthTrackIndexes, AccumulatorGetter, AccumulatorGrowFunction, GridSizingData&, LayoutUnit& availableLogicalSpace);
     96    void distributeSpaceToTracks(Vector<GridTrack*>&, const Vector<GridTrack*>* growBeyondGrowthLimitsTracks, AccumulatorGetter, AccumulatorGrowFunction, LayoutUnit& availableLogicalSpace);
    9797
    9898    double computeNormalizedFractionBreadth(Vector<GridTrack>&, const GridSpan& tracksSpan, GridTrackSizingDirection, LayoutUnit availableLogicalSpace) const;
Note: See TracChangeset for help on using the changeset viewer.