Bug 255853 – Zero rowspan should span all rows

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Tables

[Ahmad Saleem]

Hi Team,

While going through Blink's commit, I came across another potential to merge or evaluate which we are failing and splitting from another bug (https://bugs.webkit.org/show_bug.cgi?id=10300#c8):

Blink Commit - https://chromium.googlesource.com/chromium/src.git/+/93609285fd2b0f7fef0d27ad1cf2eca4b9956fc8

WPT Tests - https://wpt.fyi/results/css/css-tables?label=master&label=experimental&aligned=&q=zero-rowspan-00

Thanks!

[Radar WebKit Bug Importer]

<rdar://problem/108726847>

[Ahmad Saleem]

It is easy to do but I am stuck on one error:

>> Source/WebCore/rendering/RenderTableCell.h:

Line 46: unsigned parsedRowSpan() const;

and update 'old' rowSpan to 'rename' and introduce new 'rowSpan' below:


inline unsigned RenderTableCell::parsedRowSpan() const
{
    if (!m_hasRowSpan)
        return 1;
    return parseRowSpanFromDOM();
}
inline unsigned RenderTableCell::rowSpan() const
{
    unsigned rowSpan = parsedRowSpan();
    if (!rowSpan) {
        ASSERT(!section()->needsCellRecalc());
        rowSpan = section()->numRows() - rowIndex();
    }
    return std::min<unsigned>(rowSpan, maxRowIndex);
}

______

>> Source/WebCore/rendering/RenderTableSection.cpp:

In function: (recalcCells) Line 1351:

 bool resizedGrid = false;
    for (RenderTableRow* row = firstRow(); row; row = row->nextRow()) {
        unsigned insertionRow = m_cRow;
        row->setRowIndex(insertionRow);
        setRowLogicalHeightToRowStyleLogicalHeight(m_grid[insertionRow]);
        for (RenderTableCell* cell = row->firstCell(); cell; cell = cell->nextCell()) {
            // For rowspan, "the value zero means that the cell is to span all the
            // remaining rows in the row group." Calculate the size of the full
            // row grid now so that we can use it to count the remaining rows in
            if (!cell->parsedRowSpan() && !resizedGrid) {
                unsigned m_cRow = row->rowIndex() + 1;
                for (auto* remainingRow = row; remainingRow; remainingRow = remainingRow->nextRow())
                    m_cRow++;
                    ensureRows(m_cRow);
                    resizedGrid = true;
            }
        addCell(cell, row);
        }
    }


_______


>> Source/WebCore/rendering/RenderTableRow.cpp:

In function (didInsertTableCell) <<--- here I am stuck:

