Closed Bug 1227766 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(4 files, 3 obsolete files)

3.95 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.46 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.91 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
2.80 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → dbaron
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.
Attachment #8691688 - Flags: review?(dholbert)
Attachment #8691687 - Attachment is obsolete: true
Attachment #8691687 - Flags: review?(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.)
Attachment #8691685 - Flags: review?(dholbert) → review+
(/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 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
Attachment #8691686 - Flags: review?(dholbert) → review+
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.)
Attachment #8691688 - Flags: review?(dholbert) → review+
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
Attachment #8691690 - Flags: review?(dholbert) → review+
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
Attachment #8696207 - Attachment is obsolete: true
Attachment #8696207 - Flags: review?(dholbert)
Attachment #8696207 - Attachment is obsolete: false
Attachment #8696207 - Attachment is obsolete: true
Attachment #8696208 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.