Changeset 277435 in webkit


Ignore:
Timestamp:
May 13, 2021 8:00:12 AM (14 months ago)
Author:
Alan Bujtas
Message:

[css-flexbox] Flex item construction may affect sibling flex item height computation
​https://bugs.webkit.org/show_bug.cgi?id=225489

Reviewed by Sergio Villar Senin.

Source/WebCore:

Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
During this layout a percent height descendant may indirectly set an incorrect value on the flex container's
m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain,
see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
As the result we end up with an incorrect, 0px height value for this sibling.
e.g.
<div style="height: 100px; display: flex; flex-direction: column;">

<div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
<div id=flexItemB style="height: 50%;"></div>

</div>
By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
e.g "height: auto", while it is clearly resolvable (50% of 100px).
Now if we flip the order and flexItemB comes first, the test case passes fine.
It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
fixing it would certainly require some substantial architectural change.
In this patch, we just reset the m_hasDefiniteHeight flag inside the loop to ensure that each flex item
starts with a fresh height percent resolve state.

Test: fast/flexbox/flex-column-with-percent-height-descendants.html

  • rendering/RenderFlexibleBox.cpp:

(WebCore::RenderFlexibleBox::layoutFlexItems):

LayoutTests:

  • fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
  • fast/flexbox/flex-column-with-percent-height-descendants.html: Added.
Location:
trunk
Files:
4 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r277434 r277435  
     12021-05-13  Zalan Bujtas  <zalan@apple.com>
     2
     3        [css-flexbox] Flex item construction may affect sibling flex item height computation
     4        https://bugs.webkit.org/show_bug.cgi?id=225489
     5
     6        Reviewed by Sergio Villar Senin.
     7
     8        * fast/flexbox/flex-column-with-percent-height-descendants-expected.html: Added.
     9        * fast/flexbox/flex-column-with-percent-height-descendants.html: Added.
     10
    1112021-05-13  Chris Fleizach  <cfleizach@apple.com>
    212

  • trunk/Source/WebCore/ChangeLog

    r277434 r277435  
     12021-05-13  Zalan Bujtas  <zalan@apple.com>
     2
     3        [css-flexbox] Flex item construction may affect sibling flex item height computation
     4        https://bugs.webkit.org/show_bug.cgi?id=225489
     5
     6        Reviewed by Sergio Villar Senin.
     7
     8        Flex item construction triggers layout both on the flex item and on its descendants (see constructFlexItem).
     9        During this layout a percent height descendant may indirectly set an incorrect value on the flex container's
     10        m_hasDefiniteHeight (this is due to the odd way we choose to resolve percent height values on the ancestor chain,
     11        see setOverridingContainingBlockContentLogicalHeight(WTF::nullopt)).
     12        Now this incorrect m_hasDefiniteHeight value (Indefinite) causes the next sibling's (also) percent height
     13        resolve fail as it tells the flex item that the flex container can't help with resolving the percent height value.
     14        As the result we end up with an incorrect, 0px height value for this sibling.
     15        e.g.
     16        <div style="height: 100px; display: flex; flex-direction: column;">
     17          <div id=flexItemA style="height: 50px;"><div style="height: 100%;"></div></div>
     18          <div id=flexItemB style="height: 50%;"></div>
     19        </div>
     20        By the time we get to the construction of flexItemB, the RenderFlexibleBox's (flex container) m_hasDefiniteHeight
     21        is already (and incorrectly) set to Indefinite as the result of us trying to resolve flexItemA's descendant height percentage.
     22        This Indefinite value makes the flexItemB's height resolution fail as we believe that the flex container has indefinite height
     23        e.g "height: auto", while it is clearly resolvable (50% of 100px).
     24        Now if we flip the order and flexItemB comes first, the test case passes fine.
     25        It is very unfortunate that some descendant height resolving process can trigger a state change on the ancestor RenderFlexibleBox, but
     26        fixing it would certainly require some substantial architectural change.
     27        In this patch, we just reset the m_hasDefiniteHeight flag inside the loop to ensure that each flex item
     28        starts with a fresh height percent resolve state.
     29
     30        Test: fast/flexbox/flex-column-with-percent-height-descendants.html
     31
     32        * rendering/RenderFlexibleBox.cpp:
     33        (WebCore::RenderFlexibleBox::layoutFlexItems):
     34
    1352021-05-13  Chris Fleizach  <cfleizach@apple.com>
    236

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

    r277371 r277435  
    994994        }
    995995        allItems.append(constructFlexItem(*child, relayoutChildren));
    996     }
    997 
    998     // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
    999     m_hasDefiniteHeight = SizeDefiniteness::Unknown;
     996        // constructFlexItem() might set the override containing block height so any value cached for definiteness might be incorrect.
     997        m_hasDefiniteHeight = SizeDefiniteness::Unknown;
     998    }
    1000999   
    10011000    const LayoutUnit lineBreakLength = mainAxisContentExtent(LayoutUnit::max());
Note: See TracChangeset for help on using the changeset viewer.