Bug 252422

Summary:

Compute the correct overflow-x and overflow-y values for table elements

Product:

WebKit

Reporter:

Ahmad Saleem <ahmad.saleem792>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Local build changes based of Blink Commit	none

Description Ahmad Saleem 2023-02-16 13:45:23 PST

Created attachment 465030 [details]
Local build changes based of Blink Commit

Hi Team,

While going through Blink's commit, I came across another failing test case (just took one from the commit):

Failing Test Case - https://jsfiddle.net/xd6cmuLg/show

^ Both Chrome Canary 112 and Firefox Nightly 112 passes this.

Blink Commit - https://src.chromium.org/viewvc/blink?view=revision&revision=199640

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/style/StyleAdjuster.cpp#483

__

I manage to test this PR locally and it seems to fix above test case and show all passes.

I am attaching screenshot of my change to get input whether it is right fix so later I can do PR.

Thanks!

Comment 1 zalan 2023-02-16 20:01:43 PST

yeah, we should take this fix. Thank you!

Comment 2 Radar WebKit Bug Importer 2023-02-23 13:46:19 PST

<rdar://problem/105850323>

Comment 3 Ahmad Saleem 2023-07-29 07:48:34 PDT

bool overflowIsClipOrVisible = isOverflowClipOrVisible(style.overflowX()) && isOverflowClipOrVisible(style.overflowX());
    if (!overflowIsClipOrVisible && (style.display() == DisplayType::Table || style.display() == DisplayType::InlineTable)) {
        // Tables only support overflow:hidden and overflow:visible and ignore anything else,
        // see https://drafts.csswg.org/css2/#overflow. As a table is not a block
        // container box the rules for resolving conflicting x and y values in CSS Overflow Module
        // Level 3 do not apply. Arguably overflow-x and overflow-y aren't allowed on tables but
        // all UAs allow it.
        if (style.overflowX() != Overflow::Hidden)
            style.setOverflowX(Overflow::Visible);
        if (style.overflowY() != Overflow::Hidden)
            style.setOverflowY(Overflow::Visible);
        // If we are left with conflicting overflow values for the x and y axes on a table then resolve
        // both to Overflow::Visible. This is interoperable behaviour but is not specced anywhere.
        if (style.overflowX() == Overflow::Visible)
            style.setOverflowY(Overflow::Visible);
        else if (style.overflowY() == Overflow::Visible)
            style.setOverflowX(Overflow::Visible);
    } else if (!isOverflowClipOrVisible(style.overflowY())) {
        // Values of 'clip' and 'visible' can only be used with 'clip' and 'visible'.
        // If they aren't, 'clip' and 'visible' is reset.
        if (style.overflowX() == Overflow::Visible)
            style.setOverflowX(Overflow::Auto);
        else if (style.overflowX() == Overflow::Clip)
            style.setOverflowX(Overflow::Hidden);
    } else if (!isOverflowClipOrVisible(style.overflowX())) {
        // Values of 'clip' and 'visible' can only be used with 'clip' and 'visible'.
        // If they aren't, 'clip' and 'visible' is reset.
        if (style.overflowY() == Overflow::Visible)
            style.setOverflowY(Overflow::Auto);
        else if (style.overflowY() == Overflow::Clip)
            style.setOverflowY(Overflow::Hidden);
    }

and

static bool isOverflowClipOrVisible(Overflow overflow)
{
    return overflow == Overflow::Clip || overflow == Overflow::Clip;
}

____

It leads to massive failures in WPT test suite. :-(

Comment 4 Ahmad Saleem 2023-07-29 07:49:42 PDT

(In reply to Ahmad Saleem from comment #3)
> bool overflowIsClipOrVisible = isOverflowClipOrVisible(style.overflowX()) &&
> isOverflowClipOrVisible(style.overflowX());
>     if (!overflowIsClipOrVisible && (style.display() == DisplayType::Table
> || style.display() == DisplayType::InlineTable)) {
>         // Tables only support overflow:hidden and overflow:visible and
> ignore anything else,
>         // see https://drafts.csswg.org/css2/#overflow. As a table is not a
> block
>         // container box the rules for resolving conflicting x and y values
> in CSS Overflow Module
>         // Level 3 do not apply. Arguably overflow-x and overflow-y aren't
> allowed on tables but
>         // all UAs allow it.
>         if (style.overflowX() != Overflow::Hidden)
>             style.setOverflowX(Overflow::Visible);
>         if (style.overflowY() != Overflow::Hidden)
>             style.setOverflowY(Overflow::Visible);
>         // If we are left with conflicting overflow values for the x and y
> axes on a table then resolve
>         // both to Overflow::Visible. This is interoperable behaviour but is
> not specced anywhere.
>         if (style.overflowX() == Overflow::Visible)
>             style.setOverflowY(Overflow::Visible);
>         else if (style.overflowY() == Overflow::Visible)
>             style.setOverflowX(Overflow::Visible);
>     } else if (!isOverflowClipOrVisible(style.overflowY())) {
>         // Values of 'clip' and 'visible' can only be used with 'clip' and
> 'visible'.
>         // If they aren't, 'clip' and 'visible' is reset.
>         if (style.overflowX() == Overflow::Visible)
>             style.setOverflowX(Overflow::Auto);
>         else if (style.overflowX() == Overflow::Clip)
>             style.setOverflowX(Overflow::Hidden);
>     } else if (!isOverflowClipOrVisible(style.overflowX())) {
>         // Values of 'clip' and 'visible' can only be used with 'clip' and
> 'visible'.
>         // If they aren't, 'clip' and 'visible' is reset.
>         if (style.overflowY() == Overflow::Visible)
>             style.setOverflowY(Overflow::Auto);
>         else if (style.overflowY() == Overflow::Clip)
>             style.setOverflowY(Overflow::Hidden);
>     }
> 
> and
> 
> static bool isOverflowClipOrVisible(Overflow overflow)
> {
>     return overflow == Overflow::Clip || overflow == Overflow::Clip;
> }
> 
> ____
> 
> It leads to massive failures in WPT test suite. :-(

^ based on 'Style_Adjuster.cc' from Chromium / Blink.

Comment 5 Ahmad Saleem 2023-07-29 07:59:18 PDT

Big Facepalm for me:

static bool isOverflowClipOrVisible(Overflow overflow)
{
    return overflow == Overflow::Clip || overflow == Overflow::Clip;
}

Clip <- twice...

Comment 6 EWS 2023-08-03 17:22:58 PDT

Committed 266560@main (e6264aebbca8): <https://commits.webkit.org/266560@main>

Reviewed commits have been landed. Closing PR #16219 and removing active labels.