Fixing rowspan=0 on tables on WebKit.

Last week, I mentioned there were easy ways to fix or help the WebKit project.

Find The Bug

In January, looking at the FIXME: mentions on the WebKit project, I found this piece of code:

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());
}

Searching on bugs.webkit.org, I found this bug opened by Simon Fraser on May 5, 2018: rowspan="0" results in different table layout than Firefox/Chrome. Would I be able to solve it?

Test The Bug

The first task is very simple. Understand what are the different renderings in between browsers.

Simon had already created a testcase and Ahmad had created a screenshot for it showing the results of the testcase in Safari, Firefox and Chrome. This work was already done. If they had been missing, that would have been my first step.

Read The Specification

For having a better understanding of the issue, it is useful to read the specification related to this bug. In this case, the relevant information was in the HTML specification, where rowspan attribute on td/th elements is described. This is the text we need:

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. For this attribute, the value zero means that the cell is to span all the remaining rows in the row group.

Create More Tests

Let's take a normal simple table which is 3 by 3.

<table border="1">
  <tr><td>A1</td><td>B1</td><td>C1</td></tr>
  <tr><td>A2</td><td>B2</td><td>C2</td></tr>
  <tr><td>A3</td><td>B3</td><td>C3</td></tr>
</table>

We might want to make the first cell overlapping the 3 rows of the tables. A way to do that is to set rowspan="3" because there are 3 rows.

<table border="1">
  <tr><td rowspand="3">A1</td><td>B1</td><td>C1</td></tr>
  <tr>                        <td>B2</td><td>C2</td></tr>
  <tr>                        <td>B3</td><td>C3</td></tr>
</table>

This will create a table were the first column will overlap the 3 rows. This is already working as expected in all rendering engines : WebKit, Gecko and Blink. So far, so good.

Think About The Logic

I learned from reading the specification that rowspan had a maximum value: 65534.

My initial train of thoughts was:

1. compute the number of rows the table.
2. parse the value rowspan value
3. when the value is 0, replace it with the number of rows.

It seemed too convoluted. Would it be possible to use the maximum value for rowspan? The specification was saying "span all the remaining rows in the row group".

I experimented with a bigger rowspan value than the number of rows. For example, put the value 30 on a 3 rows table.

<table border="1">
  <tr><td rowspand="30">A1</td><td>B1</td><td>C1</td></tr>
  <tr>                         <td>B2</td><td>C2</td></tr>
  <tr>                         <td>B3</td><td>C3</td></tr>
</table>

I checked in Firefox, Chrome, and Safari. I got the same rendering. We were on the right track. Let's use the maximum value for rowspan.

I made a test case with additional examples to be able to check in different browsers the behavior:

Fixing The Code

We just had to try to change the C++ code. My patch was

diff --git a/Source/WebCore/html/HTMLTableCellElement.cpp b/Source/WebCore/html/HTMLTableCellElement.cpp
index 256c816acc37b..65450c01e369a 100644
--- a/Source/WebCore/html/HTMLTableCellElement.cpp
+++ b/Source/WebCore/html/HTMLTableCellElement.cpp
@@ -59,8 +59,14 @@ unsigned HTMLTableCellElement::colSpan() const

 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());
+    unsigned rowSpanValue = rowSpanForBindings();
+    // when rowspan=0, the HTML spec says it should apply to the full remaining rows.
+    // In https://html.spec.whatwg.org/multipage/tables.html#attr-tdth-rowspan
+    // > For this attribute, the value zero means that the cell is
+    // > to span all the remaining rows in the row group.
+    if (!rowSpanValue)
+        return maxRowspan;
+    return std::max(1u, rowSpanValue);
 }

 unsigned HTMLTableCellElement::rowSpanForBindings() const

If rowspan was 0, just give the maximum value which is defined in HTMLTableCellElement.h.

I compiled the code change and verified the results:

(note for the careful reader the last table legend is wrong, it should be rowspan="3")

This was fixed! A couple of tests needed to be rebaselined. I was ready to send a Pull Request for this bug.

What Is Next?

The fix is not yet available on the current version of Safari, but you can experiment it with Safari Technology Preview (STP 213 Release Notes).

The biggest part of fixing the bugs is researching, testing different HTML scenario without even touching the C++ code, etc. I'm not a C++ programmer, but time to time I can find bugs that are easy enough to understand that I can fix them. I hope this makes it easier for you to understand and encourage you to look at other bugs.

Note also, that it is not always necessary to fix until modifying everything. Sometimes, just creating testscases, screenshots, pointing to the right places in the specifications, creating the WPT test cases covering this bug are all super useful.

PS: Doing all this work, I found also about the behavior of colspan which is interoperable (same behavior in all browsers), but which I find illogical comparing to the behavior of rowspan.

Otsukare!