Changeset 179987 in webkit


Ignore:
Timestamp:
Feb 12, 2015 8:14:16 AM (9 years ago)
Author:
svillar@igalia.com
Message:

[CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
​https://bugs.webkit.org/show_bug.cgi?id=140763

Reviewed by Antti Koivisto.

Source/WebCore:

Content sized tracks with non-spanning grid items were not
properly sized because the growth limit was sometimes infinity
(-1) after calling resolveContentBasedTrackSizingFunctions() when
it should not. This patch adds an special initialization phase for
non-spanning grid items as the new track sizing algorithm
describes.

Granted, that was handled in the old algorithm in
distributeSpaceToTracks() as a special case. The problem is that
it regressed after the optimization added in r173868 because that
method is no longer called when the space to distribute is 0.

That's why we could fix this by allowing calls to
distributeSpaceToTracks() with spaceToDistribute>=0 but by fixing
it with an explicit initialization our implementation becomes
closer to the new algorithm and the initialization is now explicit
in the code instead of a side effect of calling
distributeSpaceToTracks() with no space to be distributed. It also
brings a slight performance improvement as we save sorts and hash
lookups.

I also took the change to add caching to several GridTrackSize
methods that were hot on the profiler (each one accounted for ~1%
of the total time, now they account for ~0.3% each).

Test: fast/css-grid-layout/grid-initialize-span-one-items.html

  • rendering/RenderGrid.cpp:

(WebCore::GridItemWithSpan::span): New helper method for ASSERTs.
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
Exclude non spanning grid items from the calls to
resolveContentBasedTrackSizingFunctionsForItems().
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems):
New method to resolve track sizes only using non-spanning grid
items.
(WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForItems):
Ensure that it isn't called for non-spanning grid items.

  • rendering/RenderGrid.h:
  • rendering/style/GridTrackSize.h:

(WebCore::GridTrackSize::GridTrackSize): Cache return values.
(WebCore::GridTrackSize::setLength): Ditto.
(WebCore::GridTrackSize::setMinMax): Ditto.
(WebCore::GridTrackSize::cacheMinMaxTrackBreadthTypes): New method
that caches the return values for hasXXXTrackBreadth() methods.
(WebCore::GridTrackSize::hasMinOrMaxContentMinTrackBreadth): Use
the cached return value.
(WebCore::GridTrackSize::hasMaxContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinOrMaxContentMaxTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMaxContentMinTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMinTrackBreadth): Ditto.
(WebCore::GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth):
Ditto.
(WebCore::GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth):
Ditto.

