Issue 1130843004: [CSS Grid Layout] New alignment related repaint layout tests.

Issue 1130843004: [CSS Grid Layout] New alignment related repaint layout tests. (Closed)

Created:
5 years, 7 months ago by jfernandez

Modified:
5 years, 7 months ago

Reviewers:
dsinclair, esprehn, Julien - ping for review

CC:
blink-reviews

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

[CSS Grid Layout] New alignment related repaint layout tests. This patch adds new repaint tests for checking style changes on the new Box Alignment properties recently added support for in Grid Layout. There were also changes on the existing tests to better describing the style change itself and the expected result when success. BUG=249451, 376823 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195662

Patch Set 1 #

Total comments: 12

Patch Set 2 : Patch rebased and applied suggested changes. #

Patch Set 3 : Anohter test expected to fail added to TestExpectations. #

Created: 5 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+362 lines, -247 lines)			Patch
M	LayoutTests/TestExpectations	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/repaint/align-content-distribution-change-grid.html	View		2 chunks	+8 lines, -5 lines	0 comments	Download
A	LayoutTests/fast/repaint/align-content-distribution-change-grid-expected.txt	View		1 chunk	+26 lines, -0 lines	0 comments	Download
A +	LayoutTests/fast/repaint/align-content-position-change-grid.html	View	1	3 chunks	+11 lines, -8 lines	0 comments	Download
A	LayoutTests/fast/repaint/align-content-position-change-grid-expected.txt	View	1	1 chunk	+26 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/repaint/align-items-overflow-change.html	View		2 chunks	+11 lines, -14 lines	0 comments	Download
M	LayoutTests/fast/repaint/align-items-overflow-change-expected.txt	View		1 chunk	+10 lines, -10 lines	0 comments	Download
A +	LayoutTests/fast/repaint/align-self-change-grid.html	View		3 chunks	+9 lines, -12 lines	0 comments	Download
A +	LayoutTests/fast/repaint/align-self-change-grid-expected.txt	View		1 chunk	+6 lines, -1 line	0 comments	Download
A +	LayoutTests/fast/repaint/align-self-change-keeping-geometry-grid.html	View		1 chunk	+13 lines, -10 lines	0 comments	Download
A +	LayoutTests/fast/repaint/align-self-change-keeping-geometry-grid-expected.txt	View	1	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	LayoutTests/fast/repaint/align-self-overflow-change.html	View		3 chunks	+4 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/repaint/align-self-overflow-change-expected.txt	View		1 chunk	+7 lines, -10 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-content-change.html	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
A +	LayoutTests/fast/repaint/justify-content-distribution-change-grid.html	View		2 chunks	+13 lines, -9 lines	0 comments	Download
A	LayoutTests/fast/repaint/justify-content-distribution-change-grid-expected.txt	View		1 chunk	+20 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-content-position-change.html	View	1	2 chunks	+2 lines, -2 lines	0 comments	Download
A	LayoutTests/fast/repaint/justify-content-position-change-grid.html	View		1 chunk	+38 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/repaint/justify-content-position-change-grid-expected.txt	View		1 chunk	+20 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-items-change.html	View		1 chunk	+4 lines, -14 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-items-change-expected.txt	View		1 chunk	+4 lines, -8 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-items-legacy-change.html	View		1 chunk	+1 line, -1 line	0 comments	Download
M	LayoutTests/fast/repaint/justify-items-overflow-change.html	View	1	2 chunks	+1 line, -3 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-items-overflow-change-expected.txt	View	1	1 chunk	+11 lines, -11 lines	0 comments	Download
A +	LayoutTests/fast/repaint/justify-self-change-keeping-geometry.html	View		3 chunks	+4 lines, -5 lines	0 comments	Download
A +	LayoutTests/fast/repaint/justify-self-change-keeping-geometry-expected.txt	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-self-overflow-change.html	View		3 chunks	+4 lines, -4 lines	0 comments	Download
M	LayoutTests/fast/repaint/justify-self-overflow-change-expected.txt	View		1 chunk	+8 lines, -7 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/align-content-distribution-change-grid-expected.txt	View		1 chunk	+13 lines, -13 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/align-content-position-change-grid-expected.txt	View	1	1 chunk	+13 lines, -13 lines	0 comments	Download
M	LayoutTests/virtual/slimmingpaint/fast/repaint/align-items-overflow-change-expected.txt	View		1 chunk	+10 lines, -10 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/align-self-change-grid-expected.txt	View		1 chunk	+7 lines, -10 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/align-self-change-keeping-geometry-grid-expected.txt	View	1	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	LayoutTests/virtual/slimmingpaint/fast/repaint/align-self-overflow-change-expected.txt	View		1 chunk	+8 lines, -12 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-content-distribution-change-grid-expected.txt	View		1 chunk	+10 lines, -8 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-content-position-change-grid-expected.txt	View		1 chunk	+10 lines, -8 lines	0 comments	Download
M	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-items-change-expected.txt	View		1 chunk	+6 lines, -14 lines	0 comments	Download
M	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-items-overflow-change-expected.txt	View	1	1 chunk	+11 lines, -11 lines	0 comments	Download
A +	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-self-change-keeping-geometry-expected.txt	View	1 2	0 chunks	+-1 lines, --1 lines	0 comments	Download
M	LayoutTests/virtual/slimmingpaint/fast/repaint/justify-self-overflow-change-expected.txt	View		1 chunk	+10 lines, -8 lines	0 comments	Download

