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-anchor-position-1] CSSTryRule.style.setProperty can set disallowed properties in a @try rule #9042

Closed
xiaochengh opened this issue Jul 7, 2023 · 1 comment · Fixed by #9044

Comments

@xiaochengh
Copy link
Contributor

The spec currently blocks disallowed properties of a @try rule at parse time:

The @try rule only accepts the following properties:

However, with the CSSOM API, we can still smuggle a disallowed property into a @try rule, and then completely break position fallback:

tryRule.style.setProperty('position', 'static');

Proposal:

We should also block the disallowed rules at used value time. In paticular, in step 2.1 of the determine the position fallback styles algorithm, change

Apply the styles in fallback styles to el

into

Apply the values of the properties that are accepted by @try rules in fallback styles to el


This issue is not entirely new, but existing ones don't cause issues other than changing the result of cssText. For example:

  • CSSFontFaceRule.style.setProperty can add an unrelated property (e.g., color) into a @font-face, which however doesn't do anything
  • CSSKeyFrameRule.style.setProperty can add an important declaration into a keyframe. However, it's treated as a normal declaration in the cascade because there's no important animation origin

However, allowing position in a @try rule completely breaks positioned layout.

There's currently no mechanism to stop CSSStyleDeclaration.setProperty from setting a property based on the parent rule type. An alternative solution is to patch it there, but I don't think it's a good idea to modify a long-established spec for this new feature.

And it seems generally safer (and less work to do) to just follow the exsting pattern: disallowed declarations can be added, but they won't do anything.

@tabatkins
Copy link
Member

Agreed on all points - it would make sense to block this at the setProperty() level (the same way we check context for inserting child rules) but changing that might be hard at this point, so we should patch the Anchor algorithm instead.

We can still look into fixing setProperty() as a separate issue; it won't block anything here.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 7, 2023
This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `@try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `@try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `@try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
aarongable pushed a commit to chromium/chromium that referenced this issue Jul 10, 2023
This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `@try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `@try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `@try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapov@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168271}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 10, 2023
This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `@try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `@try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `@try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapov@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168271}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 10, 2023
This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `@try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `@try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `@try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapov@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168271}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 20, 2023
…OM API classes, a=testonly

Automatic update from web-platform-tests
[anchor-position] Properly implement CSSOM API classes

This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `@try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `@try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `@try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapov@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1168271}

--

wpt-commits: 1379ecf2f548bc63470115859203c788834b819f
wpt-pr: 40926
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jul 21, 2023
…OM API classes, a=testonly

Automatic update from web-platform-tests
[anchor-position] Properly implement CSSOM API classes

This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapovchromium.org>
Commit-Queue: Xiaocheng Hu <xiaochenghchromium.org>
Cr-Commit-Position: refs/heads/main{#1168271}

--

wpt-commits: 1379ecf2f548bc63470115859203c788834b819f
wpt-pr: 40926

UltraBlame original commit: dc2512898d971adead2e44cc615a6a9edc1c37f8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jul 21, 2023
…OM API classes, a=testonly

Automatic update from web-platform-tests
[anchor-position] Properly implement CSSOM API classes

This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapovchromium.org>
Commit-Queue: Xiaocheng Hu <xiaochenghchromium.org>
Cr-Commit-Position: refs/heads/main{#1168271}

--

wpt-commits: 1379ecf2f548bc63470115859203c788834b819f
wpt-pr: 40926

UltraBlame original commit: dc2512898d971adead2e44cc615a6a9edc1c37f8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Jul 21, 2023
…OM API classes, a=testonly

Automatic update from web-platform-tests
[anchor-position] Properly implement CSSOM API classes

This patch adds proper implementation of the CSSPositionFallbackRule
and CSSTryRule API as specified [1]:

1. It reimplements `CSSPositionFallbackRule` as a subclass of
   `CSSGroupingRule`, and `StyleRulePositionFallback` as a subclass
   of `StyleRuleGroup`.

2. It adds some code to `CSSGroupingRule.insertRule()` to make sure
   the function only adds `try` rules into `CSSPositionFallbackRule`.

3. Since `CSSStyleRule.style.setProperty()` can add disallowed
   properties into a `try` block [2], this patch also adds filtering
   into the style cascade to make sure when applying the style from
   a `try` rule, only allowed properties' values are applied.

This patch also manually adds the IDL of the APIs into wpt so that we
can add idlharness test without waiting for the IDL auto roller.

[1] https://drafts.csswg.org/css-anchor-position-1/#interfaces
[2] w3c/csswg-drafts#9042

Bug: 1309178
Change-Id: Ib7700605a9968a3547fbe99197c882e8aa6caaea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4668131
Reviewed-by: Daniil Sakhapov <sakhapovchromium.org>
Commit-Queue: Xiaocheng Hu <xiaochenghchromium.org>
Cr-Commit-Position: refs/heads/main{#1168271}

--

wpt-commits: 1379ecf2f548bc63470115859203c788834b819f
wpt-pr: 40926

UltraBlame original commit: dc2512898d971adead2e44cc615a6a9edc1c37f8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants