Summary: | Support the frame and rules attributes on HTML tables | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Hyatt <hyatt> | ||||||||
Component: | Tables | Assignee: | Dave Hyatt <hyatt> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | browserbugs2, cgriego, fantasai.bugs, ian | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 412 | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Dave Hyatt
2005-06-01 16:04:39 PDT
Apple Bug: rdar://3481327/ 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 Created attachment 10585 [details]
Patch that implements support for the frame and rules attributes
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;
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).
Filed http://bugzilla.opendarwin.org/show_bug.cgi?id=10892 to track the head/body/foot/caption issues. 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.)
Regarding the align change.... I hadn't noticed the different return values. Nice catch. I need to keep it in a separate block. Yeah, I meant solid-cols. The value doesn't really matter as long as it's unique. Yes, HTMLTableColElement is used for both column groups and columns. Blame the DOM spec designers. :) 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 on attachment 10594 [details]
Round Three
Got all of Eric's comments, I couldn't see anything else to comment on myself. r+
Fixed. (In reply to comment #14) > Fixed. See r16399 and r16400-r16402. |