Fix sticky bottom is not applied with both sticky position

Fix sticky bottom is not applied with both sticky

The box which has a sticky position should be used instead of
scroll container block to check whether the box can be
positioned in constraningsize with sticky both sides offset.

BUG=703853
TEST=fast/css/sticky/sticky-both-sides*

Review-Url: https://codereview.chromium.org/2788053002
Cr-Commit-Position: refs/heads/master@{#462224}
Committed: https://chromium.googlesource.com/chromium/src/+/b23829632c36f42e8cbcf6a3adf8fc11aaa4b337

[wanchang, 2017-03-31 05:16:32]

wanchang.ryu@lge.com changed reviewers:
+ flackr@chromium.org

[wanchang, 2017-03-31 05:16:33]

PTAL.

[wanchang, 2017-03-31 05:25:10]

Description was changed from

==========
Fix sticky bottom is not applied with both sticky

The box which has a sticky position should be used instead of
scroll container block to check whether the box can be
positioned in constraningsize with sticky both sides offset.

BUG=703853
TEST=fast/css/sticky/sticky-both-sides*
==========

to

==========
Fix sticky bottom is not applied with both sticky

The box which has a sticky position should be used instead of
scroll container block to check whether the box can be
positioned in constraningsize with sticky both sides offset.

BUG=703853
TEST=fast/css/sticky/sticky-both-sides*
==========

[wanchang, 2017-03-31 05:25:10]

wanchang.ryu@lge.com changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2017-03-31 19:42:29]

flackr@, deferring to you for this review.

[flackr, 2017-04-03 20:45:18]

Good catch! In general this looks good, just a couple comments on the tests.

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html
(right):

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:45:
right: 50px;
Instead of calling these tests sticky-both-sides-# can we try to have more
descriptive names.

I.e.
sticky-both-sides -> sticky-both-sides-top-left-constrained
sticky-both-sides-2 -> sticky-both-sides-bottom-right-constrained

sticky-both-sides-3 and sticky-both-sides 4 seem to be replicating the
horizontal and vertical tests from sticky-both-sides, aren't they?

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:50:
left: 0;
Seems like the left shouldn't affect the vertical sticky anyways since it can't
move within its container. We could just have all of top, left, bottom, right
set to 50px for both of the sticky position elements.

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:78: <div
class="vertical space"></div>
the vertical class here (and in several of the other tests) has no effect
because neither ".vertical" or ".vertical.space" is not styled. I'd suggest
removing it.

[wanchang, 2017-04-04 00:57:49]

When I try to git cl upload. I got below error.
LayoutTests/FlagExpectations/root-layer-scrolls:266 Path does not exist.
fast/css/sticky/sticky-both-sides.html

Lint failed.

It looks I need to modify test case name on root-layer-scrolls file too, so I
modified that file and also I add my additional test case. Is it right change ?

PTAL patchset2
thanks.

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html
(right):

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:45:
right: 50px;
On 2017/04/03 20:45:17, flackr wrote:
> Instead of calling these tests sticky-both-sides-# can we try to have more
> descriptive names.
> 
> I.e.
> sticky-both-sides -> sticky-both-sides-top-left-constrained
> sticky-both-sides-2 -> sticky-both-sides-bottom-right-constrained
Done.
> 
> sticky-both-sides-3 and sticky-both-sides 4 seem to be replicating the
> horizontal and vertical tests from sticky-both-sides, aren't they?
You are right, It seems same. I removed 3,4 cases.

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:50:
left: 0;
On 2017/04/03 20:45:17, flackr wrote:
> Seems like the left shouldn't affect the vertical sticky anyways since it
can't
> move within its container. We could just have all of top, left, bottom, right
> set to 50px for both of the sticky position elements.

Done.

https://codereview.chromium.org/2788053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-2.html:78: <div
class="vertical space"></div>
On 2017/04/03 20:45:17, flackr wrote:
> the vertical class here (and in several of the other tests) has no effect
> because neither ".vertical" or ".vertical.space" is not styled. I'd suggest
> removing it.

You are right. It was not what I wanted. I changed css rules.

[flackr, 2017-04-04 02:31:58]

> It looks I need to modify test case name on root-layer-scrolls file too, so I
> modified that file and also I add my additional test case. Is it right change
?

Yes, you need to modify the test case name, and it seems likely that
root-layer-scrolls won't support your new test if it fails on the existing test.

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html
(right):

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:25:
.vertical_container {
Use dashes for class names:
https://www.chromium.org/developers/web-development-style-guide

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:42:
}
The only elements with the .box class are sticky, let's move these styles to the
sticky class.

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:67:
opacity: 0.1;
If I may make a suggestion, rather than repeating all of the styles, use a
common base class with a horizontal or vertical modifier.

