Bug 3240 – Support the frame and rules attributes on HTML tables

• Status: RESOLVED
• Resolution: FIXED
• Component: Tables

[Dave Hyatt]

The 'frame' and 'rules' attributes need to be supported on HTML tables.

[Dave Hyatt]

Apple Bug: rdar://3481327/

[Gérard Talbot (no longer involved)]

This week-end, I went into an Internet Cafe and check my own website with Safari 2.02 (416.13).

http://www.gtalbot.org/DHTMLSection/DynamicTableFormatting.html

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)
http://www.w3.org/TR/CSS2/tables.html#separated-borders
CSS 2.1, Section 17.6.1 The separated borders model (border-collapse)
http://www.w3.org/TR/CSS21/tables.html#separated-borders

[fantasai]

See also https://bugzilla.mozilla.org/show_bug.cgi?id=43178

[Dave Hyatt]

Created attachment 10585 [details]
Patch that implements support for the frame and rules attributes

[Eric Seidel (no email)]

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;

[Dave Hyatt]

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).

[Dave Hyatt]

Filed 

http://bugzilla.opendarwin.org/show_bug.cgi?id=10892

to track the head/body/foot/caption issues.

[Eric Seidel (no email)]

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.)

[Dave Hyatt]

Regarding the align change.... I hadn't noticed the different return values.  Nice catch.  I need to keep it in a separate block.

[Dave Hyatt]

Yeah, I meant solid-cols.  The value doesn't really matter as long as it's unique.

[Dave Hyatt]

Yes, HTMLTableColElement is used for both column groups and columns.  Blame the DOM spec designers. :)

[Dave Hyatt]

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.

[Brady Eidson]

Comment on attachment 10594 [details]
Round Three

Got all of Eric's comments, I couldn't see anything else to comment on myself.  r+

[Dave Hyatt]

Fixed.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> Fixed.

See r16399 and r16400-r16402.