Bug 170768 - getBoundingClientRect() returns wrong value for tr, td and its descendants for a vertical table
Summary: getBoundingClientRect() returns wrong value for tr, td and its descendants fo...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yuki Sekiguchi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-12 02:50 PDT by Yuki Sekiguchi
Modified: 2020-06-08 12:21 PDT (History)
15 users (show)

See Also:


Attachments
Reproduced content (3.34 KB, text/html)
2017-04-12 02:50 PDT, Yuki Sekiguchi
no flags Details
Patch (72.99 KB, patch)
2017-04-12 04:18 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (13.05 MB, application/zip)
2017-04-12 05:56 PDT, Build Bot
no flags Details
Patch (108.25 KB, patch)
2017-04-12 18:55 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (107.33 KB, patch)
2020-06-01 20:44 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (123.32 KB, patch)
2020-06-02 03:52 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (123.46 KB, patch)
2020-06-02 05:58 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (221.80 KB, patch)
2020-06-02 19:39 PDT, Yuki Sekiguchi
yuki.sekiguchi: review?
yuki.sekiguchi: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuki Sekiguchi 2017-04-12 02:50:15 PDT
Created attachment 306902 [details]
Reproduced content

getBoundingClientRect() returns wrong value for tr, td and its descendants for a vertical table

In the attached content, the rect for tds are out side of the td which is its parent.

Inspector highlights wrong rects for tr, td and its descendants as well.
Comment 1 Yuki Sekiguchi 2017-04-12 04:18:52 PDT
Created attachment 306907 [details]
Patch
Comment 2 Build Bot 2017-04-12 05:56:35 PDT
Comment on attachment 306907 [details]
Patch

Attachment 306907 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3523775

New failing tests:
fast/writing-mode/vertical-align-table-baseline.html
fast/table/040-vertical.html
fast/overflow/overflow-rtl-vertical.html
fast/table/rowspan-paint-order-vertical.html
fast/table/table-display-types-vertical.html
fast/table/border-collapsing/002-vertical.html
fast/table/growCellForImageQuirk-vertical.html
fast/table/038-vertical.html
fast/css/h1-in-section-elements.html
fast/table/border-collapsing/003-vertical.html
fast/table/border-collapsing/001-vertical.html
fast/table/028-vertical.html
fast/table/auto-with-percent-height-vertical.html
fast/table/border-collapsing/rtl-border-collapsing-vertical.html
fast/table/027-vertical.html
Comment 3 Build Bot 2017-04-12 05:56:36 PDT
Created attachment 306908 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 4 Yuki Sekiguchi 2017-04-12 18:55:57 PDT
Created attachment 306960 [details]
Patch
Comment 5 Maciej Stachowiak 2020-05-30 19:10:08 PDT
Comment on attachment 306960 [details]
Patch

Seems like a good idea to fix this bug, unfortunately, this version of the patch no longer applies. Seems like just one of the test diffs that has an issue, so shouldn't be too hard to fix up.
Comment 6 Yuki Sekiguchi 2020-06-01 20:44:16 PDT
Created attachment 400780 [details]
Patch
Comment 7 Yuki Sekiguchi 2020-06-02 03:52:20 PDT
Created attachment 400801 [details]
Patch
Comment 8 Yuki Sekiguchi 2020-06-02 05:58:36 PDT
Created attachment 400809 [details]
Patch
Comment 9 Yuki Sekiguchi 2020-06-02 19:39:43 PDT
Created attachment 400885 [details]
Patch
Comment 10 Yuki Sekiguchi 2020-06-04 01:53:32 PDT
Hi Maciej,

I rebased my patch onto the latest WebKit and fixed LayoutTest errors.

Could you review it again?
Comment 11 Darin Adler 2020-06-08 12:21:46 PDT
Comment on attachment 400885 [details]
Patch

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

This looks good to me. Not sure I’m sufficiently expert to be the reviewer though.

> Source/WebCore/rendering/RenderTableCell.cpp:347
> +    if (parent()) {
> +        RenderElement* containerOfRow = container.container();
> +        offset -= parent()->offsetFromContainer(*containerOfRow, point);
> +    }

What guarantees that container.container() is non-null?

Tiny point: I don’t think the local variable is doing a lot that is helpful here. I think that writing *container.container() is probably OK without explicitly documenting that it’s the container of the row. After all, we are not documenting what parent is (the section, maybe?).

> Source/WebCore/rendering/RenderTableSection.cpp:562
> +            rowRenderer->setLogicalLeft(0_lu);
> +            rowRenderer->setLogicalTop(m_rowPos[r]);
>              rowRenderer->setLogicalWidth(logicalWidth());
>              rowRenderer->setLogicalHeight(m_rowPos[r + 1] - m_rowPos[r] - vspacing);

Is there a setLogical function that takes a LayoutRect? If so, maybe we’d want to use it here?