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

stylo: Bug 1374233 - Clamp interpolated values for properties which need to be restricted #17783

Merged
merged 13 commits into from Aug 7, 2017

Conversation

@BorisChiou
Copy link
Contributor

commented Jul 19, 2017

Some properties only accept non-negative values, or values greater than or equal to one. It is possible to produce an negative interpolated values while using negative timing functions, so we have to apply a restriction to these values to avoid getting invalid values.

For example, line-height must be non-negative, but the output progress of some timing functions (e,g. cubic-bezier(0.25, -2, 0.75, 1)) may be a negative value, so the interpolated result of line-height is also negative.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix Bug 1374233.
  • These changes do not require tests because we have tests in Gecko side already.

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 19, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/inherited_text.mako.rs, components/style/values/computed/text.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/data.py and 13 more
  • @canaltinova: components/style/properties/longhand/inherited_text.mako.rs, components/style/values/computed/text.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/data.py and 13 more
  • @emilio: components/style/properties/longhand/inherited_text.mako.rs, components/style/values/computed/text.rs, components/style/properties/longhand/padding.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/data.py and 13 more
@highfive

This comment has been minimized.

Copy link

commented Jul 19, 2017

warning Warning warning

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

r? @nox

@highfive highfive assigned nox and unassigned wafflespeanut Jul 19, 2017

@nox

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

So my first issue with this is: even if we clamp at conversion-time, what about the result of interpolation? When does that get clamped?

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

We don't change the result of interpolation. All we need to do is make sure we put the correct non-negative result into ServoComputedValues (by uncompute() and then cascade it to ServoComputedValues), with Animation/Transition cascade level, so we can see the correct animation on the screen.

The out of range interpolated values are still needed to be preserved during calculating AnimationValue in the case for additive or accumulative animations.


impl AnimatedValueAsComputed for T {
#[inline]
fn restrict_value(self, restriction_type: Restriction) -> Self {

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

AFAIK there is never a need for such a method given Animatable methods can clamp directly.

/// Restriction types.
#[derive(Copy, Clone, Debug, PartialEq)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub enum Restriction {

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

We already have a type for that, please replace by AllowedNumericType.

}}
}

impl<Angle, Factor, Length, DropShadow> ToAnimatedValue for Filter<Angle, Factor, Length, DropShadow>

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

No, define the Factor impl yourself instead of clamping from here.

color: ToAnimatedValue::from_animated_value(animated.color),
horizontal: ToAnimatedValue::from_animated_value(animated.horizontal),
vertical: ToAnimatedValue::from_animated_value(animated.vertical),
blur: convert_and_clamp_nonnegative_animated_value!(animated.blur)

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

Make the blur value its own wrapper type that clamps correctly.

@@ -61,3 +62,18 @@ impl Animatable for LineHeight {
}
}
}

impl AnimatedValueAsComputed for LineHeight {

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

Why doesn't the computed type clamp itself during animation?

@@ -50,3 +51,16 @@ impl Animatable for BackgroundSize {
}
}
}

impl AnimatedValueAsComputed for BackgroundSize {

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

Why doesn't the computed type clamp itself during animation?

use values::computed::Angle as ComputedAngle;
use values::computed::BorderCornerRadius as ComputedBorderCornerRadius;

This comment has been minimized.

Copy link
@nox

nox Jul 19, 2017

Member

Please no, make a submodule border just like there is one in computed and specified.

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

This still isn't addressed, I think.

@nox

This comment has been minimized.

Copy link
Member

commented Jul 19, 2017

We don't change the result of interpolation. All we need to do is make sure we put the correct non-negative result into ServoComputedValues (by uncompute() function), with Animation/Transition cascade level, so we can see the correct animation on the screen.

Why wasn't the computed value clamped itself, then?

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2017

Per our talk on irc, I will try to add new types for those non-negative or greater-than-or-equal-to-one, to avoid additional conditional clamping.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

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

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

I've finished re-writing my patches locally (by introducing some non-negative types and greater-than-or-equal-to-one types, and updating some existing types, e.g. LineHeight), so will update this PR soon.

@BorisChiou

This comment has been minimized.

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/animation/restrictions branch 2 times, most recently from 4f0f67d to e066336 Jul 26, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

r? @nox again. Thanks.
I add some types for NonNegtaive and Greater-than-Or-Equal-To-One values, and then use these types as the specified/computed values of these properties which need to be clamped.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

⌛️ Trying commit e066336 with merge 6a757dd...

bors-servo added a commit that referenced this pull request Jul 26, 2017
Auto merge of #17783 - BorisChiou:stylo/animation/restrictions, r=<try>
stylo: Bug 1374233 - Clamp interpolated values for properties which need to be restricted

Some properties only accept non-negative values, or values greater than or equal to one. It is possible to produce an negative interpolated values while using negative timing functions, so we have to apply a restriction to these values to avoid getting invalid values.

For example, line-height must be non-negative, but the output progress of some timing functions (e,g. cubic-bezier(0.25, -2, 0.75, 1)) may be a negative value, so the interpolated result of line-height is also negative.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix Bug 1374233.
- [X] These changes do not require tests because we have tests in Gecko side already.

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

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

There are some conflicts now, but I'd like to rebase my patches after making sure the design is accepted or knowing what should I fix.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2017

@emilio, nox is still on PTO, so do you feel comfortable with reviewing these patches?

@emilio
Copy link
Member

left a comment

Looks fine over all. There's a fair amount of boilerplate (disclaimer: I don't know how previous versions of the patch were). Most of it could go away with derive, I think.

I wonder if some kind of NonNegative<T>, etc would be doable.

This also needs a rebase.

