1479859 - IsAbsPosContainingBlockForAppropriateFrame / IsFixedPosContainingBlockForAppropriateFrame are broken

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

While writing bug 1472919 comment 26 I noticed that this code:
https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/layout/style/ComputedStyle.cpp#228-252
is broken.

In particular, it does the wrong thing when IsFixedPosContainingBlockForAppropriateFrame is true but IsFixedPosContainingBlock is actually false.

For example, if an inline element has 'perspective' set (which doesn't apply) and has a dynamic change of 'filter' (which should change whether the element establishes a containing block), then this code will remove the UpdateContainingBlock hint even though the hint should not be removed, since the dynamic change of 'filter' does change whether it establishes a containing block (since 'perspective' doesn't apply).

I think the fix is to separately test the parts that always apply and test the parts that don't apply to inlines.

(So far I've only inspected code; haven't tested.)

Comment 1

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

We actually don't trigger this bug right now because of another bug:  nsCSSFrameConstructor::ConstructInline doesn't actually call display->IsAbsPosContainingBlock(newFrame) (like, say, ConstructTable, ConstructDocElementFrame, etc.); it just checks for positioning.

Comment 2

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

Attachment: testcase that I think ought to show the bug

Comment 3

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

Chrome behaves correctly (clicking "Toggle Filter" toggles the abs-pos containing block behavior), so I think we should just fix both bugs.

Comment 4

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

Turns out this was my fault to begin with, in bug 1251075.

Comment 5

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

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

Comment 6

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

It turns out making this change pass tests requires fixing the review comment from bug 1472919 that originally led me down this rabbit hole, since otherwise these patches make us fail:

TEST-UNEXPECTED-FAIL | /css/css-contain/contain-paint-002.html | Testing http://web-platform.test:8000/css/css-contain/contain-paint-002.html == http://web-platform.test:8000/css/reference/pass_if_pass_below.html
TEST-UNEXPECTED-FAIL | /css/css-contain/contain-paint-011.html | Testing http://web-platform.test:8000/css/css-contain/contain-paint-011.html == http://web-platform.test:8000/css/reference/ref-filled-green-100px-square.xht
TEST-UNEXPECTED-FAIL | /css/css-contain/contain-paint-012.html | Testing http://web-platform.test:8000/css/css-contain/contain-paint-012.html == http://web-platform.test:8000/css/reference/ref-filled-green-100px-square.xht

While thinking about fixing this, I filed a bunch of spec issues.

Comment 7

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

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

Comment 8

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

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

Comment 9

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

Attachment: Bug 1479859 patch 4 - Test becoming a containing block for contain:paint only for those frames that support it. r=dholbert

This fixes the regression of three web-platform-test reftests:
  testing/web-platform/tests/css/css-contain/contain-paint-002.html
  testing/web-platform/tests/css/css-contain/contain-paint-011.html
  testing/web-platform/tests/css/css-contain/contain-paint-012.html
that was caused by patch 1, but it's written on top of the code in
patches 2 and 3 so it's easier to fix afterwards.

Comment 10

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

Attachment: Bug 1479859 patch 1 - Make inline frames be abs-pos containing blocks for reasons other than being relatively positioned. r=emilio

The basic change here is making nsCSSFrameConstructor::ConstructInline
use the function nsIFrame::IsAbsPosContainingBlock rather than testing
for only one of the conditions in it (being relatively or absolutely
positioned).  The rest of the code changes follow from that change.

I tested locally that the added test fails without the patch and passes
with it (either with or without the next patch).

Note that this causes a regression of three web-platform-test reftests:
  testing/web-platform/tests/css/css-contain/contain-paint-002.html
  testing/web-platform/tests/css/css-contain/contain-paint-011.html
  testing/web-platform/tests/css/css-contain/contain-paint-012.html
which will be fixed in patch 4, since that fix is easier to write after
patch 2.

Comment 11

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

Attachment: Bug 1479859 patch 2 - Send nsChangeHint_UpdateContainingBlock when containing block-ness changes due to one property change, while another property that might trigger containing block-ness doesn't do so because of the frame type. r=emilio

