Implement box-shadow for table section and row

Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871
Committed: https://crrev.com/b368f26a7a45be3d5ba7906fca5ae79753b14af3
Cr-Commit-Position: refs/heads/master@{#397041}

[Xianzhu, 2016-05-11 16:44:04]

Description was changed from

==========
Implement box-shadow for table section and row

BUG=94871
==========

to

==========
Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871
==========

[Xianzhu, 2016-05-11 16:44:25]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, wkorman@chromium.org

[Xianzhu, 2016-05-11 18:35:33]

No rush for this CL. As it is a new feature, I expect to land it after m52
branch.

[chrishtr, 2016-05-14 08:56:14]

Some of the code looks like pure refactoring, could you please send as separate
CL for that?
e.g. paintContainerBackgroundBehindCell.

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/BoxPainter.cpp (left):

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/BoxPainter.cpp:56: // Use the visual
overflow rect here, because it will include overflow introduced by the theme.
This comment is incorrect somehow?

[chrishtr, 2016-05-14 18:03:47]

A bunch of this CL does indeed look like pure refactoring. Please do
that in a new CL and then come back to this.

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableCellPainter.cpp (right):

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableCellPainter.cpp:185:
paintBackground(paintInfo, paintRect, m_layoutTableCell);
Why not paintContainerBackgroundBehindCell? The old code
called paintBackgroundsBehindCell.

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableRowPainter.h (right):

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableRowPainter.h:22: void
paintBackgroundBehindCell(const LayoutTableCell&, const PaintInfo&, const
LayoutPoint&);
Doesn't a cell know its row? Why is this method not on
TableCellPainter?

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (left):

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:181: if
(!BlockPainter(cell).intersectsPaintRect(paintInfoForCells, paintOffset +
cell.location()))
Why is this code gone?

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/TableSectionPainter.h (right):

https://codereview.chromium.org/1963313002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/TableSectionPainter.h:36: const
LayoutTableCell* primaryCellToPaint(unsigned row, unsigned column, const
CellSpan& dirtiedRows, const CellSpan& dirtiedColumns) const;
This needs a comment.

[Xianzhu, 2016-05-20 22:11:45]

The depended CL (https://codereview.chromium.org/1983853003/) has landed. This
CL now contains new code only.
Ptal.

[chrishtr, 2016-05-20 22:53:31]

Do all these test results agree with Firefox and Edge? Are there any w3c
compatibility tests
for table painting?

[Xianzhu, 2016-05-20 23:40:19]

On 2016/05/20 22:53:31, chrishtr wrote:
> Do all these test results agree with Firefox and Edge?

They don't match FireFox exactly. The difference is because of crbug.com/613753
which already caused difference about outlines, now about box-shadows.

Edge behaves neither like Firefox nor like Chrome with this CL. It just applies
a row's box-shadow on each contained cell. It doesn't support box-shadow on
table sections.

> Are there any w3c compatibility tests for table painting?

Didn't find any test containing 'box-shadow' under LayoutTests/imported. Does
the directory contain all w3c compatibility tests?

[chrishtr, 2016-05-20 23:49:43]

On 2016/05/20 at 23:40:19, wangxianzhu wrote:
> On 2016/05/20 22:53:31, chrishtr wrote:
> > Do all these test results agree with Firefox and Edge?
> 
> They don't match FireFox exactly. The difference is because of
crbug.com/613753 which already caused difference about outlines, now about
box-shadows.
> 
> Edge behaves neither like Firefox nor like Chrome with this CL. It just
applies a row's box-shadow on each contained cell. It doesn't support box-shadow
on table sections.
> 
> > Are there any w3c compatibility tests for table painting?
> 
> Didn't find any test containing 'box-shadow' under LayoutTests/imported. Does
the directory contain all w3c compatibility tests?


Not sure. Christian Biesinger or Dominic Denicola might know. I suspect here are
none, but if so we should consider adding them in the future and queue up a task
for us to do that at some point.

I also googled around a bit and found the github repo for the w3c tests, which
didn't seem to have any CSS tests at all, so maybe that's the wrong place.
(https://github.com/w3c/web-platform-tests)

[chrishtr, 2016-05-31 16:49:36]

Any update on this CL?

[Xianzhu, 2016-05-31 17:06:30]

wangxianzhu@chromium.org changed reviewers:
+ domenic@chromium.org

[Xianzhu, 2016-05-31 17:06:31]

On 2016/05/31 16:49:36, chrishtr wrote:
> Any update on this CL?

The latest patch set is still the candidate for review.

+domenic@ for any advice about w3c tests.

[domenic, 2016-05-31 17:48:39]

On 2016/05/31 at 17:06:31, wangxianzhu wrote:
> +domenic@ for any advice about w3c tests.

CSS tests are at https://github.com/w3c/csswg-test.
https://github.com/w3c/csswg-test/tree/master/css-backgrounds-3 has a number of
box-shadow tests (most of which are in the root directory, not inside the
box-shadow directory). I'm not sure about whether/how we import CSS tests;
+cbiesinger@ would know.

[chrishtr, 2016-05-31 17:58:41]

lgtm

Xianzhu: I'll put a note for the team to discuss CSSWG tests at the next team
meeting.
Meantime, unblocking this CL.

[Xianzhu, 2016-05-31 20:29:34]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-05-31 20:29:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1963313002/#ps160001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-31 20:30:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963313002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963313002/160001

[commit-bot: I haz the power, 2016-05-31 20:41:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 20:41:49]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Xianzhu, 2016-05-31 21:39:42]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-05-31 21:40:35]

The CQ bit was unchecked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-05-31 21:40:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963313002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963313002/160001

[Xianzhu, 2016-05-31 21:40:50]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-05-31 21:40:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1963313002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2016-05-31 21:41:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963313002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963313002/180001

[commit-bot: I haz the power, 2016-05-31 23:39:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 23:39:22]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2016-06-01 00:15:50]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-01 00:15:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1963313002/#ps200001
(title: " ")