i.e.
.space {
  background-color: grey;
  opacity: 0.1;
}
.horizontal .space {
  width: 2000px;
  height: 100%;
}
.vertical .space {
  height: 2000px;
  width: 100%;
}

<div class="horizontal container">
  <div class="space"></div>
</div>
<div class="vertical container">
  <div class="space"></div>
</div>

Same for .sticky and .container.

[wanchang, 2017-04-04 08:19:11]

PATL.

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html
(right):

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:25:
.vertical_container {
On 2017/04/04 02:31:58, flackr wrote:
> Use dashes for class names:
> https://www.chromium.org/developers/web-development-style-guide

Done.

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:42:
}
On 2017/04/04 02:31:58, flackr wrote:
> The only elements with the .box class are sticky, let's move these styles to
the
> sticky class.

Done.

https://codereview.chromium.org/2788053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained.html:67:
opacity: 0.1;
On 2017/04/04 02:31:58, flackr wrote:
> If I may make a suggestion, rather than repeating all of the styles, use a
> common base class with a horizontal or vertical modifier.
> 
> i.e.
> .space {
>   background-color: grey;
>   opacity: 0.1;
> }
> .horizontal .space {
>   width: 2000px;
>   height: 100%;
> }
> .vertical .space {
>   height: 2000px;
>   width: 100%;
> }
> 
> <div class="horizontal container">
>   <div class="space"></div>
> </div>
> <div class="vertical container">
>   <div class="space"></div>
> </div>
> 
> Same for .sticky and .container.

Done.

[flackr, 2017-04-04 13:58:20]

Looks good, just one last comment.

https://codereview.chromium.org/2788053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained-expected.html
(right):

https://codereview.chromium.org/2788053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained-expected.html:31:
position: sticky;
Sorry, one last comment. For the expectation if you use position: fixed it
should also be in the right position. This is better since we're not relying on
sticky position to test sticky position.

[wanchang, 2017-04-05 05:11:03]

PTAL

https://codereview.chromium.org/2788053002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained-expected.html
(right):

https://codereview.chromium.org/2788053002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/css/sticky/sticky-both-sides-bottom-right-constrained-expected.html:31:
position: sticky;
On 2017/04/04 13:58:20, flackr wrote:
> Sorry, one last comment. For the expectation if you use position: fixed it
> should also be in the right position. This is better since we're not relying
on
> sticky position to test sticky position.

I agree with you. Done

[flackr, 2017-04-05 11:25:48]

LGTM thanks for the fix!

[wanchang, 2017-04-05 12:23:38]

The CQ bit was checked by wanchang.ryu@lge.com

[commit-bot: I haz the power, 2017-04-05 12:23:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wanchang, 2017-04-05 12:24:59]

On 2017/04/05 11:25:48, flackr wrote:
> LGTM thanks for the fix!

Thanks for your valuable review.

[commit-bot: I haz the power, 2017-04-05 12:31:32]

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

[commit-bot: I haz the power, 2017-04-05 12:31:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wanchang, 2017-04-05 13:13:59]

chrishtr, could you review this commit ? To land this patch it also needs your
review.

[chrishtr, 2017-04-05 16:03:14]

lgtm

[wanchang, 2017-04-05 21:19:43]

The CQ bit was checked by wanchang.ryu@lge.com

[commit-bot: I haz the power, 2017-04-05 21:20:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 21:31:47]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491427183713160,
"parent_rev": "9f7541c77a83aa2a946d3227dbbbcc5eb1983ca6", "commit_rev":
"b23829632c36f42e8cbcf6a3adf8fc11aaa4b337"}

[commit-bot: I haz the power, 2017-04-05 21:32:39]

Description was changed from

==========
Fix sticky bottom is not applied with both sticky

The box which has a sticky position should be used instead of
scroll container block to check whether the box can be
positioned in constraningsize with sticky both sides offset.

BUG=703853
TEST=fast/css/sticky/sticky-both-sides*
==========

to

==========
Fix sticky bottom is not applied with both sticky

The box which has a sticky position should be used instead of
scroll container block to check whether the box can be
positioned in constraningsize with sticky both sides offset.

BUG=703853
TEST=fast/css/sticky/sticky-both-sides*

Review-Url: https://codereview.chromium.org/2788053002
Cr-Commit-Position: refs/heads/master@{#462224}
Committed:
https://chromium.googlesource.com/chromium/src/+/b23829632c36f42e8cbcf6a3adf8...
==========

[commit-bot: I haz the power, 2017-04-05 21:32:43]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b23829632c36f42e8cbcf6a3adf8...