Bug 11588 - tables misrender because WebKit lacks support for 'cols' attribute
Summary: tables misrender because WebKit lacks support for 'cols' attribute
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Major
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2006-11-13 05:26 PST by Madhu M
Modified: 2010-06-10 17:26 PDT (History)
2 users (show)

See Also:


Attachments
The html file showing the bug (419 bytes, text/html)
2006-11-13 05:29 PST, Madhu M
no flags Details
This is a sample fix for the issue. (1.42 KB, patch)
2006-12-22 06:40 PST, Madhu M
no flags Details | Formatted Diff | Diff
This contains a sample fix for the offsetwidth issue (1.42 KB, patch)
2006-12-22 07:44 PST, Madhu M
darin: review-
Details | Formatted Diff | Diff
patch for implementing 'cols' attribute in <table> tag (6.08 KB, patch)
2007-03-06 20:51 PST, Madhu M
hyatt: review-
Details | Formatted Diff | Diff
ChangeLog of implementing cols attribute (819.07 KB, text/plain)
2007-03-06 21:00 PST, Madhu M
no flags Details
sample html showing the table layout after implementing the cols attribute (1.81 KB, text/html)
2007-03-06 21:11 PST, Madhu M
no flags Details
Layout test result before making changes for 'cols' implementation (10.86 KB, text/plain)
2007-03-06 21:15 PST, Madhu M
no flags Details
layout test results after making the changes for 'cols' implementation (12.74 KB, text/plain)
2007-03-06 21:16 PST, Madhu M
no flags Details
Layout Test Results (3.88 KB, application/octet-stream)
2007-03-06 21:18 PST, Madhu M
no flags Details
patch for implementing 'cols' attribute in <table> tag (6.97 KB, patch)
2007-03-06 22:47 PST, Madhu M
mrowe: review-
Details | Formatted Diff | Diff
Modified patch for implementation of the cols attribute in Table (21.21 KB, patch)
2007-03-19 18:35 PDT, Madhu M
hyatt: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Madhu M 2006-11-13 05:26:12 PST
If the value of cols and number of <td> in a row is different for a table, the offsetwidth of the <td> element is different in Firefox and Safari. For egÊ

<tableÊ cols ='1' width='100%'Ê >
<tr>
	<td >A</td>	
	<tdÊ id='aabb' />
</tr>
</table>

if we get the offset width of the td (id-'aabb') in safari it gives 108 and in Firefox it gives 1680(depending in the screen size). In safari the width is not dependent on the screen for this element.
Comment 1 Madhu M 2006-11-13 05:29:14 PST
Created attachment 11503 [details]
The html file showing the bug

The attached file shows this issue if we open it in Safari and Firefox
Comment 2 Madhu M 2006-12-22 06:40:28 PST
Created attachment 11965 [details]
This is a sample fix for the issue. 

Please review the sample fix. This contains the fix for the offsetWidth issue.
Comment 3 Madhu M 2006-12-22 06:56:15 PST
If we open the attched file in Safari and Firefox we can see the difference in offsetwidth for the second <td> element. I have tried to make a fix for this. Please review the fix and send me the comments.
Comment 4 David Kilzer (:ddkilzer) 2006-12-22 07:05:18 PST
Comment on attachment 11965 [details]
This is a sample fix for the issue. 

Madhu, please mark attachments as patches and set their "review?" flag if you'd like them reviewed.
Comment 5 Madhu M 2006-12-22 07:44:07 PST
Created attachment 11968 [details]
This contains a sample fix for the offsetwidth issue

if the number of coloumns for auto alignment and the number of  effective columns are the same then  the table width will be getting equally distributed for the columns.
 Please review the fix and suggest me how to write the change log.
Comment 6 David Kilzer (:ddkilzer) 2006-12-22 08:22:32 PST
Comment on attachment 11968 [details]
This contains a sample fix for the offsetwidth issue

The "review+" flag means that the patch has been approved by a qualified reviewer.  Only qualified reviewers should set this flag.

The "review?" flag means that the person creating the patch would like it reviewed.

See Bug 11346 Comment #10 on how to create a ChangeLog entry.
Comment 7 Darin Adler 2006-12-22 17:44:15 PST
Comment on attachment 11968 [details]
This contains a sample fix for the offsetwidth issue