Messages

Total messages: 14 (7 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Julien - ping for review

lgtm with some clarifications / modifications. https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/align-content-position-change-grid.html File LayoutTests/fast/repaint/align-content-position-change-grid.html (left): https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/align-content-position-change-grid.html#oldcode1 LayoutTests/fast/repaint/align-content-position-change-grid.html:1: <!DOCTYPE HTML> Doctype ...

5 years, 7 months ago (2015-05-19 17:59:18 UTC) #4

jfernandez

Thanks for the review. Applied suggested comments, so trying the CQ now. https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/align-content-position-change-grid.html File LayoutTests/fast/repaint/align-content-position-change-grid.html ...

5 years, 7 months ago (2015-05-20 20:11:34 UTC) #5

Thanks for the review.
Applied suggested comments, so trying the CQ now.

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
File LayoutTests/fast/repaint/align-content-position-change-grid.html (left):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
LayoutTests/fast/repaint/align-content-position-change-grid.html:1: <!DOCTYPE
HTML>
On 2015/05/19 17:59:17, Julien Chaffraix - PST wrote:
> Doctype removed?

Mistake, got it back.

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
File LayoutTests/fast/repaint/align-content-position-change-grid.html (right):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
LayoutTests/fast/repaint/align-content-position-change-grid.html:26: <p
style="height: 20px">Tests invalidation on align-content style change
(position). Passes if the red half is on the top.</p>
On 2015/05/19 17:59:17, Julien Chaffraix - PST wrote:
> Red should mark failure, we should change the condition for passing to remove
> seeing some 'red' (by changing the color of the item if needed).

Done.

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
File LayoutTests/fast/repaint/align-self-change-keeping-geometry-grid.html
(right):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/al...
LayoutTests/fast/repaint/align-self-change-keeping-geometry-grid.html:27: <p
style="height: 20px">There should be no invalidations because align-self style
change causes no visual change.</p>
On 2015/05/19 17:59:17, Julien Chaffraix - PST wrote:
> Usually no invalidation is better done like
> fast/repaint/fixed-position-layer-moved.html (checking if there is any repaint
> rectangle in the output and dumped FAIL in this case).

I tried with that and there is indeed one invalidation rectangle. It s caused by
the bug 474798, which it does not affect only align-items and justify-items
like it states on its description, but align-self and justify-self properties as
well.

> 
> There is one invalidation which means either that we are failing the test (and
I
> prefer an explicit FAIL in this case) or that this description is inaccurate.

I've finally added an entry in the TestExpectations file for this testes,
pointing to
the mentioned bug, so it will fail while we don't fix it (I'm already working on
it).

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
File LayoutTests/fast/repaint/justify-items-overflow-change.html (right):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
LayoutTests/fast/repaint/justify-items-overflow-change.html:15: grid: 100px
100px / 300px;
On 2015/05/19 17:59:17, Julien Chaffraix - PST wrote:
> Does this change really matter and make the test more readable?

Yes, it does matter because it's what makes possible using the 'there 
is no red' condition for success.

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
File LayoutTests/fast/repaint/justify-self-overflow-change.html (left):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
LayoutTests/fast/repaint/justify-self-overflow-change.html:25: border: solid
thin blue;
On 2015/05/19 17:59:18, Julien Chaffraix - PST wrote:
> Removing the borders is great as it aligns the size of the grid element with
the
> grid area.

Acknowledged.

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
File LayoutTests/fast/repaint/justify-self-overflow-change.html (right):

https://codereview.chromium.org/1130843004/diff/1/LayoutTests/fast/repaint/ju...
LayoutTests/fast/repaint/justify-self-overflow-change.html:16: grid: 100px 100px
/ 300px;
On 2015/05/19 17:59:18, Julien Chaffraix - PST wrote:
> Ditto.

It does matter actually, as it was commented before.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130843004/20001

5 years, 7 months ago (2015-05-20 20:12:18 UTC) #8

commit-bot: I haz the power

Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/62424)

5 years, 7 months ago (2015-05-20 20:58:45 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1130843004/40001

5 years, 7 months ago (2015-05-20 21:22:12 UTC) #13

commit-bot: I haz the power

5 years, 7 months ago (2015-05-21 02:14:32 UTC) #14

Message was sent while issue was closed.

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195662

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages