1227766 - will-change should sometimes cause creation of containing block for fixed-positioned elements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://drafts.csswg.org/css-will-change/#valdef-will-change-custom-ident now says:

If any non-initial value of a property would cause the element to generate a containing block for fixed-position elements, specifying that property in will-change must cause the element to generate a containing block for fixed-position elements.


I don't think it said this back when we initially implemented will-change.

We should implement this.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Add flag for CSS properties that establish a containing block for fixed positioned elements

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Add will-change bit for establishing a containing block for fixed positioned elements

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Make will-change cause creation of a containing block for fixed and absolutely positioned elements when needed

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4 - Tests for will-change establishing a fixed-pos and abs-pos containing block

Without the previous patches, the tests:
  will-change-fixpos-cb-contain-1.html
  will-change-fixpos-cb-filter-1.html
  will-change-fixpos-cb-perspective-1.html
  will-change-fixpos-cb-transform-style-1.html
fail, but they pass with the patches.  The new tests:
  will-change-fixpos-cb-height-1.html
  will-change-fixpos-cb-transform-1.html
pass both ways:  the first because it tests that nothing happens, and
the second because we were separately testing the will-change bit for
transform via HasTransform.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Make will-change cause creation of a containing block for fixed and absolutely positioned elements when needed

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ed5feb6d47&group_state=expanded

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8691685 [details] [diff] [review]
patch 1 - Add flag for CSS properties that establish a containing block for fixed positioned elements

Review of attachment 8691685 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 1 (and sorry for the delay -- was busy/AFK over Thanksgiving holiday long-weekend).  Just one note:

::: layout/style/nsCSSProps.h
@@ +256,5 @@
>  #define CSS_PROPERTY_INTERNAL                     (1<<28)
>  
> +// This property has values that can establish a containing block for
> +// fixed positioned and absolutely positioned elements.
> +#define CSS_PROPERTY_FIXPOS_CB                    (1<<29)

Might be worth referencing nsStyleDisplay::IsFixedPosContainingBlock() from this comment, and/or updating that function's documentation to mention this CSS_PROPERTY_FIXPOS_CB flag. (Presumably that function & the list of properties with this flag need to be kept in sync.)

Comment 8

[Daniel Holbert [:dholbert]]

(/me notices that part 4 does actually update IsFixedPosContainingBlock.  My suggestion in comment 7 is still applicable, though -- the preexisting pieces of IsFixedPosContainingBlock() do still need to match the list of properties for which CSS_PROPERTY_FIXPOS_CB is set, I think, and that's worth documenting somewhere.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8691686 [details] [diff] [review]
patch 2 - Add will-change bit for establishing a containing block for fixed positioned elements

Review of attachment 8691686 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 8691688 [details] [diff] [review]
patch 4 - Tests for will-change establishing a fixed-pos and abs-pos containing block

Review of attachment 8691688 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the tests, just one copypaste bug to address:

::: layout/reftests/w3c-css/submitted/will-change/will-change-fixpos-cb-contain-1.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset="utf-8">
> +<title>CSS will-change: 'will-change: filter' creates a stacking context</title>

This title is incorrect:
 (1) We're really testing "contain", not "filter"
 (2) We're really testing that it creates a fixed-position containing block (not that it creates a stacking context)

Looks like all of the tests here have this same <title> -- needs updating in all of them (with a different property in each test).

::: layout/reftests/w3c-css/submitted/will-change/will-change-fixpos-cb-height-1.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<meta charset="utf-8">
> +<title>CSS will-change: 'will-change: filter' creates a stacking context</title>

(When fixing this copypaste <title> issue mentioned above, note that this particular test's title needs a different tweak from the rest -- here, we're testing that we do *not* create a fixedpos containing block.  Whereas the rest of the tests are testing that we *do* create a fixedpos containing block.)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8691690 [details] [diff] [review]
patch 3 - Make will-change cause creation of a containing block for fixed and absolutely positioned elements when needed

Review of attachment 8691690 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on part 3

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f0d955ce78b140efcbf42e8027cd3d7a92cdc5
Bug 1227766 patch 1 - Add flag for CSS properties that establish a containing block for fixed positioned elements.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/d272cd8346700eca2940496711697bada98f01f8
Bug 1227766 patch 2 - Add will-change bit for establishing a containing block for fixed positioned elements.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/daf35b346b5d103dce479ac8368a642d867cacd7
Bug 1227766 patch 3 - Make will-change cause creation of a containing block for fixed and absolutely positioned elements when needed.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ed1feeef7400a8a6d9fd1dd406d552294aa164
Bug 1227766 patch 4 - Tests for will-change establishing a fixed-pos and abs-pos containing block.  r=dholbert

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/02f0d955ce78
https://hg.mozilla.org/mozilla-central/rev/d272cd834670
https://hg.mozilla.org/mozilla-central/rev/daf35b346b5d
https://hg.mozilla.org/mozilla-central/rev/e7ed1feeef74

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1227766 - Make will-change:position create a containing block for absolutely-positioned elements. r?dholbert

Bug 1227766 - Make will-change:position create a containing block for absolutely-positioned elements.  r?dholbert

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: MozReview Request: Bug 1227766 - Add test that position does not create a fixed-position containing block. r?dholbert

Bug 1227766 - Add test that position does not create a fixed-position containing block.  r?dholbert

I confirmed that the new test fails without the previous patch (since
the abspos element is 50px from the left edge of the viewport instead of
150px, and at the top edge instead of 100px from the top).  The test
passes with the patch.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696208 [details]
MozReview Request: Bug 1227766 - Add test that position does not create a fixed-position containing block.  r?dholbert

Sorry, published these to the wrong bug; bug 1230701 was intended.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/27291/#review24677

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/27293/#review24679

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/27293/#review24681

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696207 [details]
MozReview Request: Bug 1227766 - Make will-change:position create a containing block for absolutely-positioned elements.  r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27291/diff/1-2/

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8696208 [details]
MozReview Request: Bug 1227766 - Add test that position does not create a fixed-position containing block.  r?dholbert

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27293/diff/1-2/