Don't promote position: fixed elements with composited descendants if they don't scroll

Don't promote position: fixed elements with composited descendants if they don't scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2839263002
Cr-Commit-Position: refs/heads/master@{#469584}
Committed: https://chromium.googlesource.com/chromium/src/+/9b503962a017a8d69adfdae9300684c299922640

[smcgruer, 2017-04-26 20:11:27]

Description was changed from

==========
Do not promote position: fixed elements unless they move with scroll

BUG=715699
==========

to

==========
Do not promote position: fixed elements unless they move with scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-04-26 20:11:49]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 20:12:42]

Dry run: 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-26 21:26:04]

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

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

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

[smcgruer, 2017-04-26 22:31:06]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 22:32:47]

Dry run: 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-26 23:36:43]

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

[commit-bot: I haz the power, 2017-04-26 23:36:46]

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

[smcgruer, 2017-04-27 13:21:34]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 13:21:55]

Dry run: CQ is trying da patch.

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

[smcgruer, 2017-04-27 13:24:38]

The CQ bit was unchecked by smcgruer@chromium.org

[smcgruer, 2017-04-27 13:25:40]

The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 13:25:58]

Dry run: CQ is trying da patch.

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

[smcgruer, 2017-04-27 14:26:45]

smcgruer@chromium.org changed reviewers:
+ chrishtr@chromium.org, flackr@chromium.org

[smcgruer, 2017-04-27 14:26:46]

This is the fixed-position only part of
https://codereview.chromium.org/2754983002/

[flackr, 2017-04-27 14:48:47]

LGTM, just a couple comments.

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:194:
const bool ignore_lcd_text = true;
nit: Couldn't hurt to add a comment that we ignore LCD text because compositing
is necessary when we have composited descendants for correctness for fixed (and
eventually sticky) position.

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:195:
if (layer->GetLayoutObject().Style()->GetPosition() == EPosition::kFixed &&
I assume the check for EPosition::kFixed goes away once your other patch lands
right?

[smcgruer, 2017-04-27 14:53:30]

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:194:
const bool ignore_lcd_text = true;
On 2017/04/27 14:48:47, flackr wrote:
> nit: Couldn't hurt to add a comment that we ignore LCD text because
compositing
> is necessary when we have composited descendants for correctness for fixed
(and
> eventually sticky) position.

Done.

https://codereview.chromium.org/2839263002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:195:
if (layer->GetLayoutObject().Style()->GetPosition() == EPosition::kFixed &&
On 2017/04/27 14:48:46, flackr wrote:
> I assume the check for EPosition::kFixed goes away once your other patch lands
> right?

Yep, the other patch will be reduced to removing this check and renaming the
reason constant.

[smcgruer, 2017-05-03 18:56:17]

Ping for Blink OWNERS :)

[chrishtr, 2017-05-03 19:03:32]

https://codereview.chromium.org/2839263002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2839263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:198:
if (layer->GetLayoutObject().Style()->GetPosition() == EPosition::kFixed &&
The change here will affect fixed-position elements that are not fixed to the
viewport
also, correct? Intended?

[smcgruer, 2017-05-04 19:28:52]

https://codereview.chromium.org/2839263002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp
(right):

https://codereview.chromium.org/2839263002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/compositing/CompositingRequirementsUpdater.cpp:198:
if (layer->GetLayoutObject().Style()->GetPosition() == EPosition::kFixed &&
On 2017/05/03 19:03:31, chrishtr wrote:
> The change here will affect fixed-position elements that are not fixed to the
> viewport
> also, correct? Intended?

Intended, yes. My understanding was that we shouldn't need to composite in that
case. I'm welcome to being corrected if wrong :)

[chrishtr, 2017-05-05 01:18:03]

lgtm

Please update the CL description to match the actual purpose: don't promote
position: fixed
elements with composited descendants if they don't scroll.

[smcgruer, 2017-05-05 01:32:13]

Description was changed from

==========
Do not promote position: fixed elements unless they move with scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't promote position: fixed elements with composited descendants if they don't
scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[smcgruer, 2017-05-05 01:32:24]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-05-05 01:32:25]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org
Link to the patchset: https://codereview.chromium.org/2839263002/#ps40001
(title: "Address reviewer comment")

[commit-bot: I haz the power, 2017-05-05 01:32:46]

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-05-05 03:23:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493947944856690,
"parent_rev": "ed10e8d7378176b6346982f08e0930d18069f99e", "commit_rev":
"9b503962a017a8d69adfdae9300684c299922640"}

[commit-bot: I haz the power, 2017-05-05 03:24:18]

Description was changed from

==========
Don't promote position: fixed elements with composited descendants if they don't
scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Don't promote position: fixed elements with composited descendants if they don't
scroll

BUG=715699
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2839263002
Cr-Commit-Position: refs/heads/master@{#469584}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b503962a017a8d69adfdae93006...
==========

[commit-bot: I haz the power, 2017-05-05 03:24:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9b503962a017a8d69adfdae93006...