New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 870157 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[css-contain] Make contain:layout a stacking context

Project Member Reported by chrishtr@chromium.org, Aug 2

Issue description

The CSSWG just resolved that contain:layout should be a stacking context.

https://github.com/w3c/csswg-drafts/issues/2942

This requires an intent to ship and research into compat.

Assigning to myself while I find an owner.

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1472919
 
Cc: r...@igalia.com e...@chromium.org
The solution for this should be similar to the one for  bug #868102 .

Note that it's not enough to modify ComputedStyle::UpdateIsStackingContext()
and include a ContainsLayout() check there.
We need to be sure that layout containment is actually applying to the element.

The tests in this PR will cover both issues (this one and  bug #868102 ):
https://github.com/web-platform-tests/wpt/pull/12269

@rego: are you able to own this bug then?
I'm afraid I'd need some extra help, like for  bug #868102 .
I was trying to fix that one unsuccessfully (check the last comment on the bug with more details).
If someone provides me further information I could work on this one too.
Labels: -OS-Mac
Summary: [css-contain] Make contain:layout a stacking context (was: Make contain:layout a stacking context)
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbc976dff2f1d80d5244de1efa91c4fdd5815066

commit dbc976dff2f1d80d5244de1efa91c4fdd5815066
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Mon Aug 06 15:29:29 2018

[css-contain] Link failures in WPT tests to the related bugs

Two of the failures are related to stacking contexts:
* external/wpt/css/css-contain/contain-paint-021.html
* external/wpt/css/css-contain/contain-layout-016.html

The other one was a mistake that has been already fixed in WPT
(see https://github.com/web-platform-tests/wpt/pull/12314):
* external/wpt/css/css-contain/contain-paint-001.html

TBR=eae@chromium.org

BUG= 870811 , 868102 , 870157 

Change-Id: I60e89decd2f939d1d9a850a1b8b556d31658e872
Reviewed-on: https://chromium-review.googlesource.com/1163673
Reviewed-by: Manuel Rego <rego@igalia.com>
Commit-Queue: Manuel Rego <rego@igalia.com>
Cr-Commit-Position: refs/heads/master@{#580878}
[modify] https://crrev.com/dbc976dff2f1d80d5244de1efa91c4fdd5815066/third_party/WebKit/LayoutTests/TestExpectations

So after realizing that we don't support any ruby-specific display values (issue #880802)
I guess we could fix this just doing what I said won't be enough in comment #1 (http://ix.io/1m1l/diff).

Now a question for chrishtr@ about this comment:
> This requires an intent to ship and research into compat.

Do you feel we need to do this for such a small change?
We've been fixing different spec bugs related to css-contain
(some that changed behavior similar to this one)
in the past months without any of this.

Current implementation has several issues and don't follow the spec
in different parts.
Should we do intents and research for similar fixes too?

In the case we want to do this, I guess first step would be to add a use counter
and then start to collect data to try to clarify the possible impact on current websites.
Should we follow a different approach?

Owner: r...@igalia.com
Cool!

As for compat, hmm. Can you give an example of one successfully made without intent to ship or
compat research?

These are a summary of the changes I've been doing on css-contain
to make Chromium implementation closer to the spec,
however I'm not sure if they're more or less common/important than this scencario:
* #843329: Here we disabled layout, paint and size containment for certain elements to follow the spec
  (like inlines, internal table and ruby elements, ...).
* #850169: Size containment should ignore contents in flexbox/grid.
* #843320: Layout containment overflowing contents as ink overflow.
  This one was trickier as DevTools (#851422) and news.google.com (#856131) got problems.
  DevTools was promtly fixed, however it seems news.google.com still have the issue.
* #785212: Layout containment abspos and fixed descendants.
  This is the one that got reverted as it broke YouTube mobile.
  Here we added a use counter 
  https://www.chromestatus.com/metrics/feature/timeline/popularity/2463
  and notified YouTube team that changed their code.
  Once the numbers were lower this was landed again.

Ok go ahead and make the change then, hopefully it will stick.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f01f396190da97d7218ee2f5d76b6b637bf3ced2

commit f01f396190da97d7218ee2f5d76b6b637bf3ced2
Author: Manuel Rego Casasnovas <rego@igalia.com>
Date: Fri Sep 07 22:28:21 2018

[css-contain] Make contain:layout a stacking context

The CSSWG has recently resolved that layout containment
should create a stacking context like paint containment
(see issue https://github.com/w3c/csswg-drafts/issues/2942).

This is the spec text about this
(https://drafts.csswg.org/css-contain/#containment-layout):
  "5. The element creates a stacking context."

The patch is just a simple change in
ComputedStyle::UpdateIsStackingContext() to add the new condition.

Apart from that TestExpectations file is updated
as we're now passing some tests from WPT.
We're also failing one test, but for an unrelated issue
(see bug #880802).

BUG= 870157 
TEST=external/wpt/css/css-contain/contain-layout-016.html
TEST=external/wpt/css/css-contain/contain-layout-018.html
TEST=external/wpt/css/vendor-imports/mozilla/mozilla-central-reftests/contain/contain-layout-stacking-context-001.html

Change-Id: Icdc102692801534364adf3fc0929192b886cdbc4
Reviewed-on: https://chromium-review.googlesource.com/1211922
Commit-Queue: Manuel Rego <rego@igalia.com>
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589689}
[modify] https://crrev.com/f01f396190da97d7218ee2f5d76b6b637bf3ced2/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/f01f396190da97d7218ee2f5d76b6b637bf3ced2/third_party/blink/renderer/core/style/computed_style.cc
[modify] https://crrev.com/f01f396190da97d7218ee2f5d76b6b637bf3ced2/third_party/blink/renderer/core/style/computed_style_test.cc

Status: Fixed (was: Assigned)

Sign in to add a comment