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

Various issues around conditions for syntax accepted by @supports #15482

Closed
upsuper opened this issue Feb 10, 2017 · 6 comments · Fixed by #17173 or #17792
Closed

Various issues around conditions for syntax accepted by @supports #15482

upsuper opened this issue Feb 10, 2017 · 6 comments · Fixed by #17173 or #17792
Assignees
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection)

Comments

@upsuper
Copy link
Contributor

upsuper commented Feb 10, 2017

[This can be tested with alert(CSS.supports(cond));]

There are several issues:

  • It rejects !important when the property is a shorthand. e.g. (background: none !important) returns false, but it should return true.
  • It incorrectly accepts conditions without parens. There are several cases which it returns true while it should return false:
    • (color: green) and (color: green) or (color: green) - correct form is ((color: green) and (color: green)) or (color: green)
    • not (color: rainbow) and not (color: iridescent) - correct form is (not (color: rainbow)) and (not (color: iridescent))
    • color: green - correct form is (color: green) (This is a Gecko issue)

Spec: https://drafts.csswg.org/css-conditional-3/#at-supports

(Servo also accepts unbalanced brackets inside the condition, which Gecko doesn't accept, but Blink accepts. This is probably a question to the spec.)

@upsuper upsuper added the A-content/css Interacting with CSS from web content (parsing, serializing, introspection) label Feb 10, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Feb 10, 2017

Spec issue has been filed in w3c/csswg-drafts#1016.

@SimonSapin
Copy link
Member

@upsuper
Copy link
Contributor Author

upsuper commented Feb 10, 2017

color: green is allowed in CSS.supports, see "wrapped in implied parentheses" in https://drafts.csswg.org/css-conditional-3/#the-css-interface

Probably we should file a bug to Gecko. Could you do that?

@SimonSapin
Copy link
Member

@Manishearth
Copy link
Member

#17173 fixes the !important thing

@Manishearth Manishearth self-assigned this Jun 5, 2017
bors-servo pushed a commit that referenced this issue Jun 6, 2017
Stop parsing @supports rules before Delimiter::Bang

Fixes #15482

Shorthand parsing uses `parse_entirely`, so we have to ask it to stop before the `!important`.

An alternate fix is to not use `parse_entirely` there and ensuring that all callers of `PropertyDeclaration::parse_into()` check that the input is exhausted. Two of the three callers do that anyway because they check for `!important`.

We also weren't checking for the end of the parser for `CSS.supports()`.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17173)
<!-- Reviewable:end -->
@upsuper upsuper reopened this Jul 19, 2017
@upsuper
Copy link
Contributor Author

upsuper commented Jul 19, 2017

This isn't completely fixed. The unbalanced cases are still failing in test_css_supports.html. Specifically, Gecko returns false for not ({ something @with (unbalanced brackets }) and (color: green) or an-extension(that is [unbalanced) while stylo returns true.

This is related to w3c/csswg-drafts#1016, but per @SimonSapin's comment there, we probably should follow Gecko's behavior on this.

bors-servo pushed a commit that referenced this issue Jul 20, 2017
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17792)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jul 20, 2017
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17792)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jul 20, 2017
Fix supports rule parsing issues with <any-value>

This eventually fixes #15482, as well as several reftests in mozilla-central which were added for [bug 883987](https://bugzilla.mozilla.org/show_bug.cgi?id=883987).

The new function should probably be moved into cssparser crate at some point.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17792)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/css Interacting with CSS from web content (parsing, serializing, introspection)
Projects
None yet
3 participants