This fixes a rather subtle bug.  What the underlying code here is trying
to do is remove nsChangeHint_UpdateContainingBlock when some properties
that influence whether a frame is a containing block for absolutely
positioned or fixed positioned elements have changed, but the final
calculation of being a containing block has not changed.  However, the
old code was using a function that tested whether the style could
*possibly* lead to a frame being a containing block.  Some of the
properties (like the transform properties) that lead to being a
containing block, for example, don't apply to non-replaced inlines.
Some, however, do (such as 'filter').  So if there's a dynamic change
adding or removing a filter, on an inline that also has an *ignored*
transform property (like 'transform' or 'perspective') set, then the
code prior to this patch causes us to remove the UpdateContainingBlock
hint.

This patch fixes things by testing whether being a containing block
could have changed for *any* type of frame, by separately testing the
changes.

The added tests fail without the patch and pass with the patch, viewed
in isolation.  However, without the previous patch, test 003 passes.

Test 003 also fails in Chrome (but 001 and 002 pass).

Comment 12

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

Attachment: Bug 1479859 patch 3 - Add an nsIFrame::IsFrameOfType bit to say whether frames support contain:layout and contain:paint. r=dholbert

This is needed for patch 4.

This is based both on the wording in the spec and the discussion in
https://github.com/w3c/csswg-drafts/issues/2987

Comment 13

[Phabricator Automation]

Comment on attachment 8998059 [details]
Bug 1479859 patch 1 - Make inline frames be abs-pos containing blocks for reasons other than being relatively positioned.  r=emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.

https://phabricator.services.mozilla.com/D2813

Comment 14

[Phabricator Automation]

Comment on attachment 8998060 [details]
Bug 1479859 patch 2 - Send nsChangeHint_UpdateContainingBlock when containing block-ness changes due to one property change, while another property that might trigger containing block-ness doesn't do so because of the frame type.  r=emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.

https://phabricator.services.mozilla.com/D2814

Comment 15

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

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

Comment 16

[Phabricator Automation]

Comment on attachment 8998061 [details]
Bug 1479859 patch 3 - Add an nsIFrame::IsFrameOfType bit to say whether frames support contain:layout and contain:paint.  r=dholbert

Daniel Holbert [:dholbert] has approved the revision.

https://phabricator.services.mozilla.com/D2815

Comment 17

[Phabricator Automation]

Comment on attachment 8998058 [details]
Bug 1479859 patch 4 - Test becoming a containing block for contain:paint only for those frames that support it.  r=dholbert

Daniel Holbert [:dholbert] has approved the revision.

https://phabricator.services.mozilla.com/D2812

Comment 18

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

So I think I've addressed all of the review comments except for Emilio's request for better function names on patch 2... I'm not sure I have any better ideas.  (Yes, they're pretty verbose!)

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/950df70cf532fa9ac658ffd3fa3d022eba9bc4c1
Bug 1479859 patch 1 - Make inline frames be abs-pos containing blocks for reasons other than being relatively positioned.  r=emilio

https://hg.mozilla.org/integration/mozilla-inbound/rev/55c49d4fe2874a2306dc36db6c86e287fcc7aa11
Bug 1479859 patch 2 - Send nsChangeHint_UpdateContainingBlock when containing block-ness changes due to one property change, while another property that might trigger containing block-ness doesn't do so because of the frame type.  r=emilio

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e24328ba71484dd144b9d0d2fdd287a9327c1b4
Bug 1479859 patch 3 - Add an nsIFrame::IsFrameOfType bit to say whether frames support contain:layout and contain:paint.  r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/6897f3935cc77f531ce4b1ac5f5b6febdb010c58
Bug 1479859 patch 4 - Test becoming a containing block for contain:paint only for those frames that support it.  r=dholbert

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12347 for changes under testing/web-platform/tests

Comment 21

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 22

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

It looks like the web-platform-tests test-verify jobs [Tier 2] failed on Mac and Windows because of antialiasing differences (subpixel vs. grayscale AA) between test and reference.

However, it looks like the actual web-platform-tests reftests runs don't *run* the tests in the CSS directory at all... maybe because of these differences?  So I *think* no Tier-1 jobs will fail, and I suspect the Tier-2 job fails only when the test gets touched.

Comment 23

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

(I've asked jgraham where the bug is for most web-platform-tests reftests not being run on Mac and Windows... and I'm curious if the reason is the same reason that caused these test-verify jobs to fail.)

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/950df70cf532
https://hg.mozilla.org/mozilla-central/rev/55c49d4fe287
https://hg.mozilla.org/mozilla-central/rev/8e24328ba714
https://hg.mozilla.org/mozilla-central/rev/6897f3935cc7

Comment 25

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged