Bug 259254

Summary:	Correctly identify overflow direction inside flexbox
Product:	WebKit	Reporter:	Ahmad Saleem <ahmad.saleem792>
Component:	Layout and Rendering	Assignee:	Nobody <webkit-unassigned>
Status:	NEW ---
Severity:	Normal	CC:	bfulgham, fantasai.bugs, post, simon.fraser, webkit-bug-importer, zalan
Priority:	P2	Keywords:	BrowserCompat, InRadar, WPTImpact
Version:	WebKit Nightly Build
Hardware:	Unspecified
OS:	Unspecified

Description Ahmad Saleem 2023-07-15 20:48:28 PDT

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!

Comment 1 Ahmad Saleem 2023-07-15 20:52:54 PDT

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

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

Comment 2 Radar WebKit Bug Importer 2023-07-22 20:49:18 PDT

<rdar://problem/112723066>

Comment 3 Ahmad Saleem 2023-07-31 05:28:27 PDT

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.

Comment 4 Ahmad Saleem 2023-07-31 05:29:53 PDT

(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

Comment 5 Ahmad Saleem 2023-07-31 05:40:02 PDT

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

Comment 6 Ahmad Saleem 2023-07-31 08:14:12 PDT

(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

Comment 7 Ahmad Saleem 2023-07-31 09:49:46 PDT


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;

Comment 8 Ahmad Saleem 2024-01-22 06:26:16 PST

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'.

___