[commit-bot: I haz the power, 2016-06-01 00:16:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1963313002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1963313002/200001

[commit-bot: I haz the power, 2016-06-01 03:28:35]

Description was changed from

==========
Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871
==========

to

==========
Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871
==========

[commit-bot: I haz the power, 2016-06-01 03:28:36]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-06-01 03:31:33]

Description was changed from

==========
Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871
==========

to

==========
Implement box-shadow for table section and row

A table section's normal box-shadow is painted before backgrounds of
colgroup, col and section are painted behind cells, then inset box-shadow.

A table row's normal box-shadow is painted before its background is
painted behind cells, then inset box-shadow.

BUG=94871

Committed: https://crrev.com/b368f26a7a45be3d5ba7906fca5ae79753b14af3
Cr-Commit-Position: refs/heads/master@{#397041}
==========

[commit-bot: I haz the power, 2016-06-01 03:31:34]

Patchset 11 (id:??) landed as
https://crrev.com/b368f26a7a45be3d5ba7906fca5ae79753b14af3
Cr-Commit-Position: refs/heads/master@{#397041}

[cbiesinger, 2016-06-06 22:40:41]

On 2016/05/31 17:48:39, domenic wrote:
> On 2016/05/31 at 17:06:31, wangxianzhu wrote:
> > +domenic@ for any advice about w3c tests.
> 
> CSS tests are at https://github.com/w3c/csswg-test.
> https://github.com/w3c/csswg-test/tree/master/css-backgrounds-3 has a number
of
> box-shadow tests (most of which are in the root directory, not inside the
> box-shadow directory). I'm not sure about whether/how we import CSS tests;
> +cbiesinger@ would know.

I'm a little late here, but see
https://www.chromium.org/blink/importing-the-w3c-tests for importing tests. We
don't currently import css-backgrounds-3 but that would be pretty easy to do (we
do import other CSS tests such as flexbox et al)