Bug 14848 - DOM table rules are not updated when changed
Summary: DOM table rules are not updated when changed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 523.x (Safari 3)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2007-08-01 09:12 PDT by Brett Wilson (Google)
Modified: 2007-08-25 10:04 PDT (History)
2 users (show)

See Also:


Attachments
Testcase (623 bytes, text/html)
2007-08-01 09:14 PDT, Brett Wilson (Google)
no flags Details
First attempt (3.62 KB, patch)
2007-08-07 00:40 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Different approach (52.48 KB, patch)
2007-08-09 00:51 PDT, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2007-08-01 09:12:48 PDT
Toggling the "rules" attribute of a table in JavaScript does not update the table.
Comment 1 Brett Wilson (Google) 2007-08-01 09:14:27 PDT
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.
Comment 2 Rob Buis 2007-08-07 00:40:14 PDT
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.
Comment 3 Rob Buis 2007-08-09 00:51:05 PDT
Created attachment 15878 [details]
Different approach

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

Rob.
Comment 4 David Kilzer (:ddkilzer) 2007-08-10 06:01:31 PDT
(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

Comment 5 Darin Adler 2007-08-13 11:51:34 PDT
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.
Comment 6 Mark Rowe (bdash) 2007-08-25 05:30:00 PDT
Rob, can you update the patch as per Darin's comments and land it?  It's been sitting here for a few weeks now.
Comment 7 Rob Buis 2007-08-25 05:42:44 PDT
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.
Comment 8 Rob Buis 2007-08-25 10:03:30 PDT
Landed in r25246.
Comment 9 Mark Rowe (bdash) 2007-08-25 10:04:27 PDT
<rdar://problem/5437865>