Bug 11588 – tables misrender because WebKit lacks support for 'cols' attribute

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

[Madhu M]

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.

[Madhu M]

Created attachment 11503 [details]
The html file showing the bug

The attached file shows this issue if we open it in Safari and Firefox

[Madhu M]

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.

[Madhu M]

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.

[David Kilzer (:ddkilzer)]

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.

[Madhu M]

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.

[David Kilzer (:ddkilzer)]

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.

[Darin Adler]

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

[Dave Hyatt]

I'm not convinced this patch is correct without a lot more test cases.

[Dave Hyatt]

It seemss particularly suspicious that the way space was distributed would change so dramatically based off whether any other column types were present.

[Jake Logan]

<rdar://problem/4636741>

[Madhu M]

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

[Madhu M]

Created attachment 13502 [details]
ChangeLog of implementing cols attribute

[Dave Hyatt]

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.

[Madhu M]

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

[Madhu M]

Created attachment 13504 [details]
Layout test result before making changes for 'cols' implementation

[Madhu M]

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

[Madhu M]

Created attachment 13506 [details]
Layout Test Results

Layout test results after implementing the code changes to handle the 'cols' attribute in table tag.

[Madhu M]

Created attachment 13508 [details]
patch for implementing 'cols' attribute in <table> tag

Thanks David

I have removed the tabs and uploading the patch again

[David Kilzer (:ddkilzer)]

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!

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Mark Rowe (bdash)]

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.

[Madhu M]

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

[Oliver Hunt]

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

[Oliver Hunt]

Oops, forgot to say -- we appreciate your contributing, especially to such a complicated area as layout :D

[Dave Hyatt]

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

[Dave Hyatt]

Comment on attachment 13713 [details]
Modified patch for implementation of the cols attribute in Table 

This needs to wait until post-Safari-3.

[Ahmad Saleem]

I am not able to see that this patch landed from Webkit Github via searching with bug number "11588" and from the attached HTML, these are results across all browsers:

*** Safari 15.6 on macOS 12.5 ***

Alert1  view 167

*** Firefox Nightly 105 ***

Alert1  view 175

*** Chrome Canary 106 ***

Alert1  view 177

I am not sure whether this patch can applied now or whether the underlying issue is fixed or not. Appreciate if someone else can have a look and update the bug status accordingly. Thanks!

[Oliver Hunt]

This test case is window size dependent, so I think you'd need to have identical window sizes. Alas window.resizeTo() no longer works, which would make this much easier :-/

[Ahmad Saleem]

(In reply to Oliver Hunt from comment #33)
> This test case is window size dependent, so I think you'd need to have
> identical window sizes. Alas window.resizeTo() no longer works, which would
> make this much easier :-/

I aligned the window width and I get 167 for all browsers (Safari 15.6, Chrome Canary 106 and Firefox Nightly 105). Thanks!

[Oliver Hunt]

(In reply to Ahmad Saleem from comment #34)
> (In reply to Oliver Hunt from comment #33)
> > This test case is window size dependent, so I think you'd need to have
> > identical window sizes. Alas window.resizeTo() no longer works, which would
> > make this much easier :-/
> 
> I aligned the window width and I get 167 for all browsers (Safari 15.6,
> Chrome Canary 106 and Firefox Nightly 105). Thanks!

In that case I think we can close this.