Correctly identify overflow direction inside flexbox

[Ahmad Saleem]

Hi Team,

While looking into Flexbox failure, came across following failing WPT test:

Live Link: http://wpt.live/css/css-flexbox/overflow-top-left.html

Blink Commit: https://chromium-review.googlesource.com/c/chromium/src/+/1618017

WebKit Source: https://github.com/WebKit/WebKit/blob/3ae98fa2c02b272e3e55c4e0c324d01da17cb5dc/Source/WebCore/rendering/RenderFlexibleBox.cpp#L2502

________

I tried changing to this:

bool RenderFlexibleBox::isTopLayoutOverflowAllowed() const
{
    auto flexDirection = style().flexDirection();
    if (isHorizontalWritingMode())
        return flexDirection == FlexDirection::ColumnReverse;
    return !style().isLeftToRightDirection() ^ (flexDirection == FlexDirection::RowReverse);
}

bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
{
    auto flexDirection = style().flexDirection();
    if (isHorizontalWritingMode())
        return flexDirection == FlexDirection::RowReverse;
    return !style().isLeftToRightDirection() ^ (flexDirection == FlexDirection::ColumnReverse);
}

_______

Although this does not fix the test case but still good to track.

Hence, I am raising it and adding 'BrowserCompat' and 'WPTImpact' tags.

Thanks!

[Ahmad Saleem]

Also why we have 'RenderBlock' while 'isTopLayoutOverflowAllowed' is in RenderBox, in 'RenderBoxInlines.h':

https://searchfox.org/wubkat/source/Source/WebCore/rendering/RenderBoxInlines.h#53

[Radar WebKit Bug Importer]

<rdar://problem/112723066>

[Ahmad Saleem]

I did wrong patch in Comment 0 but:

bool RenderFlexibleBox::isTopLayoutOverflowAllowed() const
{
    auto flexDirection = style().flexDirection();
    if (isHorizontalWritingMode())
        return flexDirection == FlexDirection::ColumnReverse;
    return !style().isLeftToRightDirection() ^ (flexDirection == FlexDirection::RowReverse);
}

bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
{
    auto flexDirection = style().flexDirection();
    if (isHorizontalWritingMode())
        return !style().isLeftToRightDirection() ^ (flexDirection == FlexDirection::RowReverse);
    return flexDirection == FlexDirection::ColumnReverse;
}

^ This make us progress on two subtests of 'css/css-flexbox/negative-overflow-002.html', for some reason of 'scrollbar' direction failure in 'overflow-top-left' seems to be 'scrollbar' related bug to me.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #3)
> I did wrong patch in Comment 0 but:
> 
> bool RenderFlexibleBox::isTopLayoutOverflowAllowed() const
> {
>     auto flexDirection = style().flexDirection();
>     if (isHorizontalWritingMode())
>         return flexDirection == FlexDirection::ColumnReverse;
>     return !style().isLeftToRightDirection() ^ (flexDirection ==
> FlexDirection::RowReverse);
> }
> 
> bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
> {
>     auto flexDirection = style().flexDirection();
>     if (isHorizontalWritingMode())
>         return !style().isLeftToRightDirection() ^ (flexDirection ==
> FlexDirection::RowReverse);
>     return flexDirection == FlexDirection::ColumnReverse;
> }
> 
> ^ This make us progress on two subtests of
> 'css/css-flexbox/negative-overflow-002.html', for some reason of 'scrollbar'
> direction failure in 'overflow-top-left' seems to be 'scrollbar' related bug
> to me.

Precisely PASS .container 16 and PASS .container 17

[Ahmad Saleem]

NOTE - implementing this might progress more as well: https://chromium-review.googlesource.com/c/chromium/src/+/3686776

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #5)
> NOTE - implementing this might progress more as well:
> https://chromium-review.googlesource.com/c/chromium/src/+/3686776

Based on this:

bool RenderFlexibleBox::isTopLayoutOverflowAllowed() const
{
    bool isWrapReverse = style().flexWrap() == FlexWrap::Reverse;
    if (isHorizontalWritingMode())
        return style().isColumnReverseFlexDirection() || (style().isRowFlexDirection() && isWrapReverse);
    return style().isLeftToRightDirection() == (style().isRowReverseFlexDirection() || (style().isColumnFlexDirection() && isWrapReverse));
}

bool RenderFlexibleBox::isLeftLayoutOverflowAllowed() const
{
    bool isWrapReverse = style().flexWrap() == FlexWrap::Reverse;
    if (isHorizontalWritingMode())
        return style().isLeftToRightDirection() == (style().isRowReverseFlexDirection() || (style().isColumnFlexDirection() && isWrapReverse));
    return style().isFlippedLinesWritingMode() == (style().isColumnReverseFlexDirection() || (style().isRowFlexDirection() && isWrapReverse));
}

and also necessary changes in RenderStyle.h and RenderStyleInlines.h, we progress following but fail two already passing as well:

>> css-flexbox/negative-overflow.html:

Pass now '.flexbox 5'.

>> negative-overflow-002.html:

Failures: .container 31, .container 32, .container 25 and .container 26

Passes: .container 16 & .container 17 and .container 69

[Ahmad Saleem]

UPDATE:
> Passes: .container 16 & .container 17 and .container 69 and .container 21 as well.

New to add in RenderStyleInlines.h (and also modify one old to account for 'DeprecatedFlexBox):

inline bool RenderStyle::isColumnFlexDirection() const { if (isDisplayDeprecatedFlexibleBox(display())) { return boxOrient() == BoxOrient::Vertical; } return flexDirection() == FlexDirection::Column || flexDirection() == FlexDirection::ColumnReverse; }

inline bool RenderStyle::isColumnReverseFlexDirection() const { if (isDisplayDeprecatedFlexibleBox(display())) { return boxOrient() == BoxOrient::Vertical && boxDirection() == BoxDirection::Reverse; } return flexDirection() == FlexDirection::ColumnReverse; }

inline bool RenderStyle::isRowFlexDirection() const { if (isDisplayDeprecatedFlexibleBox(display())) { return boxOrient() == BoxOrient::Horizontal; } return flexDirection() == FlexDirection::Row || flexDirection() == FlexDirection::RowReverse; }

inline bool RenderStyle::isRowReverseFlexDirection() const { if (isDisplayDeprecatedFlexibleBox(display())) { return boxOrient() == BoxOrient::Horizontal && boxDirection() == BoxDirection::Reverse; } return flexDirection() == FlexDirection::RowReverse; }

and following in RenderStyle.h:

inline bool isRowFlexDirection() const;
inline bool isColumnReverseFlexDirection() const;
inline bool isRowReverseFlexDirection() const;

[Ahmad Saleem]

Just to update - all progress tests with this are now fixed via Elika's PR: https://github.com/WebKit/WebKit/pull/23038

Except:

>> css-flexbox/negative-overflow.html:


Pass now '.flexbox 5'.

___