The Wayback Machine - https://web.archive.org/web/20240803004650/https://github.com/web-platform-tests/wpt/issues/33767
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[@layer] Add tests for !important styles in @layer #33767

Closed
romainmenke opened this issue Apr 23, 2022 · 6 comments · Fixed by #34074
Closed

[@layer] Add tests for !important styles in @layer #33767

romainmenke opened this issue Apr 23, 2022 · 6 comments · Fixed by #34074

Comments

@romainmenke
Copy link
Contributor

romainmenke commented Apr 23, 2022

Currently the behaviour of !important styles combined with @layer rules doesn't have sufficient test coverage.

The most relevant test I could find was added in this PR : #31944

It does however appear that this test has a bug and will always pass (I might be wrong about this) :

 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">
 <style>
 @layer A { target { background-color: red !important} }
 </style>
 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">

Safari TP and Chrome Canary have also started to diverge.

@layer { target { color: red !important; } }
@layer { target { color: green !important; } }

For this style the current stable versions both give priority to red.

Chrome Canary now gives priority to green.

I am not a 100% sure which one is actually correct.


There is also some weirdness in the inspector for both stable Safari and Chrome.
Having better tests for !important together with @layer might also help to resolve those issues.

Screenshot 2022-04-23 at 11 23 31

@mirisuzanne
Copy link
Contributor

I haven't looked carefully at what tests exist, but I can answer what should happen in this case. The layer order (including the placement of unlayered styles) is reversed in the !important layers. So the resulting priority, from highest to lowest, is:

  • !important lowest layer
  • !important highest layer
  • !important unlayered
  • normal unlayered
  • normal highest layer
  • normal lowest layer

In this example:

 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">
 <style>
 @layer A { target { background-color: red !important} }
 </style>
 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">

The result should be green applied from the first of the two anonymous layers, but I agree the test is confusing and probably unhelpful. The second layer should have a different color specified to make the test more accurate.


In this example:

@layer { target { color: red !important; } }
@layer { target { color: green !important; } }

Chrome Canary has the correct behavior. The first layer important declaration should override the second layer important declaration. If green is intended as a sign of success, these declarations should be switched.

@lilles
Copy link
Member

lilles commented May 9, 2022

Currently the behaviour of !important styles combined with @layer rules doesn't have sufficient test coverage.

The most relevant test I could find was added in this PR : #31944

It does however appear that this test has a bug and will always pass (I might be wrong about this) :

 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">
 <style>
 @layer A { target { background-color: red !important} }
 </style>
 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green !important}}">

Looking at the Chromium-CL that introduced this test, it's testing a style sharing/caching issue in Chrome. It should probably have a rel=help link to the crbug to make that visible.

Safari TP and Chrome Canary have also started to diverge.

@layer { target { color: red !important; } }
@layer { target { color: green !important; } }

This is red in both Chrome Canary and Chrome Stable for me on Mac.

For this style the current stable versions both give priority to red.

Chrome Canary now gives priority to green.

Not for me.

@romainmenke
Copy link
Contributor Author

Not for me.

🤔 I normally double check before reporting an issue but I am sure you are right as I can no longer reproduce this.

Unsure if I confused the color in the inspector for the actual color or if there was something else on my end.


Looking at the Chromium-CL that introduced this test, it's testing a style sharing/caching issue in Chrome. It should probably have a rel=help link to the crbug to make that visible.

It was indeed not clear to me that this test was added for a specific Chrome issue.

Would it be possible to add some general tests for !important behaviour in @layer?
As far as I can tell this isn't tested on it's own.

There are some tests with @layer and !important but these are focussed on other features (@import, revert-layer, layer vs inline, ...).

@lilles
Copy link
Member

lilles commented May 9, 2022

Would it be possible to add some general tests for !important behaviour in @layer? As far as I can tell this isn't tested on it's own.

There are some tests with @layer and !important but these are focussed on other features (@import, revert-layer, layer vs inline, ...).

Yes, feel free to add a PR with some basic !important tests.

@romainmenke
Copy link
Contributor Author

@lilles I've added a PR with some basic tests.


I've also been able to pinpoint the issue I mentioned earlier.

Screenshot 2022-05-16 at 16 53 06

In Chrome Canary important layered styles give unexpected results only when there previously was a style element with only unlayered important styles.

These tests pass for all current versions of Safari, Chrome, Firefox.

My initial description of the issue above was incomplete and when I tried to reproduce it from that description I was unable to. Which is why it seemed to have gone away.

@romainmenke
Copy link
Contributor Author

Also filed a bug report about this issue : https://bugs.chromium.org/p/chromium/issues/detail?id=1326791

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants