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

style: Properly track whether negative values of calc() are allowed #13121

Merged
merged 1 commit into from Sep 1, 2016

Conversation

@emilio
Copy link
Member

emilio commented Aug 30, 2016

r? @SimonSapin


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • There are tests for these changes OR

In order to clamp them at computed value time.


This change is Reviewable

@emilio
Copy link
Member Author

emilio commented Aug 30, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Aug 30, 2016

Trying commit 9a49f1b with merge be54daa...

bors-servo added a commit that referenced this pull request Aug 30, 2016
style: Properly track whether negative values of calc() are allowed

<!-- Please describe your changes on the following line: -->

r? @SimonSapin

---
<!-- 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

<!-- Either: -->
- [x] There are tests for these changes OR

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

In order to clamp them at computed value time.
@bors-servo
Copy link
Contributor

bors-servo commented Aug 30, 2016

💔 Test failed - mac-rel-wpt

@SimonSapin
Copy link
Member

SimonSapin commented Aug 30, 2016

https://drafts.csswg.org/css-values/#calc-range

the used value resulting from an expression must be clamped to the range allowed in the target context.

(Emphasis added.) So to_computed_value is not the place to do the clamping.

I’d suggest adding a method on the computed-value-types that layout can call. And maybe make some fields private to force it to go through that method?

@bors-servo
Copy link
Contributor

bors-servo commented Aug 30, 2016

The latest upstream changes (presumably #13075) made this pull request unmergeable. Please resolve the merge conflicts.

In order to clamp them at computed value time.
@emilio emilio force-pushed the emilio:negative-calc branch from 9a49f1b to 2617d15 Aug 31, 2016
@emilio
Copy link
Member Author

emilio commented Aug 31, 2016

For the record, I opened a spec issue about this. w3c/csswg-drafts#434

@nox nox removed the S-needs-rebase label Aug 31, 2016
@SimonSapin
Copy link
Member

SimonSapin commented Sep 1, 2016

Apparently this matches WebKit and Blink: w3c/csswg-drafts#434 (comment)

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 1, 2016

📌 Commit 2617d15 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Sep 1, 2016

Testing commit 2617d15 with merge fbf77e4...

bors-servo added a commit that referenced this pull request Sep 1, 2016
style: Properly track whether negative values of calc() are allowed

<!-- Please describe your changes on the following line: -->

r? @SimonSapin

---
<!-- 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

<!-- Either: -->
- [x] There are tests for these changes OR

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

In order to clamp them at computed value time.

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

bors-servo commented Sep 1, 2016

@bors-servo bors-servo merged commit 2617d15 into servo:master Sep 1, 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
@emilio emilio deleted the emilio:negative-calc branch Sep 1, 2016
@emilio
Copy link
Member Author

emilio commented Sep 1, 2016

This was still wrong, my fault.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.