Bug 71944 – CSS 2.1 failure: outline-color-applies-to* tests fail

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Robert Hogan]

WebKit isn't painting the outline on RenderTableSection and RenderTableRow

[Robert Hogan]

Created attachment 114346 [details]
Patch

[WebKit Review Bot]

Comment on attachment 114346 [details]
Patch

Attachment 114346 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10395082

New failing tests:
inspector/debugger/scripts-sorting.html
fast/transforms/transform-table-row.html

[Robert Hogan]

Created attachment 120883 [details]
Patch

[Julien Chaffraix]

Comment on attachment 120883 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=120883&action=review

Part of the approach needs to be re-thought to avoid painting twice.

> Source/WebCore/rendering/RenderTableRow.cpp:232
>      if (!layer())
>          return;

This check seems useless: the ASSERT was never reached and guarantees that we do have a layer.

Also it looks like your code would like to run even if we have a layer which seem wrong.

> Source/WebCore/rendering/RenderTableSection.cpp:962
> +            if (child->isTableRow() && child->hasOutline() && child->style()->visibility() == VISIBLE)
> +                child->paint(paintInfo, paintOffset);

This looks wrong.

You can paint your table rows and cells through 2 code paths: either through a self-painting layer or directly through RenderTableSection::paintObject (which actually paints cells directly ie bypassing some of the row's painting code). If you look closely at the painting code in RenderTableSection and RenderTableRow, there are checks to avoid calling one code path from the other one. Here you are calling RenderTableRow::paint() without caring which code path you are on so you will either trigger the ASSERT or paint twice. I don't know if the ASSERT is covered but please add a test with an outline on a row with a self-painting layer if it is not!

By patching RenderTableRow::paint(), you have fixed part of the self-painting layer code path (the one involving a row's self painting layer). However the other code path is already calling RenderTableCell::paint and thus would need to be patched to account for the row's outline.

[Robert Hogan]

Created attachment 121585 [details]
Patch

[Robert Hogan]

(In reply to comment #5)
> Created an attachment (id=121585) [details]
> Patch

Hi Julien,

Hopefully this is an improvement! I opted to share RenderTableRow::paint() between rows with and without self-painting layers rather than introduce a wrapper function. Hope it's the right way..

[Julien Chaffraix]

Comment on attachment 121585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121585&action=review

CSS does not define how multiple outlines should be handled. Here it looks like you are doing the opposite of CSS 2.1 table's layers (17.5.1) which makes me wonder if our ordering is right. Did you check what FF or IE are doing when there are multiple outlines on different parts of your table?

> Source/WebCore/rendering/RenderTableRow.cpp:229
> -    ASSERT(hasSelfPaintingLayer());
> -    if (!layer())
> +    PaintPhase paintPhase = paintInfo.phase;
> +    if ((paintPhase == PaintPhaseOutline || paintPhase == PaintPhaseSelfOutline) && style()->visibility() == VISIBLE)
> +        paintOutline(paintInfo.context, LayoutRect(paintOffset, size()));
> +
> +    if (!hasSelfPaintingLayer())
>          return;
> +

I don't really like this change. Before you had a clear separation between the 2 paths and now, it's less clear. Also you special-case outline here which looks wrong: why don't you just call the appropriate paintOutline() function in the caller instead of patching here?

> Source/WebCore/rendering/RenderTableSection.cpp:1014
> +    if ((paintInfo.phase == PaintPhaseOutline || paintInfo.phase == PaintPhaseSelfOutline) && cell) {

Normally we prefer early returns.

> Source/WebCore/rendering/RenderTableSection.cpp:1108
> +                    RenderTableCell* cell = 0;
>                      for (unsigned c = startcol; c < endcol; c++) {
>                          CellStruct& current = cellAt(r, c);
> -                        RenderTableCell* cell = current.primaryCell();
> +                        cell = current.primaryCell();
>                          if (!cell || (r > startrow && primaryCellAt(r - 1, c) == cell) || (c > startcol && primaryCellAt(r, c - 1) == cell))
>                              continue;
>                          paintCell(cell, paintInfo, paintOffset);
>                      }
> +                    paintRowOutline(cell, paintInfo, paintOffset);

This is flawed and you could forget to properly paint your outline if your last CellStruct does not have a RenderTableCell (I think a trailing rowSpan could do that). You can get your row renderer pretty easily using the index r:

RenderTableRow* row = m_grid[r].rowRenderer;

> Source/WebCore/rendering/RenderTableSection.cpp:1141
> +                paintRowOutline(cell, paintInfo, paintOffset);

Ditto here too.

[Robert Hogan]

(In reply to comment #7)
> (From update of attachment 121585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121585&action=review
> 
> CSS does not define how multiple outlines should be handled. Here it looks like you are doing the opposite of CSS 2.1 table's layers (17.5.1) which makes me wonder if our ordering is right. Did you check what FF or IE are doing when there are multiple outlines on different parts of your table?

The results vary wildly! FF paints each outline separately so that you see all of them. Opera allows the cell outline to trump all the others. WebKit lets the table outline win at the table borders but the cell outline win in internal borders.

Is Opera correct here? Does this seem like a separate bug to you?


> 
> > Source/WebCore/rendering/RenderTableRow.cpp:229
> > -    ASSERT(hasSelfPaintingLayer());
> > -    if (!layer())
> > +    PaintPhase paintPhase = paintInfo.phase;
> > +    if ((paintPhase == PaintPhaseOutline || paintPhase == PaintPhaseSelfOutline) && style()->visibility() == VISIBLE)
> > +        paintOutline(paintInfo.context, LayoutRect(paintOffset, size()));
> > +
> > +    if (!hasSelfPaintingLayer())
> >          return;
> > +
> 
> I don't really like this change. Before you had a clear separation between the 2 paths and now, it's less clear. Also you special-case outline here which looks wrong: why don't you just call the appropriate paintOutline() function in the caller instead of patching here?

I suspected you'd feel that way - paintOutline() is protected, so the caller can't use it. paintOutlineForRow() it is I guess..

[Robert Hogan]

Created attachment 121869 [details]
Multiple Outlines

Results differ across FF, Opera and WebKit. Suspect Opera is correct here.

[Julien Chaffraix]

Comment on attachment 121585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121585&action=review

> Does this seem like a separate bug to you?

Let's just move the multiple outline case to another bug as there does not seem to be any consensus among engines and it is undefined in CSS.

>>> Source/WebCore/rendering/RenderTableRow.cpp:229
>>> +
>> 
>> I don't really like this change. Before you had a clear separation between the 2 paths and now, it's less clear. Also you special-case outline here which looks wrong: why don't you just call the appropriate paintOutline() function in the caller instead of patching here?
> 
> I suspected you'd feel that way - paintOutline() is protected, so the caller can't use it. paintOutlineForRow() it is I guess..

There is still 2 sane ways to do the same: make paintOutlineForRow a public member or just inline paintOutlineForRow (modified to call paintOutline) inside RenderTableSection::paintObject now we get the RenderTableRow directly.

[Robert Hogan]

Created attachment 121898 [details]
Patch

[Julien Chaffraix]

Comment on attachment 121898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121898&action=review

> Source/WebCore/rendering/RenderTableRow.cpp:221
> +void RenderTableRow::paintOutlineForRow(PaintInfo& paintInfo, const LayoutPoint& paintOffset)

Nit: paintOutlineForRowIfNeeded may be better as you may not paint it?

> Source/WebCore/rendering/RenderTableRow.cpp:225
> +    if ((paintPhase != PaintPhaseOutline && paintPhase != PaintPhaseSelfOutline) || style()->visibility() != VISIBLE)
> +        return;

I was the one to mention the early return but on second thoughts it is inconsistent with the rest of the painting code and thus not that great. Better to be consistent here than following my mumblings!

> Source/WebCore/rendering/RenderTableSection.cpp:1092
> +                    if (row && !row->hasSelfPaintingLayer())

I don't think it is possible for |row| to be NULL here as the recalcCells was called. Is the |row| check really required or can it be removed?

> Source/WebCore/rendering/RenderTableSection.cpp:1115
> +                if (row && !row->hasSelfPaintingLayer())

Same question here.

[Robert Hogan]

Committed r105021: <http://trac.webkit.org/changeset/105021>

[Julien Chaffraix]

*** Bug 35813 has been marked as a duplicate of this bug. ***