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

[css-nesting] Inserting new sub-rules with .style #7933

Closed
sesse opened this issue Oct 21, 2022 · 9 comments
Closed

[css-nesting] Inserting new sub-rules with .style #7933

sesse opened this issue Oct 21, 2022 · 9 comments
Labels
css-nesting-1 Current Work

Comments

@sesse
Copy link
Contributor

sesse commented Oct 21, 2022

During test writing, this came to mind:

<style>
  .a {
    color: red;
    &.c { color: hotpink; }
  }
</style>
<script>
document.styleSheets[0].rules[0].style = 'color: navy; &.b{color: green;}';
</script>

What should happen?

  1. The change is silently ignored.
  2. “color: red” is changed to “color: navy” and no other changes take place.
  3. The same, but an additional rule “&.b { color: green; }” is added, above &.c.
  4. The same, but it's instead added after &.c.
  5. The same, but &.c is removed.

I'd say the sane thing to do is 2 since we parse a list of declarations (and already ignore @media etc. when setting .style), but this needs to be explicit in the spec, I think.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
@lilles lilles added the css-nesting-1 Current Work label Oct 21, 2022
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
@LeaVerou
Copy link
Member

Setting .style should replace anything inside the rule so: “color: red” is changed to “color: navy”, &.c is removed, and &.b is added (not sure what number that is in your list).

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3970663
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062164}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3970663
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062164}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 21, 2022
Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3970663
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062164}
@sesse
Copy link
Contributor Author

sesse commented Oct 21, 2022

Setting .style should replace anything inside the rule so: “color: red” is changed to “color: navy”, &.c is removed, and &.b is added (not sure what number that is in your list).

That would be 5. But 5 is inconsistent with current behavior for e.g. @media, and it's also inconsistent with the current definition of .style in CSSOM (which says to forward to .cssText, which says to parse a CSS declaration block, which cannot contain other rules). https://www.w3.org/TR/cssom-1/#dom-cssstyledeclaration-csstext

@Loirooriol
Copy link
Contributor

If a CSSStyleRule has CSSStyleDeclaration style and CSSRuleList cssRules as different members, it seems strange that assigning to one of them will change the other one...

This seems a good argument for @FremyCompany's proposal that keeps each thing separate.

@LeaVerou
Copy link
Member

This seems a good argument for @FremyCompany's proposal that keeps each thing separate.

Or a good argument to hang this off of CSSStyleDeclaration, paving the way for nested inline styles too.

@sesse
Copy link
Contributor Author

sesse commented Oct 21, 2022

paving the way for nested inline styles too.

I hate to repeat myself, but we are unlikely to implement this, at least in a form that is not strongly restricted. (I cannot speak for Gecko and WebKit, but both have already signaled worries about CSSOM complexity as I understand it, so increasing it further might not be the best course of action.)

@LeaVerou
Copy link
Member

paving the way for nested inline styles too.

I hate to repeat myself, but we are unlikely to implement this, at least in a form that is not strongly restricted. (I cannot speak for Gecko and WebKit, but both have already signaled worries about CSSOM complexity as I understand it, so increasing it further might not be the best course of action.)

Given that many things that were previously hard to implement are currently being implemented, I'd be wary of choosing a CSSOM design that excludes this possibility. That would mean that once we do have nested inline styles, we'd end up with an inconsistent CSSOM.

@sesse
Copy link
Contributor Author

sesse commented Oct 21, 2022

That would mean that once we do have nested inline styles, we'd end up with an inconsistent CSSOM.

It already is inconsistent in exactly this fashion. Reading .cssText includes conditional group rules (such as @media or @container), but you cannot set them with .style (which forwards to .cssText). Making setting .style erase existing nested rules would just be even more inconsistent. (Also, “once we do have nested inline styles” may be construed as somewhat assumptious.)

@Loirooriol
Copy link
Contributor

Can @media and @container already be nested inside a style rule? I thought this was new in css-nesting. So it all seems part of the same issue.

@sesse
Copy link
Contributor Author

sesse commented Oct 21, 2022

No, you're right. But if you take e.g. a @media rule (at top level), it can contain another @media rule, whose cssText can be read but setting is ignored. Anyway, after this discussion, I'm convinced the standard as of today says what I had hoped it would, so I'm closing this as no action needed. (I've added a WPT test to verify it, so interop should be fine.)

@sesse sesse closed this as completed Oct 21, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Nov 7, 2022
…e., a=testonly

Automatic update from web-platform-tests
[css-nesting] Add a CSSOM test for .style.

Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3970663
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062164}

--

wpt-commits: 38ddcd38f6da0171075e02792c777485f6d51319
wpt-pr: 36587
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Nov 11, 2022
…e., a=testonly

Automatic update from web-platform-tests
[css-nesting] Add a CSSOM test for .style.

Cf. w3c/csswg-drafts#7933.

Change-Id: Id0327f0188f6edd3faeda92aafd06b5f5c518fe4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3970663
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1062164}

--

wpt-commits: 38ddcd38f6da0171075e02792c777485f6d51319
wpt-pr: 36587
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-nesting-1 Current Work
Projects
None yet
Development

No branches or pull requests

4 participants