Looks pretty good to me.

I'd like to understand whether the case where numAuto == nEffCols is really a special case, or if the general algorithm needs to be corrected.

This change needs a layout test and change log.

Has someone run the layout tests with this patch in place? does this affect the results of any of the layout tests?

Formatting is not correct in this patch. For example, it uses tabs for indentation, and this:

+ int w = available / nCols -- ;

should be formatted like this:

    int w = available / nCols--;

and this:

+ }else{

should be formatted like this:

    } else {

Because of these concerns, review-.
Comment 8 Dave Hyatt 2006-12-23 10:08:22 PST
I'm not convinced this patch is correct without a lot more test cases.
Comment 9 Dave Hyatt 2006-12-23 10:08:59 PST
It seemss particularly suspicious that the way space was distributed would change so dramatically based off whether any other column types were present.
Comment 10 Jake Logan 2007-02-01 12:46:11 PST
<rdar://problem/4636741>
Comment 11 Madhu M 2007-03-06 20:51:05 PST
Created attachment 13501 [details]
patch for implementing 'cols' attribute in <table> tag

It is found out that this issue is because of the 'cols' attribute in table tag.
This is not handled in Webkit
I have made the changes in webkit code for implementing 'cols'. Please review this patch and give the comments.
I have made the changelog and it is being uploaded
Comment 12 Madhu M 2007-03-06 21:00:11 PST
Created attachment 13502 [details]
ChangeLog of implementing cols attribute
Comment 13 Dave Hyatt 2007-03-06 21:03:50 PST
Comment on attachment 13501 [details]
patch for implementing 'cols' attribute in <table> tag

Please fix the tab indentation to be space instead.  I can't read this until it is formatted properly.
Comment 14 Madhu M 2007-03-06 21:11:33 PST
Created attachment 13503 [details]
sample html showing the table layout after implementing the cols attribute

This html file shows the table layout after implementing the required changes for  handling cols attribute. The table cells will be having equal width if the cols value is equal to or greater than the nuber of TDs. If the cols value is less than the number of TDs, that many TDs equal to the cols value will be having equal width (equivalent to the maximum width among them) and the rest of the TDs will be sharing the remaining available width equally. This is the behavior in Firefox
Comment 15 Madhu M 2007-03-06 21:15:03 PST
Created attachment 13504 [details]
Layout test result before making changes for 'cols' implementation
Comment 16 Madhu M 2007-03-06 21:16:55 PST
Created attachment 13505 [details]
layout test results after making the changes for 'cols' implementation

The test cases failed are having COLS attribute in <table> tag
Comment 17 Madhu M 2007-03-06 21:18:49 PST
Created attachment 13506 [details]
Layout Test Results

Layout test results after implementing the code changes to handle the 'cols' attribute in table tag.
Comment 18 Madhu M 2007-03-06 22:47:58 PST
Created attachment 13508 [details]
patch for implementing 'cols' attribute in <table> tag

Thanks David

I have removed the tabs and uploading the patch again
Comment 19 David Kilzer (:ddkilzer) 2007-03-07 02:40:35 PST
Madhu, please include the changelog entries, the layout tests and the code changes in a single patch.  This is most easily done by doing the following:

1. Run WebKitTools/Scripts/prepare-ChangeLog listing the files/directories that have changed.  (New files should have "svn add" run on them first.)

2. Edit all of the ChangeLog files changed to reference the bug fixed and to describe the changes made.   (Use other changelog entries as examples.)

3. Run WebKitTools/Scripts/svn-create-patch listing the same files/directories that have changed, then redirect the output to a file.

4. Upload the file in Step #3 as the entire patch.

More info here:  http://webkit.org/coding/contributing.html

Thanks!

Comment 20 Darin Adler 2007-03-07 06:39:15 PST
Comment on attachment 13508 [details]
patch for implementing 'cols' attribute in <table> tag

Patch should remove the "// ###" -- that's how the original KDE folks marked places where work needs to be done (today we would probably say "FIXME: need to implement this"). So it should be removed when you implement something.

The patch has tab characters in it. We don't allow tabs and in fact Subversion won't let us commit a patch with tabs in it.
Comment 21 Darin Adler 2007-03-07 06:45:30 PST
The way we normally do this is that there's a single patch containing both the code change, the layout test, and the expected layout test results. Putting 5 separate files up for review is the wrong way to do it. Let me try to fix some of that.
Comment 22 Darin Adler 2007-03-07 07:03:32 PST
Comment on attachment 13503 [details]
sample html showing the table layout after implementing the cols attribute

If this sample is going to be a layout test, then it needs to be put in an appropriate place in the LayoutTests directory, svn add-ed. amd included in a single patch for review with the code change to fix the bug. Clearing the review flag on this standalone file.
Comment 23 Darin Adler 2007-03-07 08:48:49 PST
Comment on attachment 13505 [details]
layout test results after making the changes for 'cols' implementation

The way this is supposed to work is that you supply a patch with corrected layout test results, reflecting any changes caused by your code change. This looks good, but there's no need to review this. Clearing the review flag.
Comment 24 Darin Adler 2007-03-07 08:49:53 PST
Comment on attachment 13504 [details]
Layout test result before making changes for 'cols' implementation

Clearing review flag for the same reason I did on the other test results. See comment above.
Comment 25 Darin Adler 2007-03-07 08:52:43 PST
Comment on attachment 13506 [details]
Layout Test Results

Clearing review flag for the same reason as above. New layout tests, new layout test results, updated layout tests, and updated layout test results should be included in the patch for review alongside the code change.
Comment 26 Mark Rowe (bdash) 2007-03-14 00:32:31 PDT
Comment on attachment 13508 [details]
patch for implementing 'cols' attribute in <table> tag

This patch hasn't addressed many of the previous comments.  It still lacks layout tests, has many tabs throughout it, and includes the now-incorrect "// ###" todo marker used by the KDE folk.  There are many places in which the code doesn't meet our coding style guidlines (<http://webkit.org/coding/coding-style.html>) with regards to placement of & characters and whitespace around "else" keywords.
Comment 27 Madhu M 2007-03-19 18:35:18 PDT
Created attachment 13713 [details]
Modified patch for implementation of the cols attribute in Table 

I have gone through all the comments regarding the procedure of submitting a patch.I think all of them are incorporated in the new patch file submitted. Now I understand the procedure and I will follow the same in future.Sorry for making the mistakes earlier and thanks a lot for the quick responses. Please review this patch and give comments.

Regards

Madhu
Comment 28 Oliver Hunt 2007-04-27 23:55:02 PDT
Comment on attachment 13713 [details]
Modified patch for implementation of the cols attribute in Table 

Trivial issues:
* Refer to the bug number/url in the changelogs -- it makes it easier for us when hunting regressions
* In the main changelog of your patch (WebCore/ChangeLog) put a brief description of what the patch does -- look at other entries to get some idea of what we expect
* Check for "\ No newline at end of file" in your patch, and add the final new lines to the appropriate files -- your layout test source in this case
* you frequently have "}else..." -- there should be a space between } and else, "} else.."


A few things of note:
* HTMLTableElement::attach
I would probably write something along the lines of 
if (renderer() && renderer()->isTable()) {
    RenderTable* table = static_cast<RenderTable*>(renderer());
    table->setCellPadding(m_padding);
    table->setCols(m_cols);
}

* HTMLTableElement.h
Define m_cols with the other locals.  Eg. before the friend class HTMLTableCellElement; declaration

* Random comment
I'm not entirely comfortable with the replication of m_nCols/m_cols in RenderTable and HTMLTableElement -- but it's been so long since i've worked anywhere near so i can't remember how necessary it is/is not.

Unfortunately as i just said i'm nowhere near qualified here to do the logic review for the real layout code in this patch -- that' probably best left to hyatt
Comment 29 Oliver Hunt 2007-04-27 23:56:07 PDT
Oops, forgot to say -- we appreciate your contributing, especially to such a complicated area as layout :D
Comment 30 Dave Hyatt 2007-05-29 02:00:00 PDT
Is there any information that defines how this attribute is supposed to behave?  It's difficult for me to review this without that information.  I have no idea if the behavior being implemented is correct or not.  It seems suspicious to me that only layout is being patched (and only AutoTableLayout at that).
Comment 31 Dave Hyatt 2007-06-26 21:56:25 PDT
Comment on attachment 13713 [details]
Modified patch for implementation of the cols attribute in Table 

This needs to wait until post-Safari-3.