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

Store raw string for prop decl in @supports #17813

Merged
merged 1 commit into from Jul 21, 2017

Conversation

@upsuper
Copy link
Member

commented Jul 21, 2017

This fixes the serialization issue of @supports rule that whitespaces are not preserved like in other browsers.

It makes the work a bit redundant (the property name and colon is parsed twice), but I suppose this isn't a big deal.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 21, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/stylesheets/supports_rule.rs
  • @canaltinova: components/style/stylesheets/supports_rule.rs
  • @emilio: components/style/stylesheets/supports_rule.rs
@highfive

This comment has been minimized.

Copy link

commented Jul 21, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@highfive highfive assigned SimonSapin and unassigned KiChjang Jul 21, 2017

@jdm

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

error[E0560]: struct `style::stylesheets::supports_rule::Declaration` has no field named `prop`

  --> /home/travis/build/servo/servo/components/script/dom/css.rs:33:34

   |

33 |         let decl = Declaration { prop: property.into(), val: value.into() };

   |                                  ^^^^^ `style::stylesheets::supports_rule::Declaration` does not have this field

error[E0560]: struct `style::stylesheets::supports_rule::Declaration` has no field named `val`

  --> /home/travis/build/servo/servo/components/script/dom/css.rs:33:57

   |

33 |         let decl = Declaration { prop: property.into(), val: value.into() };

   |                                                         ^^^^ `style::stylesheets::supports_rule::Declaration` does not have this field

error: aborting due to 2 previous errors

error: Could not compile `script`.

@jdm jdm added the S-fails-travis label Jul 21, 2017

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

I think that "evaluating" an @supports condition should not be separate from parsing in the first place (which would solve the "parsing twice" thing), but that doesn’t need to block this PR.

r=me with libscript fixed. (Try ./mach check for type-checking without compiling.)

@upsuper upsuper force-pushed the upsuper:supports-decl branch from abeffc9 to adee1e4 Jul 21, 2017

@upsuper

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@SimonSapin Updated. The failure should be fixed now. You probably want to have another look?

@SimonSapin

This comment has been minimized.

Copy link
Member

commented Jul 21, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

📌 Commit adee1e4 has been approved by SimonSapin

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

⌛️ Testing commit adee1e4 with merge d1ac8b2...

bors-servo added a commit that referenced this pull request Jul 21, 2017
Auto merge of #17813 - upsuper:supports-decl, r=SimonSapin
Store raw string for prop decl in @supports

This fixes the serialization issue of `@supports` rule that whitespaces are not preserved like in other browsers.

It makes the work a bit redundant (the property name and colon is parsed twice), but I suppose this isn't a big deal.

<!-- 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/17813)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

@bors-servo bors-servo merged commit adee1e4 into servo:master Jul 21, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@upsuper upsuper deleted the upsuper:supports-decl branch Jul 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.