Skip to content

/ servo

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

Treat top-level number in calc() invalid #14421

Merged
merged 1 commit into from Dec 3, 2016
Merged

Treat top-level number in calc() invalid #14421

merged 1 commit into from Dec 3, 2016

Conversation

@upsuper
Copy link
Member

upsuper commented Nov 30, 2016 •
edited

This should probably considered as a temporary fix (for bug 1321206), to avoid assertion when trying to serialize calc value with only numbers. Certain properties (e.g. line-height) would eventually need to keep numbers inside calc.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

Currently, CalcLengthOrPercentage doesn't actually keep the number value. If we don't treat it invalid, we can end up generating empty calc() value when one contains numbers (e.g. calc(1)), which would violate assertion elsewhere that calc must not be empty.

This change is Reviewable
@highfive
Copy link

highfive commented Nov 30, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/specified/length.rs
  • @emilio: components/style/values/specified/length.rs
@highfive
Copy link

highfive commented Nov 30, 2016

warning Warning warning

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

upsuper commented Nov 30, 2016

This should probably have a test...
@upsuper upsuper force-pushed the upsuper-forks:patch-2 branch from 9643cdb to 58c4d08 Dec 2, 2016
@upsuper
Copy link
Member Author

upsuper commented Dec 2, 2016

Test added.
@wafflespeanut
Copy link
Member

wafflespeanut commented Dec 2, 2016

r? @emilio
@highfive highfive assigned emilio and unassigned larsbergstrom Dec 2, 2016
@dzbarsky
dzbarsky reviewed Dec 2, 2016
View changes
components/style/values/specified/length.rs
@@ -576,7 +576,6 @@ impl CalcLengthOrPercentage {
let mut ch = None;
let mut rem = None;
let mut percentage = None;
let mut number = None;

This comment has been minimized.

Sign in to view
@dzbarsky

dzbarsky Dec 2, 2016

Member

have you tested what other browsers do? I remember this bit being weird and inconsistent between browsers. In particular, Firefox doesn't get calc right.

This comment has been minimized.

Sign in to view
@upsuper

upsuper Dec 2, 2016 •
edited

Author Member

Firefox didn't get calc() right for properties which support numbers (particular line-height), but people are fixing that.

Not returning number as part of the result but treating it valid make no sense anyway. For properties which doesn't accept numbers, it is pretty consistent between browsers to reject calc() contains top level numbers. You can try this testcase:

<!DOCTYPE html>
<style>
div {
  width: 1px;
  width: calc(5); /* invalid */
  width: calc(10 + 20px); /* invalid */
}
</style>
<script>
document.write(document.styleSheets[0].cssRules[0].style.width);
</script>

It would likely crash Servo because of serializing empty calc(). See the stack backtrace in bug 1321206.
@emilio
emilio reviewed Dec 2, 2016
View changes
Copy link
Member

emilio left a comment

r=me with that.
components/style/values/specified/length.rs
@@ -606,7 +605,6 @@ impl CalcLengthOrPercentage {
FontRelativeLength::Rem(val) =>
rem = Some(rem.unwrap_or(0.) + val),
},

This comment has been minimized.

Sign in to view
@emilio

emilio Dec 2, 2016

Member

Can you add a TODO here pointing to this PR or similar?
@emilio
Copy link
Member

emilio commented Dec 2, 2016

@bors-servo delegate+
@bors-servo
Copy link
Contributor

bors-servo commented Dec 2, 2016

✌️ @upsuper can now approve this pull request
@upsuper
Treat top-level number in calc() invalid
Loading status checks…
6fec4d7 
Currently, CalcLengthOrPercentage doesn't actually keep the number
value, so if we don't treat it invalid, we can end up generating empty
`calc()` value when one contains top-level numbers (e.g. `calc(1)`),
which would violate assertion elsewhere that `calc` must not be empty.
@upsuper upsuper force-pushed the upsuper-forks:patch-2 branch from 58c4d08 to 6fec4d7 Dec 2, 2016
@upsuper
Copy link
Member Author

upsuper commented Dec 2, 2016

@bors-servo r=emilio
@bors-servo
Copy link
Contributor

bors-servo commented Dec 2, 2016

📌 Commit 6fec4d7 has been approved by emilio
@bors-servo
Copy link
Contributor

bors-servo commented Dec 2, 2016

⌛ Testing commit 6fec4d7 with merge 16e74c5...
bors-servo added a commit that referenced this pull request Dec 2, 2016
@bors-servo
Auto merge of #14421 - upsuper:patch-2, r=emilio
Loading status checks…
16e74c5 
Treat top-level number in calc() invalid

<!-- Please describe your changes on the following line: -->
This should probably considered as a temporary fix (for [bug 1321206](https://bugzilla.mozilla.org/show_bug.cgi?id=1321206)), to avoid assertion when trying to serialize calc value with only numbers. Certain properties (e.g. `line-height`) would eventually need to keep numbers inside calc.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Currently, CalcLengthOrPercentage doesn't actually keep the number value. If we don't treat it invalid, we can end up generating empty `calc()` value when one contains numbers (e.g. `calc(1)`), which would violate assertion elsewhere that `calc` must not be empty.

<!-- 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/14421)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 2, 2016

💔 Test failed - linux-rel-css
@KiChjang
Copy link
Member

KiChjang commented Dec 3, 2016 

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/changing-while-transition.htm:
  └ PASS [expected FAIL] changing transition-duration / values

  ▶ Unexpected subtest result in /css-transitions-1_dev/html/changing-while-transition.htm:
  └ PASS [expected FAIL] changing transition-property / values
@upsuper
Copy link
Member Author

upsuper commented Dec 3, 2016 •
edited by KiChjang

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Dec 3, 2016

⌛ Testing commit 6fec4d7 with merge 7c346e0...
bors-servo added a commit that referenced this pull request Dec 3, 2016
@bors-servo
Auto merge of #14421 - upsuper:patch-2, r=emilio
Loading status checks…
7c346e0 
Treat top-level number in calc() invalid

<!-- Please describe your changes on the following line: -->
This should probably considered as a temporary fix (for [bug 1321206](https://bugzilla.mozilla.org/show_bug.cgi?id=1321206)), to avoid assertion when trying to serialize calc value with only numbers. Certain properties (e.g. `line-height`) would eventually need to keep numbers inside calc.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Currently, CalcLengthOrPercentage doesn't actually keep the number value. If we don't treat it invalid, we can end up generating empty `calc()` value when one contains numbers (e.g. `calc(1)`), which would violate assertion elsewhere that `calc` must not be empty.

<!-- 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/14421)
<!-- Reviewable:end -->
@highfive highfive removed the S-tests-failed label Dec 3, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Dec 3, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 6fec4d7 into servo:master Dec 3, 2016
3 checks passed
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-forks:patch-2 branch Dec 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilio emilio

@dzbarsky dzbarsky

Assignees

@emilio emilio

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@upsuper @highfive @wafflespeanut @emilio @bors-servo @KiChjang @dzbarsky @larsbergstrom
You can’t perform that action at this time.