Summary: | DOM table rules are not updated when changed | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brett Wilson (Google) <brettw> | ||||||||
Component: | Tables | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mrowe, rwlbuis | ||||||||
Priority: | P2 | Keywords: | HasReduction, InRadar | ||||||||
Version: | 523.x (Safari 3) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Brett Wilson (Google)
2007-08-01 09:12:48 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.
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.
Created attachment 15878 [details]
Different approach
As discussed with dhyatt, this is probably a better approach.
Cheers,
Rob.
(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 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.
Rob, can you update the patch as per Darin's comments and land it? It's been sitting here for a few weeks now. 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. |