New issue
Advanced search Search tips

Issue 719106 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Wrong ContentsVisualOverflowRect of table row containing cells with rowspan

Project Member Reported by wangxianzhu@chromium.org, May 6 2017

Issue description

The related code was changed in
https://codereview.chromium.org/2786463004/diff/120001/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp.

Both the old code and the new code are incorrect.
 
Labels: -OS-iOS
Project Member

Comment 2 by bugdroid1@chromium.org, May 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb

commit 0391059128167dc8dc8fdbcbc7d3e3b81b7edefb
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue May 09 03:49:28 2017

Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG= 719106 , 603993 , 646015 
R=wkorman@chromium.org

Review-Url: https://codereview.chromium.org/2861653006 .
Cr-Commit-Position: refs/heads/master@{#470166}

[modify] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table-expected.html
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table.html
[add] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell-expected.html
[add] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell-expected.txt
[add] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell.html
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/platform/linux/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/platform/mac-mac10.11/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/platform/mac-retina/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/7f0b7f49f8d74428d8aa688576307738801689ac/third_party/WebKit/LayoutTests/platform/win7/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[modify] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/0391059128167dc8dc8fdbcbc7d3e3b81b7edefb/third_party/WebKit/Source/core/layout/LayoutTableRowTest.cpp

Status: Fixed (was: Assigned)
Labels: Merge-Request-59
Project Member

Comment 5 by sheriffbot@chromium.org, May 11 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 6 by bugdroid1@chromium.org, May 11 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00be1dbf48663cdc4d867913f9420d2500c73c10

commit 00be1dbf48663cdc4d867913f9420d2500c73c10
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Thu May 11 22:37:33 2017

Fix LayoutTableRow::AddOverflowFromCell()

This fixes the following problems:
1. Incorrect offset of contents overflow from cell:
   This was changed in https://codereview.chromium.org/2786463004
   Both the old version and the new version are incorrect:
   Should adjust by the difference of locations of row and cell.
2. Missing cell overflow propagating to row when the cell doesn't
   span rows.
3. Checking of HasBackground made row's visual overflow depend
   on the existence of background. Just remove the check to avoid
   tricky logic which otherwise would be required to invalidate row's
   overflow on background existence change.
4. Cell generates not only visual overflow, but also layout overflow
   (e.g. when there is a relative-positioned child). This fix doesn't
   change behavior because row doesn't use the layout overflow.
   However, this makes it possible to optimize LayoutTableSection::
   ComputeOverflowFromCell() to just accumulate overflows from rows
   instead of all cells. The optimization will be in the next CL.

BUG= 719106 , 603993 , 646015 
R=wkorman@chromium.org
TBR=wangxianzhu@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2861653006 .
Cr-Original-Commit-Position: refs/heads/master@{#470166}
Review-Url: https://codereview.chromium.org/2880663002
Cr-Commit-Position: refs/branch-heads/3071@{#520}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table-expected.html
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/paint/invalidation/hover-invalidation-table.html
[add] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell-expected.html
[add] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell-expected.txt
[add] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/LayoutTests/paint/invalidation/table/row-change-background-rowspan-cell.html
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/platform/linux/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/platform/mac-mac10.11/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/platform/mac-retina/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[delete] https://crrev.com/37b22ee298b3cde4c44ffc5ab83449c26b0dfe59/third_party/WebKit/LayoutTests/platform/win7/virtual/disable-spinvalidation/paint/invalidation/hover-invalidation-table-expected.txt
[modify] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/Source/core/layout/LayoutTableRow.cpp
[modify] https://crrev.com/00be1dbf48663cdc4d867913f9420d2500c73c10/third_party/WebKit/Source/core/layout/LayoutTableRowTest.cpp

Sign in to add a comment