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

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yuki Sekiguchi]

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.

[Yuki Sekiguchi]

Created attachment 306907 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Yuki Sekiguchi]

Created attachment 306960 [details]
Patch

[Maciej Stachowiak]

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.

[Yuki Sekiguchi]

Created attachment 400780 [details]
Patch

[Yuki Sekiguchi]

Created attachment 400801 [details]
Patch

[Yuki Sekiguchi]

Created attachment 400809 [details]
Patch

[Yuki Sekiguchi]

Created attachment 400885 [details]
Patch

[Yuki Sekiguchi]

Hi Maciej,

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

Could you review it again?

[Darin Adler]

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?

[Ahmad Saleem]

I am still able to reproduce this bug in Safari 16 and Safari Technology Preview 154 using attached "Reproduced Content" while both Chrome Canary 108 and Firefox Nightly 107 show "success' in the test case. Thanks!

[EWS]

Committed 254918@main (1e62e8aa2236): <https://commits.webkit.org/254918@main>

Reviewed commits have been landed. Closing PR #4583 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/100462489>