@nox does this patch address your comments properly? I'm not aware of the full discussion. I know you're on PTO, but it'd be nice if you could take a quick look. Thanks!

<%def name="impl_simple_wrapper_setter(ident, gecko_ffi_name)">
#[allow(non_snake_case)]
pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
${set_gecko_property(gecko_ffi_name, "v.0")}

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

We have something to implement stuff with conversions. Perhaps we should implement From and remove this mako code instead, wdyt?

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Aug 2, 2017

Author Contributor

Sounds great, I can try it.


#[inline]
fn from_computed_value(computed: &Self::ComputedValue) -> Self {
GreaterThanOrEqualToOneNumber(Number::from_computed_value(&computed.0))

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

Ick, that's a decent amount of boilerplate...

impl Animatable for NonNegativeNumber {
#[inline]
fn add_weighted(&self, other: &Self, self_portion: f64, other_portion: f64) -> Result<Self, ()> {
self.0.add_weighted(&other.0, self_portion, other_portion).map(NonNegativeNumber)

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

So, we're not clamping here, and I think that's the intention, right? If so, some comments about it would be nice.

no_viewport_percentage!(GreaterThanOrEqualToOneNumber);

impl Parse for GreaterThanOrEqualToOneNumber {
fn parse<'i, 't>(context: &ParserContext, input: &mut Parser<'i, 't>) -> Result<Self, ParseError<'i>> {

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

Can we remove Number::parse_at_least_one and Number::parse_non_negative now?

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Aug 2, 2017

Author Contributor

Not all non-negative properties are animated, so I didn't replace the type of all non-negative properties with NonNegativeNumber. I will try to check it.

This comment has been minimized.

Copy link
@BorisChiou

BorisChiou Aug 2, 2017

Author Contributor

OK. I can do it, but if you don't mind, I'd like to remove Number::parse_at_least_one() and Number::parse_non_negative() in a follow-up PR to keep this not too complicated because it seems I have to update more types from Number to NonNegativeNumber.


#[inline]
fn from_animated_value(animated: Self::AnimatedValue) -> Self {
T { horizontal: NonNegativeAu::from_animated_value(animated.horizontal),

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

nit: indentation here is weird.

T {
    horizontal: ...,
    vertical: ...,
}
use values::computed::Angle as ComputedAngle;
use values::computed::BorderCornerRadius as ComputedBorderCornerRadius;

This comment has been minimized.

Copy link
@emilio

emilio Aug 2, 2017

Member

This still isn't addressed, I think.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

NonNegative<T> might be a good way to refactor these patches (in the same PR or others). I'd like to know what nox think.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2017

I just tried to use NonNegative<T> as the type of NonNegativeXXX, and it looks much better because lots of code could be written as template. I'd like to update a newer version by this way. Thanks for the suggestion, Emilio.

Bug 1374233 - Part 1: Add NonNegativeNumber and GreaterThanOrEqualToO…
…neNumber.

NonNegativeNumber: for -moz-box-flex, flex-grow, and flex-shrink.
GreaterThanOrEqualToOneNumber: for stroke-miterlimit.

MozReview-Commit-ID: Kgbt99BPdVA

@BorisChiou BorisChiou force-pushed the BorisChiou:stylo/animation/restrictions branch from e066336 to 8651acd Aug 4, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

gecko try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2db4c2a8d83bd0d837350505cb5046edb029f0c

The test failures on the try is not related to these patches, so the overall status looks good now.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

⌛️ Trying commit 8651acd with merge d1f22c0...

bors-servo added a commit that referenced this pull request Aug 4, 2017
Auto merge of #17783 - BorisChiou:stylo/animation/restrictions, r=<try>
stylo: Bug 1374233 - Clamp interpolated values for properties which need to be restricted

Some properties only accept non-negative values, or values greater than or equal to one. It is possible to produce an negative interpolated values while using negative timing functions, so we have to apply a restriction to these values to avoid getting invalid values.

For example, line-height must be non-negative, but the output progress of some timing functions (e,g. cubic-bezier(0.25, -2, 0.75, 1)) may be a negative value, so the interpolated result of line-height is also negative.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix Bug 1374233.
- [X] These changes do not require tests because we have tests in Gecko side already.

<!-- 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/17783)
<!-- Reviewable:end -->
@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

I rebased the patches and use NonNegative<T> as the boilerplate to remove lots of redundant code.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

@nox

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

r=me if geckotry is green.

(Comments addressed)

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

Thanks for the review. I just push these commits to gecko try again. (Gecko try tree is closed now, so I will put the try link later.)

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

@bors-servo r=nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

📌 Commit 8651acd has been approved by nox

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

⌛️ Testing commit 8651acd with merge 016ea11...

bors-servo added a commit that referenced this pull request Aug 7, 2017
Auto merge of #17783 - BorisChiou:stylo/animation/restrictions, r=nox
stylo: Bug 1374233 - Clamp interpolated values for properties which need to be restricted

Some properties only accept non-negative values, or values greater than or equal to one. It is possible to produce an negative interpolated values while using negative timing functions, so we have to apply a restriction to these values to avoid getting invalid values.

For example, line-height must be non-negative, but the output progress of some timing functions (e,g. cubic-bezier(0.25, -2, 0.75, 1)) may be a negative value, so the interpolated result of line-height is also negative.

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix Bug 1374233.
- [X] These changes do not require tests because we have tests in Gecko side already.

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

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

💔 Test failed - linux-rel-wpt

@jdm

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

That's #17168; you can retry.

@BorisChiou

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2017

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

@bors-servo bors-servo merged commit 8651acd into servo:master Aug 7, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@emilio emilio referenced this pull request Dec 3, 2017
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.