LayoutTests:

  • fast/css-grid-layout/grid-content-sized-columns-resolution-expected.txt:
  • fast/css-grid-layout/grid-content-sized-columns-resolution.html:
  • fast/css-grid-layout/grid-initialize-span-one-items-expected.txt: Added.
  • fast/css-grid-layout/grid-initialize-span-one-items.html: Added.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r179983 r179987  
     12015-01-23  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
     4        https://bugs.webkit.org/show_bug.cgi?id=140763
     5
     6        Reviewed by Antti Koivisto.
     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        * fast/css-grid-layout/grid-initialize-span-one-items-expected.txt: Added.
     11        * fast/css-grid-layout/grid-initialize-span-one-items.html: Added.
     12
    1132015-02-11  Chris Fleizach  <cfleizach@apple.com>
    214

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

    r179824 r179987  
    1414Check that items are processed by ascending span to compute track breadths forbidding extra space distribution.
    1515PASS window.getComputedStyle(gridMinContentFixedAndAutoUnsortedConstrained, '').getPropertyValue('-webkit-grid-template-columns') is "0px 40px"
    16 PASS window.getComputedStyle(gridAutoAndAutoUnsortedConstrained, '').getPropertyValue('-webkit-grid-template-columns') is "5px 35px"
     16PASS window.getComputedStyle(gridAutoAndAutoUnsortedConstrained, '').getPropertyValue('-webkit-grid-template-columns') is "10px 30px"
    1717PASS window.getComputedStyle(gridMinContentAndMinContentFixedUnsortedConstrained, '').getPropertyValue('-webkit-grid-template-columns') is "0px 40px"
    1818PASS window.getComputedStyle(gridMaxContentAndMinContentUnsortedConstrained, '').getPropertyValue('-webkit-grid-template-columns') is "0px 70px"
    … …  
    2828Check that items are processed by ascending span to compute track breadths allowing extra space distribution.
    2929PASS window.getComputedStyle(gridMinContentFixedAndAutoUnsorted, '').getPropertyValue('-webkit-grid-template-columns') is "15px 90px"
    30 PASS window.getComputedStyle(gridAutoAndAutoUnsorted, '').getPropertyValue('-webkit-grid-template-columns') is "30px 60px"
     30PASS window.getComputedStyle(gridAutoAndAutoUnsorted, '').getPropertyValue('-webkit-grid-template-columns') is "60px 30px"
    3131PASS window.getComputedStyle(gridMinContentAndMinContentFixedUnsorted, '').getPropertyValue('-webkit-grid-template-columns') is "0px 40px"
    3232PASS window.getComputedStyle(gridMaxContentAndMinContentUnsorted, '').getPropertyValue('-webkit-grid-template-columns') is "0px 70px"

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

    r179824 r179987  
    317317debug("Check that items are processed by ascending span to compute track breadths forbidding extra space distribution.");
    318318testGridColumnsValues("gridMinContentFixedAndAutoUnsortedConstrained", "0px 40px");
    319 testGridColumnsValues("gridAutoAndAutoUnsortedConstrained", "5px 35px");
     319testGridColumnsValues("gridAutoAndAutoUnsortedConstrained", "10px 30px");
    320320testGridColumnsValues("gridMinContentAndMinContentFixedUnsortedConstrained", "0px 40px");
    321321testGridColumnsValues("gridMaxContentAndMinContentUnsortedConstrained", "0px 70px");
    … …  
    332332debug("Check that items are processed by ascending span to compute track breadths allowing extra space distribution.");
    333333testGridColumnsValues("gridMinContentFixedAndAutoUnsorted", "15px 90px");
    334 testGridColumnsValues("gridAutoAndAutoUnsorted", "30px 60px");
     334testGridColumnsValues("gridAutoAndAutoUnsorted", "60px 30px");
    335335testGridColumnsValues("gridMinContentAndMinContentFixedUnsorted", "0px 40px");
    336336testGridColumnsValues("gridMaxContentAndMinContentUnsorted", "0px 70px");

  • trunk/Source/WebCore/ChangeLog

    r179985 r179987  
     12015-01-23  Sergio Villar Senin  <svillar@igalia.com>
     2
     3        [CSS Grid Layout] Invalid initialization of track sizes with non spanning grid items
     4        https://bugs.webkit.org/show_bug.cgi?id=140763
     5
     6        Reviewed by Antti Koivisto.
     7
     8        Content sized tracks with non-spanning grid items were not
     9        properly sized because the growth limit was sometimes infinity
     10        (-1) after calling resolveContentBasedTrackSizingFunctions() when
     11        it should not. This patch adds an special initialization phase for
     12        non-spanning grid items as the new track sizing algorithm
     13        describes.
     14
     15        Granted, that was handled in the old algorithm in
     16        distributeSpaceToTracks() as a special case. The problem is that
     17        it regressed after the optimization added in r173868 because that
     18        method is no longer called when the space to distribute is 0.
     19
     20        That's why we could fix this by allowing calls to
     21        distributeSpaceToTracks() with spaceToDistribute>=0 but by fixing
     22        it with an explicit initialization our implementation becomes
     23        closer to the new algorithm and the initialization is now explicit
     24        in the code instead of a side effect of calling
     25        distributeSpaceToTracks() with no space to be distributed. It also
     26        brings a slight performance improvement as we save sorts and hash
     27        lookups.
     28
     29        I also took the change to add caching to several GridTrackSize
     30        methods that were hot on the profiler (each one accounted for ~1%
     31        of the total time, now they account for ~0.3% each).
     32
     33        Test: fast/css-grid-layout/grid-initialize-span-one-items.html
     34
     35        * rendering/RenderGrid.cpp:
     36        (WebCore::GridItemWithSpan::span): New helper method for ASSERTs.
     37        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctions):
     38        Exclude non spanning grid items from the calls to
     39        resolveContentBasedTrackSizingFunctionsForItems().
     40        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems):
     41        New method to resolve track sizes only using non-spanning grid
     42        items.
     43        (WebCore::RenderGrid::resolveContentBasedTrackSizingFunctionsForItems):
     44        Ensure that it isn't called for non-spanning grid items.
     45        * rendering/RenderGrid.h:
     46        * rendering/style/GridTrackSize.h:
     47        (WebCore::GridTrackSize::GridTrackSize): Cache return values.
     48        (WebCore::GridTrackSize::setLength): Ditto.
     49        (WebCore::GridTrackSize::setMinMax): Ditto.
     50        (WebCore::GridTrackSize::cacheMinMaxTrackBreadthTypes): New method
     51        that caches the return values for hasXXXTrackBreadth() methods.
     52        (WebCore::GridTrackSize::hasMinOrMaxContentMinTrackBreadth): Use
     53        the cached return value.
     54        (WebCore::GridTrackSize::hasMaxContentMaxTrackBreadth): Ditto.
     55        (WebCore::GridTrackSize::hasMinContentMaxTrackBreadth): Ditto.
     56        (WebCore::GridTrackSize::hasMinOrMaxContentMaxTrackBreadth): Ditto.
     57        (WebCore::GridTrackSize::hasMaxContentMinTrackBreadth): Ditto.
     58        (WebCore::GridTrackSize::hasMinContentMinTrackBreadth): Ditto.
     59        (WebCore::GridTrackSize::hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth):
     60        Ditto.
     61        (WebCore::GridTrackSize::hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth):
     62        Ditto.
     63
    1642015-02-12  Zan Dobersek  <zdobersek@igalia.com>
    265

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

    r179824 r179987  
    585585    RenderBox& gridItem() const { return m_gridItem; }
    586586    GridCoordinate coordinate() const { return m_coordinate; }
     587#if !ASSERT_DISABLED
     588    size_t span() const { return m_span; }
     589#endif
    587590
    588591    bool operator<(const GridItemWithSpan other) const
    … …  
    620623    for (auto trackIndex : sizingData.contentSizedTracksIndex) {
    621624        GridIterator iterator(m_grid, direction, trackIndex);
     625        GridTrack& track = (direction == ForColumns) ? sizingData.columnTracks[trackIndex] : sizingData.rowTracks[trackIndex];
    622626
    623627        while (RenderBox* gridItem = iterator.nextGridItem()) {
    624628            if (itemsSet.add(gridItem).isNewEntry) {
    625629                const GridCoordinate& coordinate = cachedGridCoordinate(*gridItem);
    626                 // We should not include items spanning more than one track that span tracks with flexible sizing functions.
    627                 if (integerSpanForDirection(coordinate, direction) == 1 || !spanningItemCrossesFlexibleSizedTracks(coordinate, direction))
     630                if (integerSpanForDirection(coordinate, direction) == 1)
     631                    resolveContentBasedTrackSizingFunctionsForNonSpanningItems(direction, coordinate, *gridItem, track, sizingData.columnTracks);
     632                else if (!spanningItemCrossesFlexibleSizedTracks(coordinate, direction))
    628633                    sizingData.itemsSortedByIncreasingSpan.append(GridItemWithSpan(*gridItem, coordinate, direction));
    629634            }
    … …  
    646651}
    647652
     653void RenderGrid::resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection direction, const GridCoordinate& coordinate, RenderBox& gridItem, GridTrack& track, Vector<GridTrack>& columnTracks)
     654{
     655    const GridResolvedPosition trackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;
     656    GridTrackSize trackSize = gridTrackSize(direction, trackPosition.toInt());
     657
     658    if (trackSize.hasMinContentMinTrackBreadth())
     659        track.setBaseSize(std::max(track.baseSize(), minContentForChild(gridItem, direction, columnTracks)));
     660    else if (trackSize.hasMaxContentMinTrackBreadth())
     661        track.setBaseSize(std::max(track.baseSize(), maxContentForChild(gridItem, direction, columnTracks)));
     662
     663    if (trackSize.hasMinContentMaxTrackBreadth())
     664        track.setGrowthLimit(std::max(track.growthLimit(), minContentForChild(gridItem, direction, columnTracks)));
     665    else if (trackSize.hasMaxContentMaxTrackBreadth())
     666        track.setGrowthLimit(std::max(track.growthLimit(), maxContentForChild(gridItem, direction, columnTracks)));
     667}
     668
    648669void RenderGrid::resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection direction, GridSizingData& sizingData, GridItemWithSpan& gridItemWithSpan, FilterFunction filterFunction, SizingFunction sizingFunction, AccumulatorGetter trackGetter, AccumulatorGrowFunction trackGrowthFunction, FilterFunction growAboveMaxBreadthFilterFunction)
    649670{
     671    ASSERT(gridItemWithSpan.span() > 1);
    650672    const GridCoordinate& coordinate = gridItemWithSpan.coordinate();
    651673    const GridResolvedPosition initialTrackPosition = (direction == ForColumns) ? coordinate.columns.resolvedInitialPosition : coordinate.rows.resolvedInitialPosition;

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

    r179824 r179987  
    9292    typedef void (GridTrack::* AccumulatorGrowFunction)(LayoutUnit);
    9393    typedef bool (GridTrackSize::* FilterFunction)() const;
     94    void resolveContentBasedTrackSizingFunctionsForNonSpanningItems(GridTrackSizingDirection, const GridCoordinate&, RenderBox& gridItem, GridTrack&, Vector<GridTrack>& columnTracks);
    9495    void resolveContentBasedTrackSizingFunctionsForItems(GridTrackSizingDirection, GridSizingData&, GridItemWithSpan&, FilterFunction, SizingFunction, AccumulatorGetter, AccumulatorGrowFunction, FilterFunction growAboveMaxBreadthFilterFunction = nullptr);
    9596    void distributeSpaceToTracks(Vector<GridTrack*>&, Vector<unsigned>* growAboveMaxBreadthTrackIndexes, AccumulatorGetter, AccumulatorGrowFunction, GridSizingData&, LayoutUnit& availableLogicalSpace);

  • trunk/Source/WebCore/rendering/style/GridTrackSize.h

    r174643 r179987  
    4747public:
    4848    GridTrackSize(LengthType type = Undefined)
    49         : m_type(LengthTrackSizing)
    50         , m_minTrackBreadth(type)
    51         , m_maxTrackBreadth(type)
    5249    {
     50        setLength(GridLength(Length(type)));
    5351    }
    5452
    5553    GridTrackSize(const GridLength& minTrackBreadth, const GridLength& maxTrackBreadth)
     54        : m_type(MinMaxTrackSizing)
     55        , m_minTrackBreadth(minTrackBreadth)
     56        , m_maxTrackBreadth(maxTrackBreadth)
    5657    {
    57         setMinMax(minTrackBreadth, maxTrackBreadth);
     58        cacheMinMaxTrackBreadthTypes();
    5859    }
    5960
    … …  
    7273        m_minTrackBreadth = length;
    7374        m_maxTrackBreadth = length;
     75        cacheMinMaxTrackBreadthTypes();
    7476    }
    7577
    … …  
    99101        m_minTrackBreadth = minTrackBreadth;
    100102        m_maxTrackBreadth = maxTrackBreadth;
     103        cacheMinMaxTrackBreadthTypes();
    101104    }
    102105
    … …  
    112115    }
    113116
    114     bool hasMinOrMaxContentMinTrackBreadth() const { return minTrackBreadth().isLength() && (minTrackBreadth().length().isMinContent() || minTrackBreadth().length().isMaxContent()); }
    115     bool hasMaxContentMinTrackBreadth() const { return minTrackBreadth().isLength() && minTrackBreadth().length().isMaxContent(); }
    116     bool hasMinOrMaxContentMaxTrackBreadth() const { return maxTrackBreadth().isLength() && (maxTrackBreadth().length().isMinContent() || maxTrackBreadth().length().isMaxContent()); }
    117     bool hasMaxContentMaxTrackBreadth() const { return maxTrackBreadth().isLength() && maxTrackBreadth().length().isMaxContent(); }
    118     bool hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth() const { return minTrackBreadth().isLength() && minTrackBreadth().length().isMinContent() && hasMinOrMaxContentMaxTrackBreadth(); }
    119     bool hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth() const { return hasMaxContentMinTrackBreadth() && hasMaxContentMaxTrackBreadth(); }
     117    void cacheMinMaxTrackBreadthTypes()
     118    {
     119        m_minTrackBreadthIsMinContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMinContent();
     120        m_minTrackBreadthIsMaxContent = minTrackBreadth().isLength() && minTrackBreadth().length().isMaxContent();
     121        m_maxTrackBreadthIsMaxContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMaxContent();
     122        m_maxTrackBreadthIsMinContent = maxTrackBreadth().isLength() && maxTrackBreadth().length().isMinContent();
     123    }
    120124
     125    bool hasMinOrMaxContentMinTrackBreadth() const { return m_minTrackBreadthIsMaxContent || m_minTrackBreadthIsMinContent; }
     126    bool hasMaxContentMaxTrackBreadth() const { return m_maxTrackBreadthIsMaxContent; }
     127    bool hasMinContentMaxTrackBreadth() const { return m_maxTrackBreadthIsMinContent; }
     128    bool hasMinOrMaxContentMaxTrackBreadth() const { return m_maxTrackBreadthIsMaxContent || m_maxTrackBreadthIsMinContent; }
     129    bool hasMaxContentMinTrackBreadth() const { return m_minTrackBreadthIsMaxContent; }
     130    bool hasMinContentMinTrackBreadth() const { return m_minTrackBreadthIsMinContent; }
     131    bool hasMinContentMinTrackBreadthAndMinOrMaxContentMaxTrackBreadth() const { return m_minTrackBreadthIsMinContent && hasMinOrMaxContentMaxTrackBreadth(); }
     132    bool hasMaxContentMinTrackBreadthAndMaxContentMaxTrackBreadth() const { return m_minTrackBreadthIsMaxContent && m_maxTrackBreadthIsMaxContent; }
    121133
    122134private:
    … …  
    124136    GridLength m_minTrackBreadth;
    125137    GridLength m_maxTrackBreadth;
     138    bool m_minTrackBreadthIsMaxContent;
     139    bool m_minTrackBreadthIsMinContent;
     140    bool m_maxTrackBreadthIsMaxContent;
     141    bool m_maxTrackBreadthIsMinContent;
    126142};
    127143
Note: See TracChangeset for help on using the changeset viewer.