if (auto* section = this->section()) {
        section->addCell(&child, this);
        if (beforeChild || nextRow() || !cell.parsedRowSpan())

--> Here - I get uninitialised or undefined for 'cell'.

Looking into Blink patch, it is defined as:

 LayoutTableCell* cell = ToLayoutTableCell(child);

which usually means:


RenderTableCell* (or auto*) cell = downcast<RenderTableCell>(child);

^ but it is giving error that it is 'static_assert failed due to requirement
      '!std::is_same_v<WebCore::RenderTableCell, WebCore::RenderTableCell>'
      "Unnecessary cast to same type"' error. :-(

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #2)
> It is easy to do but I am stuck on one error:
> 
> >> Source/WebCore/rendering/RenderTableCell.h:
> 
> Line 46: unsigned parsedRowSpan() const;
> 
> and update 'old' rowSpan to 'rename' and introduce new 'rowSpan' below:
> 
> 
> inline unsigned RenderTableCell::parsedRowSpan() const
> {
>     if (!m_hasRowSpan)
>         return 1;
>     return parseRowSpanFromDOM();
> }
> inline unsigned RenderTableCell::rowSpan() const
> {
>     unsigned rowSpan = parsedRowSpan();
>     if (!rowSpan) {
>         ASSERT(!section()->needsCellRecalc());
>         rowSpan = section()->numRows() - rowIndex();
>     }
>     return std::min<unsigned>(rowSpan, maxRowIndex);
> }
> 
> ______
> 
> >> Source/WebCore/rendering/RenderTableSection.cpp:
> 
> In function: (recalcCells) Line 1351:
> 
>  bool resizedGrid = false;
>     for (RenderTableRow* row = firstRow(); row; row = row->nextRow()) {
>         unsigned insertionRow = m_cRow;
>         row->setRowIndex(insertionRow);
>         setRowLogicalHeightToRowStyleLogicalHeight(m_grid[insertionRow]);
>         for (RenderTableCell* cell = row->firstCell(); cell; cell =
> cell->nextCell()) {
>             // For rowspan, "the value zero means that the cell is to span
> all the
>             // remaining rows in the row group." Calculate the size of the
> full
>             // row grid now so that we can use it to count the remaining
> rows in
>             if (!cell->parsedRowSpan() && !resizedGrid) {
>                 unsigned m_cRow = row->rowIndex() + 1;
>                 for (auto* remainingRow = row; remainingRow; remainingRow =
> remainingRow->nextRow())
>                     m_cRow++;
>                     ensureRows(m_cRow);
>                     resizedGrid = true;
>             }
>         addCell(cell, row);
>         }
>     }
> 
> 
> _______
> 
> 
> >> Source/WebCore/rendering/RenderTableRow.cpp:
> 
> In function (didInsertTableCell) <<--- here I am stuck:
> 
> if (auto* section = this->section()) {
>         section->addCell(&child, this);
>         if (beforeChild || nextRow() || !cell.parsedRowSpan())
> 
> --> Here - I get uninitialised or undefined for 'cell'.
> 
> Looking into Blink patch, it is defined as:
> 
>  LayoutTableCell* cell = ToLayoutTableCell(child);
> 
> which usually means:
> 
> 
> RenderTableCell* (or auto*) cell = downcast<RenderTableCell>(child);
> 
> ^ but it is giving error that it is 'static_assert failed due to requirement
>       '!std::is_same_v<WebCore::RenderTableCell, WebCore::RenderTableCell>'
>       "Unnecessary cast to same type"' error. :-(

I fixed it by doing:

>> Source/WebCore/rendering/RenderTableRow.cpp:

void RenderTableRow::didInsertTableCell(RenderTableCell& child, RenderObject* beforeChild)
{
    // Generated content can result in us having a null section so make sure to null check our parent.
    if (auto* section = this->section()) {
        section->addCell(&child, this);
        if (beforeChild || nextRow() || !child.parsedRowSpan())
            section->setNeedsCellRecalc();
    }
    if (auto* table = this->table())
        table->invalidateCollapsedBorders();
}

Although, it compiles, we don't pass the WPT test case, could be due to this:

https://searchfox.org/wubkat/source/Source/WebCore/html/HTMLTableCellElement.cpp#72


unsigned HTMLTableCellElement::rowSpan() const
{
    // FIXME: a rowSpan equal to 0 should be allowed, and mean that the cell is to span all the remaining rows in the row group.
    return std::max(1u, rowSpanForBindings());
}

[Ahmad Saleem]

From Comment 03:

I tried doing following:

unsigned HTMLTableCellElement::rowSpanForBindings() const
{
    unsigned value = 0;
    if (!clampHTMLNonNegativeIntegerToRange(attributeWithoutSynchronization(rowspanAttr), minRowspan, maxRowspan, value))
        return defaultRowspan;
    return value;
}

^ Still no progress.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #4)
> From Comment 03:
> 
> I tried doing following:
> 
> unsigned HTMLTableCellElement::rowSpanForBindings() const
> {
>     unsigned value = 0;
>     if
> (!
> clampHTMLNonNegativeIntegerToRange(attributeWithoutSynchronization(rowspanAtt
> r), minRowspan, maxRowspan, value))
>         return defaultRowspan;
>     return value;
> }
> 
> ^ Still no progress.

I modified below as well, since now we account for 'defaultRowSpan' in above function anyway but still not fixed.

unsigned HTMLTableCellElement::rowSpan() const
{
    return rowSpanForBindings();
}

_______

@Alan / @Simon - any help or input?

[Karl Dubost]

Created attachment 467195 [details]
rendering in Safari, firefox, chrome

rendering for 
http://wpt.live/css/css-tables/zero-rowspan-001.html
http://wpt.live/css/css-tables/zero-rowspan-002.html

to better see the differences. 

The HTML spec says:
https://html.spec.whatwg.org/multipage/tables.html#attributes-common-to-td-and-th-elements

> The td and th elements may also have a rowspan content attribute specified, whose value must be a valid non-negative integer less than or equal to 65534. 

and 

> For this attribute, the value zero means that the cell is to span all the remaining rows in the row group.


In https://html.spec.whatwg.org/multipage/tables.html#forming-a-table


section: The algorithm for processing rows

steps 9 and 10.

9. If the current cell has a rowspan attribute, then parse that attribute's value, and let rowspan be the result.

If parsing that value failed or if the attribute is absent, then let rowspan be 1, instead.

If rowspan is greater than 65534, let it be 65534 instead.

10. If rowspan is zero and the table element's node document is not set to quirks mode, then let cell grows downward be true, and set rowspan to 1. Otherwise, let cell grows downward be false.

[Karl Dubost]

*** This bug has been marked as a duplicate of bug 185341 ***