Bug 3240 - Support the frame and rules attributes on HTML tables
: Support the frame and rules attributes on HTML tables
: WebKit
: 412
: All All
: P2 Normal
Assigned To:
: InRadar
  Show dependency treegraph
Reported: 2005-06-01 16:04 PST by
Modified: 2006-09-17 08:22 PST (History)

Patch that implements support for the frame and rules attributes (28.76 KB, patch)
2006-09-16 02:36 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Address some of Eric's concerns (30.57 KB, patch)
2006-09-16 16:41 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Round Three (28.46 KB, patch)
2006-09-16 22:15 PST, Dave Hyatt
beidson: review+
Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2005-06-01 16:04:39 PST
The 'frame' and 'rules' attributes need to be supported on HTML tables.
------- Comment #1 From 2005-06-01 16:05:15 PST -------
Apple Bug: rdar://3481327/
------- Comment #2 From 2006-04-01 17:05:25 PST -------
This week-end, I went into an Internet Cafe and check my own website with Safari 2.02 (416.13).


will indicate that frame attribute is supported. 

Regarding the rules attribute,

In border-collapse: separate
Safari only needs to support rules="all" and rules="none".

In border-collapse: collapse
Safari must support rules="all", rules="none", rules="groups", rules="cols" and rules="rows".

The rationale behind this is given at section 17.6.1 of CSS 2.x:
"In this model [border-collapse: separate model], (...) Rows, columns, row groups, and column groups cannot have borders (i.e., user agents must ignore the border properties for those elements). (...) In the collapsing border model, it is possible to specify borders that surround all or part of a cell, row, row group, column, and column group. Borders for HTML's 'rules' attribute can be specified this way."
CSS 2, Section 17.6.1 The separated borders model (border-collapse)
CSS 2.1, Section 17.6.1 The separated borders model (border-collapse)
------- Comment #3 From 2006-09-14 03:05:51 PST -------
See also https://bugzilla.mozilla.org/show_bug.cgi?id=43178
------- Comment #4 From 2006-09-16 02:36:55 PST -------
Created an attachment (id=10585) [details]
Patch that implements support for the frame and rules attributes
------- Comment #5 From 2006-09-16 02:51:27 PST -------
(From update of attachment 10585 [details])
A few comments:

Glad to see all the m_ changes.

IMO this is unnecessarily verbose:
+    if (p) {
+        HTMLTableElement* table = static_cast<HTMLTableElement*>(p);
+        return table->getSharedRowGroupDecl();
+    }

return static_cast<HTMLTableElement*>(p)->getSharedRowGroupDecl();  works just as well (two places)

It looks like m_firstBody should be a RefPtr

m_head should also be a RefPtr.

and m_foot.

and m_caption

These could all be one line if you wanted:
+            m_frameAttr = true;
+            borders[cTop] = true;
m_frameAttr = borders[cTop] = true;
------- Comment #6 From 2006-09-16 16:41:56 PST -------
Created an attachment (id=10593) [details]
Address some of Eric's concerns

Ok, I cleaned up the static casts and compacted that code.

I did not convert the member variables to RefPtrs.  I started to do so and realized that the code surrounding the caching of these variables is (right now) completely buggy (only the body member is actually being ref'ed right now).  Because the problem is more complicated (and not relevant at all to this patch),  I'm filing a followup bug about making the management of those pointers better (both RefPtr conversion and then making them update properly as well).
------- Comment #7 From 2006-09-16 16:44:39 PST -------


to track the head/body/foot/caption issues.
------- Comment #8 From 2006-09-16 18:22:03 PST -------
(From update of attachment 10593 [details])
html/HTMLTableRowElement.h has no substantial changes.

I don't understand why the hasLocalName is necessary?
I guess HTMLTableColElement us both table columns and column groups?

I'm not sure why this is right?

+    if (attrName == borderAttr || attrName == frameAttr || attrName == rulesAttr || attrName == alignAttr) {
         result = eTable;
         return true;
-    if (attrName == alignAttr) {
-        result = eTable;
-        return false;
-    } 

(but then again, I'm not sure what it does...)

Mitz pointed this out at a possible typo:
+                                         (m_rulesAttr == ColsRules ? "solid'cols" : 

did you mean solid-cols?

This comment isn't really clear to me what it means:+        decl->setStrictParsing(false); // Mapped attributes are just always quirky.

Is that a comment on the behavior of the code? or the use of quirks mode?

It looks like:
+CSSMutableStyleDeclaration* HTMLTableElement::getSharedRowGroupDecl()
+CSSMutableStyleDeclaration* HTMLTableElement::getSharedColGroupDecl()

could be simplified to share a single helper function.  One which just took "colgroups" vs "rowgroups"

All and all this change looks fine. I'm not sure I fully understand it, but the code looks sane.  We could chat a bit more in IRC then I can r+ (or someone else who feels they understand better could do so before me.)
------- Comment #9 From 2006-09-16 21:29:28 PST -------
Regarding the align change.... I hadn't noticed the different return values.  Nice catch.  I need to keep it in a separate block.
------- Comment #10 From 2006-09-16 21:30:27 PST -------
Yeah, I meant solid-cols.  The value doesn't really matter as long as it's unique.
------- Comment #11 From 2006-09-16 21:31:36 PST -------
Yes, HTMLTableColElement is used for both column groups and columns.  Blame the DOM spec designers. :)
------- Comment #12 From 2006-09-16 22:15:56 PST -------
Created an attachment (id=10594) [details]
Round Three

Here we go.  Fixes the alignAttr problem (nice catch) and the typo for solid-cols.  Also combines rows and cols into the same common function.
------- Comment #13 From 2006-09-17 00:19:05 PST -------
(From update of attachment 10594 [details])
Got all of Eric's comments, I couldn't see anything else to comment on myself.  r+
------- Comment #14 From 2006-09-17 00:51:50 PST -------
------- Comment #15 From 2006-09-17 08:22:43 PST -------
(In reply to comment #14)
> Fixed.

See r16399 and r16400-r16402.