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

The computed `font-size` and `line-height` must be clamped when the specified value is out-of-range #15296

Closed
upsuper opened this issue Jan 30, 2017 · 8 comments
Assignees
Labels

Comments

@upsuper
Copy link
Member

@upsuper upsuper commented Jan 30, 2017

Although it is invalid to specify something like font-size: -1px, it is allowed to do font-size: calc(-1px). In the latter case, font-size must be computed to zero rather than -1px.

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

The relevant code is probably https://github.com/servo/servo/blob/master/components/style/properties/longhand/font.mako.rs#L378-L382

(I found font-size and line-height, but other properties may be affected as well.)

@Maaarcocr
Copy link
Contributor

@Maaarcocr Maaarcocr commented Jan 30, 2017

What I'm going to say may be really wrong, due to my ignorance about how style works in servo.

Wouldn't it be better if every property that can implement calc() should provide a maximum and minimum value for calc()?

@Manishearth
Copy link
Member

@Manishearth Manishearth commented Jan 31, 2017

We should add a to_computed_value_positive method that takes the result of to_computed_value() and clamps it.

@upsuper
Copy link
Member Author

@upsuper upsuper commented Feb 10, 2017

That should be to_computed_value_non_negative. I actually think we should somehow put non-negative into the type system, so that we can enforce it everywhere. Many properties would need this.

@nox
Copy link
Member

@nox nox commented Apr 13, 2017

I am pretty sure I fixed this in #16382.

@nox nox closed this Apr 13, 2017
@upsuper
Copy link
Member Author

@upsuper upsuper commented May 11, 2017

Actually it isn't fixed. The issue can still be reproduced with

<div id="test"></div>
<script>
test.style.fontSize = 'calc(-1px)';
alert(getComputedStyle(test).fontSize);
</script>
@upsuper upsuper reopened this May 11, 2017
@nox nox self-assigned this May 11, 2017
@nox
Copy link
Member

@nox nox commented May 11, 2017

SpecifiedValue::Length(LengthOrPercentage::Calc(ref calc)) => {
let calc = calc.to_computed_value(context);
calc.length() + base_size.resolve(context)
.scale_by(calc.percentage())
}

@nox
Copy link
Member

@nox nox commented May 16, 2017

I'm not sure to_computed_value_non_negative would make a lot of sense on CalcLengthOrPercentage, we can't know for sure whether 30px - 10% is negative or not. font-size is a special snowflake though.

@nox
Copy link
Member

@nox nox commented May 16, 2017

Actually am wrong, but there should still not be any to_computed_value_non_negative method, instead calc() values should remember that they should clamp themselves when needed.

bors-servo added a commit that referenced this issue May 16, 2017
Refactor how calc() clamping is done on computed values (fixes #15296)
bors-servo added a commit that referenced this issue May 17, 2017
Refactor how calc() clamping is done on computed values (fixes #15296)

<!-- 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/16889)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 18, 2017
Refactor how calc() clamping is done on computed values (fixes #15296)

<!-- 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/16889)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue May 18, 2017
Refactor how calc() clamping is done on computed values (fixes #15296)

<!-- 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/16889)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.