Bug 18092 - table's text aligned on top instead of center because of rowspan
Summary: table's text aligned on top instead of center because of rowspan
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tables (show other bugs)
Version: 525.x (Safari 3.1)
Hardware: PC Windows XP
: P2 Normal
Assignee: Nobody
URL: http://fleurs.3suisses.fr/
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2008-03-25 17:26 PDT by jasneet
Modified: 2013-04-22 23:35 PDT (History)
10 users (show)

See Also:


Attachments
screenshot (237.46 KB, image/jpeg)
2008-03-25 17:26 PDT, jasneet
no flags Details
reduction (602 bytes, text/html)
2008-03-25 17:27 PDT, jasneet
no flags Details
Patch (902.17 KB, patch)
2013-04-18 03:20 PDT, Suchit Agrawal
no flags Details | Formatted Diff | Diff
Patch (900.91 KB, patch)
2013-04-18 05:54 PDT, Suchit Agrawal
no flags Details | Formatted Diff | Diff
Patch (901.53 KB, patch)
2013-04-22 02:38 PDT, Suchit Agrawal
no flags Details | Formatted Diff | Diff
Patch (901.49 KB, patch)
2013-04-22 03:50 PDT, Suchit Agrawal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jasneet 2008-03-25 17:26:04 PDT
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
Comment 1 jasneet 2008-03-25 17:26:44 PDT
Created attachment 20062 [details]
screenshot
Comment 2 jasneet 2008-03-25 17:27:17 PDT
Created attachment 20063 [details]
reduction
Comment 3 Suchit Agrawal 2013-04-18 03:20:45 PDT
Created attachment 198708 [details]
Patch
Comment 4 Suchit Agrawal 2013-04-18 03:39:47 PDT
(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.
Comment 5 Suchit Agrawal 2013-04-18 05:54:53 PDT
Created attachment 198726 [details]
Patch
Comment 6 Suchit Agrawal 2013-04-18 05:56:32 PDT
(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.
Comment 7 Suchit Agrawal 2013-04-18 23:55:39 PDT
Beth, Please let me know your feedback about the patch. Thank you
Comment 8 Beth Dakin 2013-04-19 16:16:57 PDT
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.
Comment 9 Suchit Agrawal 2013-04-22 02:38:40 PDT
Created attachment 199010 [details]
Patch
Comment 10 Suchit Agrawal 2013-04-22 02:44:59 PDT
(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.
Comment 11 Suchit Agrawal 2013-04-22 03:50:58 PDT
Created attachment 199013 [details]
Patch
Comment 12 Suchit Agrawal 2013-04-22 03:55:31 PDT
(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.
Comment 13 Suchit Agrawal 2013-04-22 16:30:06 PDT
Beth, please review this patch and let me know your comments for the patch. Thanks
Comment 14 Beth Dakin 2013-04-22 16:33:11 PDT
Comment on attachment 199013 [details]
Patch

The new explanation and comments are so much easier to follow! Thanks for re-working them.
Comment 15 Suchit Agrawal 2013-04-22 23:04:11 PDT
(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.
Comment 16 WebKit Commit Bot 2013-04-22 23:35:41 PDT
Comment on attachment 199013 [details]
Patch

Clearing flags on attachment: 199013

Committed r148944: <http://trac.webkit.org/changeset/148944>
Comment 17 WebKit Commit Bot 2013-04-22 23:35:46 PDT
All reviewed patches have been landed.  Closing bug.