Changing 'DisplayType::InlineGrid' and 'DisplayType::InlineFlex' to output properly in StyleAdjuster.cpp

[Ahmad Saleem]

Hi Team,

While trying to fix another bug, I came across another fix to failing testcase for following WPT test:

WPT Test: legend-grid-flex-multicol.html & legend-display.html

WPT Test Link (Live): http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/legend-grid-flex-multicol.html

^ Make us pass last two remaining failing tests.

WPT Test: legend-display.html

WPT Test Link (Live): http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-elements/legend-display.html

^ Make us pass more 8 tests cases.

________________________

WebKit Source to change:

https://github.com/WebKit/WebKit/blob/e17c2153ab908a5821a2b72ee9fe7856b98aac12/Source/WebCore/style/StyleAdjuster.cpp#L116

^ Change above to following (Partial Copy - diff):

    case DisplayType::Block:
    case DisplayType::Table:
    case DisplayType::Box:
    case DisplayType::FlowRoot:
    case DisplayType::ListItem:
        return display;
    case DisplayType::InlineTable:
        return DisplayType::Table;
    case DisplayType::InlineBox:
        return DisplayType::InlineBox;
    case DisplayType::Flex:
        return DisplayType::Flex;
    case DisplayType::InlineFlex:
        return DisplayType::InlineFlex;
    case DisplayType::Grid:
        return DisplayType::Grid;
    case DisplayType::InlineGrid:
        return DisplayType::InlineGrid;
    case DisplayType::Inline:
    case DisplayType::InlineBlock:

_____________________________________
^ Just wanted to raise so we can look into it.

Thanks!

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #0)
> Hi Team,
> 
> While trying to fix another bug, I came across another fix to failing
> testcase for following WPT test:
> 
> WPT Test: legend-grid-flex-multicol.html & legend-display.html
> 
> WPT Test Link (Live):
> http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> elements/legend-grid-flex-multicol.html
> 
> ^ Make us pass last two remaining failing tests.
> 
> WPT Test: legend-display.html
> 
> WPT Test Link (Live):
> http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> elements/legend-display.html
> 
> ^ Make us pass more 8 tests cases.
> 
> ________________________
> 
> WebKit Source to change:
> 
> https://github.com/WebKit/WebKit/blob/
> e17c2153ab908a5821a2b72ee9fe7856b98aac12/Source/WebCore/style/StyleAdjuster.
> cpp#L116
> 
> ^ Change above to following (Partial Copy - diff):
> 
>     case DisplayType::Block:
>     case DisplayType::Table:
>     case DisplayType::Box:
>     case DisplayType::FlowRoot:
>     case DisplayType::ListItem:
>         return display;
>     case DisplayType::InlineTable:
>         return DisplayType::Table;
>     case DisplayType::InlineBox:
>         return DisplayType::InlineBox;
>     case DisplayType::Flex:
>         return DisplayType::Flex;
>     case DisplayType::InlineFlex:
>         return DisplayType::InlineFlex;
>     case DisplayType::Grid:
>         return DisplayType::Grid;
>     case DisplayType::InlineGrid:
>         return DisplayType::InlineGrid;
>     case DisplayType::Inline:
>     case DisplayType::InlineBlock:
> 
> _____________________________________
> ^ Just wanted to raise so we can look into it.
> 
> Thanks!

Although it regresses:

css-display/parsing/display-computed.html
css-flexbox/flexbox_inline-float.html
css-flexbox/percentage-padding-002.html

^ Might be something else. We might need something specific, just for 'legend' attribute in StyleAdjuster.cpp'.

[zalan]

