Bug 14848 – DOM table rules are not updated when changed

• Component: Tables
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

Toggling the "rules" attribute of a table in JavaScript does not update the table.

[Brett Wilson (Google)]

Created attachment 15779 [details]
Testcase

This table initially has no rules. Setting the rules attribute will properly update the table. However, setting the rules attribute *again* to a different value will not update the table.

WebKit doesn't seem to be treating an update to the rules attribute if it already exists as something that requires refreshing the layout or DOM.

[Rob Buis]

Created attachment 15856 [details]
First attempt

Not doing layout tests yet since I need to know first whether it is the right approach :)
Cheers,

Rob.

[Rob Buis]

Created attachment 15878 [details]
Different approach

As discussed with dhyatt, this is probably a better approach.
Cheers,

Rob.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Created an attachment (id=15878) [edit]
> Different approach

Small typo in WebCore/ChangeLog:

+        On a dynamic rules attr change, mark the tabel cells and

[Darin Adler]

Comment on attachment 15878 [details]
Different approach

I think setTableCellsChanged could be written so it's not recursive, but I guess it's OK as-is. I do worry about adding more and more recursive algorithms that could be a problem if someone makes a deeply nested DOM tree, but Hyatt tells me not to worry.

+    bool result = false;

I think "cellChanged" might be a better name for this local variable. Also, I'd put a space after this to match the paragraphing of the rest of the function.

+    if (n->hasTagName(theadTag) || n->hasTagName(tbodyTag) ||
+        n->hasTagName(tfootTag) || n->hasTagName(trTag) ||
+         n->hasTagName(thTag)) {

I'd prefer a helper function for this (inline if necessary) -- it's hard to read a long list like this.

+        for (Node *child = n->firstChild();child;child = child->nextSibling()) {
+            result |= setTableCellsChanged(child);
+        }

There should not be space between Node and the * after it. There should be spaces after the semicolons. There should not be braces around the single line block inside the loop.

+    } else if (n->hasTagName(tdTag)) {
+        result = true;
+    }

We normally don't put braces around single line statements like this one. Putting the tdTag case first before the other tags might make the formatting easier to read.

What about cases where we don't have actual <td> elements, but rather things styled as table-cell by CSS. Does this bug happen in that case?

r=me anyway, even though I had a lot of questions.

[Mark Rowe (bdash)]

Rob, can you update the patch as per Darin's comments and land it?  It's been sitting here for a few weeks now.

[Rob Buis]

Hi Mark,

(In reply to comment #6)
> Rob, can you update the patch as per Darin's comments and land it?  It's been
> sitting here for a few weeks now.

It is on my TODO list for the weekend.
Cheers,

Rob.

[Rob Buis]

Landed in r25246.

[Mark Rowe (bdash)]

<rdar://problem/5437865>