Bug 3240 - Support the frame and rules attributes on HTML tables
Summary: Support the frame and rules attributes on HTML tables
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 412
Hardware: All All
: P2 Normal
Assignee: Dave Hyatt
Keywords: InRadar
Depends on:
Reported: 2005-06-01 16:04 PDT by Dave Hyatt
Modified: 2006-09-17 08:22 PDT (History)
4 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2005-06-01 16:04:39 PDT
The 'frame' and 'rules' attributes need to be supported on HTML tables.
Comment 1 Dave Hyatt 2005-06-01 16:05:15 PDT
Apple Bug: rdar://3481327/
Comment 2 Gérard Talbot 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 fantasai 2006-09-14 03:05:51 PDT
See also https://bugzilla.mozilla.org/show_bug.cgi?id=43178
Comment 4 Dave Hyatt 2006-09-16 02:36:55 PDT
Created attachment 10585 [details]
Patch that implements support for the frame and rules attributes
Comment 5 Eric Seidel 2006-09-16 02:51:27 PDT
Comment on attachment 10585 [details]
Patch that implements support for the frame and rules attributes

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 Dave Hyatt 2006-09-16 16:41:56 PDT
Created attachment 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 Dave Hyatt 2006-09-16 16:44:39 PDT


to track the head/body/foot/caption issues.
Comment 8 Eric Seidel 2006-09-16 18:22:03 PDT
Comment on attachment 10593 [details]
Address some of Eric's concerns

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 Dave Hyatt 2006-09-16 21:29:28 PDT
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 Dave Hyatt 2006-09-16 21:30:27 PDT
Yeah, I meant solid-cols.  The value doesn't really matter as long as it's unique.
Comment 11 Dave Hyatt 2006-09-16 21:31:36 PDT
Yes, HTMLTableColElement is used for both column groups and columns.  Blame the DOM spec designers. :)
Comment 12 Dave Hyatt 2006-09-16 22:15:56 PDT
Created attachment 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 Brady Eidson 2006-09-17 00:19:05 PDT
Comment on attachment 10594 [details]
Round Three

Got all of Eric's comments, I couldn't see anything else to comment on myself.  r+
Comment 14 Dave Hyatt 2006-09-17 00:51:50 PDT
Comment 15 David Kilzer (:ddkilzer) 2006-09-17 08:22:43 PDT
(In reply to comment #14)
> Fixed.

See r16399 and r16400-r16402.