(In reply to Ahmad Saleem from comment #0)
> Hi Team,
> 
> While trying to fix another bug, I came across another fix to failing
> testcase for following WPT test:
> 
> WPT Test: legend-grid-flex-multicol.html & legend-display.html
> 
> WPT Test Link (Live):
> http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> elements/legend-grid-flex-multicol.html
> 
> ^ Make us pass last two remaining failing tests.
> 
> WPT Test: legend-display.html
> 
> WPT Test Link (Live):
> http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> elements/legend-display.html
> 
> ^ Make us pass more 8 tests cases.
> 
> ________________________
> 
> WebKit Source to change:
> 
> https://github.com/WebKit/WebKit/blob/
> e17c2153ab908a5821a2b72ee9fe7856b98aac12/Source/WebCore/style/StyleAdjuster.
> cpp#L116
> 
> ^ Change above to following (Partial Copy - diff):
> 
>     case DisplayType::Block:
>     case DisplayType::Table:
>     case DisplayType::Box:
>     case DisplayType::FlowRoot:
>     case DisplayType::ListItem:
>         return display;
>     case DisplayType::InlineTable:
>         return DisplayType::Table;
>     case DisplayType::InlineBox:
>         return DisplayType::InlineBox;
>     case DisplayType::Flex:
>         return DisplayType::Flex;
>     case DisplayType::InlineFlex:
>         return DisplayType::InlineFlex;
>     case DisplayType::Grid:
>         return DisplayType::Grid;
>     case DisplayType::InlineGrid:
>         return DisplayType::InlineGrid;
>     case DisplayType::Inline:
>     case DisplayType::InlineBlock:
> 
> _____________________________________
> ^ Just wanted to raise so we can look into it.
> 
> Thanks!
equivalentBlockDisplay (as the name implies) is supposed to return the block version of an inline type of value.
>     case DisplayType::InlineFlex:
>         return DisplayType::InlineFlex;
would certainly break this contract.

[Ahmad Saleem]

(In reply to zalan from comment #2)
> (In reply to Ahmad Saleem from comment #0)
> > Hi Team,
> > 
> > While trying to fix another bug, I came across another fix to failing
> > testcase for following WPT test:
> > 
> > WPT Test: legend-grid-flex-multicol.html & legend-display.html
> > 
> > WPT Test Link (Live):
> > http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> > elements/legend-grid-flex-multicol.html
> > 
> > ^ Make us pass last two remaining failing tests.
> > 
> > WPT Test: legend-display.html
> > 
> > WPT Test Link (Live):
> > http://wpt.live/html/rendering/non-replaced-elements/the-fieldset-and-legend-
> > elements/legend-display.html
> > 
> > ^ Make us pass more 8 tests cases.
> > 
> > ________________________
> > 
> > WebKit Source to change:
> > 
> > https://github.com/WebKit/WebKit/blob/
> > e17c2153ab908a5821a2b72ee9fe7856b98aac12/Source/WebCore/style/StyleAdjuster.
> > cpp#L116
> > 
> > ^ Change above to following (Partial Copy - diff):
> > 
> >     case DisplayType::Block:
> >     case DisplayType::Table:
> >     case DisplayType::Box:
> >     case DisplayType::FlowRoot:
> >     case DisplayType::ListItem:
> >         return display;
> >     case DisplayType::InlineTable:
> >         return DisplayType::Table;
> >     case DisplayType::InlineBox:
> >         return DisplayType::InlineBox;
> >     case DisplayType::Flex:
> >         return DisplayType::Flex;
> >     case DisplayType::InlineFlex:
> >         return DisplayType::InlineFlex;
> >     case DisplayType::Grid:
> >         return DisplayType::Grid;
> >     case DisplayType::InlineGrid:
> >         return DisplayType::InlineGrid;
> >     case DisplayType::Inline:
> >     case DisplayType::InlineBlock:
> > 
> > _____________________________________
> > ^ Just wanted to raise so we can look into it.
> > 
> > Thanks!
> equivalentBlockDisplay (as the name implies) is supposed to return the block
> version of an inline type of value.
> >     case DisplayType::InlineFlex:
> >         return DisplayType::InlineFlex;
> would certainly break this contract.

Creating separate function for 'Legend' & 'Fieldset' and then using it?

[Radar WebKit Bug Importer]

<rdar://problem/109923265>

[Ahmad Saleem]

Creating another 'Static' function in StyleAdjuster.cpp:

static DisplayType equivalentDisplayForLegend(const RenderStyle& style)
{
    switch (auto display = style.display()) {
    case DisplayType::Block:
    case DisplayType::Table:
    case DisplayType::Box:
    case DisplayType::FlowRoot:
    case DisplayType::ListItem:
        return display;
    case DisplayType::InlineTable:
        return DisplayType::Table;
    case DisplayType::InlineBox:
        return DisplayType::InlineBox;
    case DisplayType::Flex:
        return DisplayType::Flex;
    case DisplayType::InlineFlex:
        return DisplayType::InlineFlex;
    case DisplayType::Grid:
        return DisplayType::Grid;
    case DisplayType::InlineGrid:
        return DisplayType::InlineGrid;
    case DisplayType::Inline:
    case DisplayType::InlineBlock:
    case DisplayType::TableRowGroup:
    case DisplayType::TableHeaderGroup:
    case DisplayType::TableFooterGroup:
    case DisplayType::TableRow:
    case DisplayType::TableColumnGroup:
    case DisplayType::TableColumn:
    case DisplayType::TableCell:
    case DisplayType::TableCaption:
        return DisplayType::Block;
    case DisplayType::Contents:
        ASSERT_NOT_REACHED();
        return DisplayType::Contents;
    case DisplayType::None:
        ASSERT_NOT_REACHED();
        return DisplayType::None;
    }
    ASSERT_NOT_REACHED();
    return DisplayType::Block;
}

and then modifying Line 363 to:

style.setEffectiveDisplay(equivalentDisplayForLegend(style));

__________________

@Antti , @Tim , @Alan - any suggestion? It will fix some of these tests.