Bug 18092 – table's text aligned on top instead of center because of rowspan

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

[jasneet]

I Steps:
Go to 
http://fleurs.3suisses.fr/

II Issue:
Notice the alignment of the description of the featured item. In Safari, the text is immediately under the price whereas in FF, IE and Opera, it is centered in the section.

III Conclusion:
The misalignment is caused by the rowspan=4 defined within the table. There are technically only 3 rows so if I change the rowspan = 3, then Safari displays correctly.

IV Other browsers:
IE7: ok
FF3: ok
Opera9.24: ok

V Nightly tested: 31238

[jasneet]

Created attachment 20062 [details]
screenshot

[jasneet]

Created attachment 20063 [details]
reduction

[Suchit Agrawal]

Created attachment 198708 [details]
Patch

[Suchit Agrawal]

(In reply to comment #3)
> Created an attachment (id=198708) [details]
> Patch
> 

Rowspan height is not applied to last row when rowspan is more than 'rows remains below rowspan cell + 1'.

If row contains row span cell and it is last row of the table then rowspan height applied to last row of the table.

If row contains rowspan cell than it checks that is there any valid row present below this row, if not then rowapn height applied to this row.

Result of test cases are matching with IE and mozilla except some test cases (present in fast/table/Rowspan-value-more-than-number-of-rows-present.html) are not matching with mozilla but as per rowspan property, mozilla is not displaying proper in these cases.

[Suchit Agrawal]

Created attachment 198726 [details]
Patch

[Suchit Agrawal]

(In reply to comment #5)
> Created an attachment (id=198726) [details]
> Patch
> 

Rowspan height is not applied to last row when rowspan is more than 'rows remains below rowspan cell + 1'.

If row contains row span cell and it is last row of the table then rowspan height applied to last row of the table.

If row contains rowspan cell than it checks that is there any valid row present below this row, if not then rowapn height applied to this row.

Result of test cases are matching with IE and mozilla except some test cases (present in fast/table/Rowspan-value-more-than-number-of-rows-present.html) are not matching with mozilla but as per rowspan property, mozilla is not displaying proper in these cases.

[Suchit Agrawal]

Beth, Please let me know your feedback about the patch. Thank you

[Beth Dakin]

Comment on attachment 198726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=198726&action=review

Ultimately, I find this patch a little confusing right now. I would like to see another pass with clearer comments.

> Source/WebCore/ChangeLog:9
> +        then rowspan height is not applied to last row.

This description is very confusing. Please find a more clear way to say this.

> Source/WebCore/rendering/RenderTableSection.cpp:303
> +                    // than 'rows below rowspan cell + 1'

Just like I said in the Changelog, this description is very confusing. Please find a more clear way to say this.

> Source/WebCore/rendering/RenderTableSection.cpp:308
> +                        while (nextRowCell.cells.size() && nextRowCell.cells[0]->rowSpan() > 1 && nextRowCell.cells[0]->rowIndex() < (r + 1)) {

The code would be more clear if you added a comment. Based on your description in the bug, you are trying to deterring if there is a valid next row, correct? Please say something like that here.

[Suchit Agrawal]

Created attachment 199010 [details]
Patch

[Suchit Agrawal]

(In reply to comment #9)
> Created an attachment (id=199010) [details]
> Patch
> 
Updated the patch with clear description in ChangeLog file and comments on changed lines in source file.

[Suchit Agrawal]

Created attachment 199013 [details]
Patch

[Suchit Agrawal]

(In reply to comment #11)
> Created an attachment (id=199013) [details]
> Patch
> 
Updated the patch with clear description in ChangeLog file and comments on changed lines in source file.

[Suchit Agrawal]

Beth, please review this patch and let me know your comments for the patch. Thanks

[Beth Dakin]

Comment on attachment 199013 [details]
Patch

The new explanation and comments are so much easier to follow! Thanks for re-working them.

[Suchit Agrawal]

(In reply to comment #14)
> (From update of attachment 199013 [details])
> The new explanation and comments are so much easier to follow! Thanks for re-working them.
> 
Hello Beth, Thank you.
How will it become commit plus? Do I have permission to change the commit-queue as plus.

[WebKit Commit Bot]

Comment on attachment 199013 [details]
Patch

Clearing flags on attachment: 199013

Committed r148944: <http://trac.webkit.org/changeset/148944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.