Bug 170768

Summary: getBoundingClientRect() returns wrong value for tr, td and its descendants for a vertical table
Product: WebKit Reporter: Yuki Sekiguchi <yuki.sekiguchi>
Component: Layout and RenderingAssignee: Yuki Sekiguchi <yuki.sekiguchi>
Status: RESOLVED FIXED    
Severity: Normal CC: ahmad.saleem792, bfulgham, buildbot, changseok, darin, esprehn+autocc, ews-watchlist, glenn, hyatt, koivisto, kondapallykalyan, mjs, pdr, simon.fraser, skoji, webkit-bug-importer, ysuzuki, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Reproduced content
none
Patch
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch yuki.sekiguchi: review?, yuki.sekiguchi: commit-queue?

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?
Comment 12 Ahmad Saleem 2022-09-21 13:37:40 PDT
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!
Comment 13 EWS 2022-09-27 09:27:05 PDT
Committed 254918@main (1e62e8aa2236): <https://commits.webkit.org/254918@main>

Reviewed commits have been landed. Closing PR #4583 and removing active labels.
Comment 14 Radar WebKit Bug Importer 2022-09-27 09:28:25 PDT
<rdar://problem/100462489>