New issue
Advanced search Search tips

Issue 760709 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Incorrect rendering of table row background when row is hidden but cell is visible

Reported by bzbar...@mit.edu, Aug 30 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0

Steps to reproduce the problem:
1. Load testcase

What is the expected behavior?
Green rectangle.

What went wrong?
Red rectangle.

Did this work before? N/A 

Does this work in other browsers? Yes

Chrome version: 62.0.3198.0 (Official Build) dev (64-bit)  Channel: n/a
OS Version: OS X 10.12
Flash Version: Shockwave Flash 26.0 r0

Safari gets this right.  Per CSS spec, the visibility of the _cell_ should be the only thing affecting whether backgrounds for the cell get painted.
 
baz.html
282 bytes View Download
Labels: Needs-Triage-M62
Cc: pnangunoori@chromium.org
Owner: wangxianzhu@chromium.org
Status: Assigned (was: Unconfirmed)
Tested on latest Chrome Stable #60.0.3112.113 and Canary #62.0.3200.0 and able to reproduce the issue on Windows 7, Mac 10.12.6 and Ubuntu 14.04 

Using the per-revision bisect providing the bisect results,
Good build: 59.0.3062.0 (461587)
Bad build: 59.0.3063.0 (461928)

You are probably looking for a change made after 461906 (known good), but no later than 461907 (first known bad).

CHANGELOG URL:
The script might not always return single CL as suspect as some perf builds might get missing due to failure.
https://chromium.googlesource.com/chromium/src/+log/b226550dbd1834c2f7addec9d1df7d57498fb7fc..62ad1c5bc2e538f2a57c2e16e06327414a6fbf7a

From the CL above, assigning the issue to the concern owner

@wangxianzhu: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-URL: https://codereview.chromium.org/2786463004
Labels: hasbisect-per-revision OS-Linux OS-Windows
bzbarsky@mit.edu, thanks for your report. Can you point me the spec about table cell background visibility? Thanks.

I read an open issue in the spec:

 ISSUE 12 : Should we hide the row-group background by saying cells only draw the backgrounds of visibility:visible grouping elements?

It looks to me that whether the background of a table row with visibility:hidden should be visible behind a visible cell is not defined yet.

Comment 7 by bzbar...@mit.edu, Aug 31 2017

While true, WebKit and Edge both show green in my testcase.  Chrome used to until recently.  Firefox does not at the moment, but I just landed a change in https://bugzilla.mozilla.org/show_bug.cgi?id=1395312 to align it with WebKit and Edge.

Comment 8 by bzbar...@mit.edu, Aug 31 2017

Further, that issue shows up in the issues index but not in the main spec body.  And I don't see any corresponding issues in the github repo, so I'm not sure whether it's even still open.

All that said, if we take the "not ready for implementation" bit at its word, the relevant spec is https://www.w3.org/TR/CSS21/tables.html#model which doesn't say anything about not painting things if the visibility of the container is hidden.
I will revert the behavior.

However, The spec should still make it clear. I think we should consider that "the table spec doesn't saying anything about table row's visibility:hidden" to be that "we should use its original definition in the css spec, i.e. not to paint the element".

Comment 10 by bzbar...@mit.edu, Aug 31 2017

The element is not being painted.  The cell is being painted.  But the cell's background pulls in layers from various sources including that element...

I agree that the tables spec needs a heck of a lot of clarification, of course.  Please do file spec issues as needed.
Components: -Blink>Layout Blink>Paint
https://chromium-review.googlesource.com/c/chromium/src/+/653608 will fix this bug.
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 8 2017

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

commit b30f7c7ec99855d695a06c2c55651c8f03e941e4
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Fri Sep 08 21:30:14 2017

Fix visibility handling of table col, row group, row

The visibility:hidden rule of table col, row group and row is different
from other elements. For example, column's visibility:hidden doesn't
apply; row's visibility:hidden shouldn't hide row's background painted
behind visible cells, etc. See the bug for details.

Let their LocalVisualRect() ignore visibility to ensure the visual rect
covers anything they paint or affect.

Bug:  760709 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I907123e63ec6b4a0ae1f23f35aaffad020eebbd8
Reviewed-on: https://chromium-review.googlesource.com/653608
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Walter Korman <wkorman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500687}
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/table/invisible-col-visible-td-expected.txt
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td-expected.html
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td-expected.txt
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td.html
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td-expected.html
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td-expected.txt
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td.html
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td-expected.html
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td-expected.txt
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td.html
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutInline.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutInline.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutTableCol.h
[add] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutTableColTest.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutText.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/LayoutView.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/svg/LayoutSVGGradientStop.h
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/b30f7c7ec99855d695a06c2c55651c8f03e941e4/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h

Status: Fixed (was: Assigned)
Labels: Merge-Request-62
Status: Assigned (was: Fixed)
Besides backgrounds, there was also issue about collapsed borders. Good to have the fix in M-62.
Project Member

Comment 15 by sheriffbot@chromium.org, Sep 9 2017

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

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: TE-Verified-M63 TE-Verified-63.0.3212.0
Verified this issue on chrome Windows-10, Ubuntu 14.04 and Mac OS 10.12.6 using chrome latest canary #63.0.3212.0 by following steps mentioned in the original comment, observed the page loads in green rectangle as expected. Hence adding TE-Verified label for M-63.

Thanks!
760709.png
3.3 KB View Download

Comment 17 by bzbar...@mit.edu, Sep 11 2017

Thank you for fixing this!
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 11 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7dae09a0b16b3bd94013284f3303d7f0e11145d

commit a7dae09a0b16b3bd94013284f3303d7f0e11145d
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Mon Sep 11 16:38:48 2017

Fix visibility handling of table col, row group, row

The visibility:hidden rule of table col, row group and row is different
from other elements. For example, column's visibility:hidden doesn't
apply; row's visibility:hidden shouldn't hide row's background painted
behind visible cells, etc. See the bug for details.

Let their LocalVisualRect() ignore visibility to ensure the visual rect
covers anything they paint or affect.

TBR=wangxianzhu@chromium.org

(cherry picked from commit b30f7c7ec99855d695a06c2c55651c8f03e941e4)

Bug:  760709 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I907123e63ec6b4a0ae1f23f35aaffad020eebbd8
Reviewed-on: https://chromium-review.googlesource.com/653608
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Walter Korman <wkorman@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500687}
Reviewed-on: https://chromium-review.googlesource.com/660841
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#126}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/flag-specific/enable-slimming-paint-v2/paint/invalidation/table/invisible-col-visible-td-expected.txt
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td-expected.html
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td-expected.txt
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-col-visible-td.html
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td-expected.html
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td-expected.txt
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tbody-visible-td.html
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td-expected.html
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td-expected.txt
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/LayoutTests/paint/invalidation/table/invisible-tr-visible-td.html
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutBox.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutBox.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutInline.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutInline.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutMultiColumnSet.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutObject.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutObjectTest.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutTable.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutTableCol.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutTableCol.h
[add] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutTableColTest.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutText.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutText.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutView.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/LayoutView.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/svg/LayoutSVGGradientStop.h
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
[modify] https://crrev.com/a7dae09a0b16b3bd94013284f3303d7f0e11145d/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.h

Status: Fixed (was: Assigned)

Sign in to add a comment