Note: There are a few cases of duplicates in user autocompletion which are being worked on.

stylo: Clamp negative interpolated values for non-negative properties while using negative timing functions

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
ASSIGNED
a month ago
2 days ago

People

(Reporter: boris, Assigned: boris)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments, 2 obsolete attachments)

59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
hiro
: review+
Details | Review
41 bytes, text/x-github-pull-request
Details | Review | Splinter Review
(Assignee)

Description

a month ago
In Gecko, we restrict interpolated results [1] according to the restrictions of this property (i.e. nsCSSProps::ValueRestrictions(aProperty)).
e.g. the restrictions of line-height is CSS_PROPERTY_VALUE_NONNEGATIVE, so we clamp it if we get a negative value in StyleAnimationValue::AddWeighted() [2].

However, Servo backend doesn't clamp the interpolated values, so we got a lot of test failures in test_transitions_per_property.html while using a negative timing function [3], and an assertion [4] during reflow line-height.

[1] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/style/StyleAnimationValue.cpp#550-571

[2] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/style/StyleAnimationValue.cpp#2940

[3] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/style/test/test_transitions_per_property.html#1111

[4] http://searchfox.org/mozilla-central/rev/7cc377ce3f0a569c5c9b362d589705cd4fecc0ac/layout/generic/ReflowInput.cpp#2889
(Assignee)

Updated

a month ago
See Also: → bug 1374151
(Assignee)

Updated

a month ago
Depends on: 1374151
See Also: bug 1374151
(Assignee)

Comment 1

a month ago
At least, we have to clamp negatives for these properties:

1. -moz-box-flex
2. -moz-outline-radius-{*}
3. -moz-tab-size
4. column-count
5. column-gap
6. column-rule-width
7. column-width
8. flex-basis
9. font-size
10. grid-{column|row}-gap
11. line-height
12. max-height
13. min-height
14. outline-width
15. stroke-miterlimit
16. stroke-dasharray
17. stroke-width
18. text-shadow (radius)
19. box-shadow (radius)
20. border-{bottom|top}-{left|right}-radius
(Assignee)

Updated

a month ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

29 days ago
I add a trait and a mako flag to make this more general, instead of implementing an IntermediateXXXX type for each property which should be "non-negative" or "at-least-one". I will clean up my code and let you guys check tomorrow.
(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #2)
> I add a trait and a mako flag to make this more general, instead of
> implementing an IntermediateXXXX type for each property which should be
> "non-negative" or "at-least-one". I will clean up my code and let you guys
> check tomorrow.

Great! It sounds a right way to me. We don't need Intermediate type for properties that can already store out of range values.
(Assignee)

Updated

29 days ago
Blocks: 1374151
No longer depends on: 1374151
Duplicate of this bug: 1374151
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

29 days ago
Attachment #8880704 - Flags: review?(hikezoe)
I am not confident that introducing the new traits and clamp values in the traits.  I imagined the values to be clamped in from_computed_value().

:nox, could you please give us some advises here?  We need to preserve out of range values in AnimationValue and clamp them when we insert the AnimationValue into rule node tree (i.e. when we convert the AnimationValue into PropertyDeclarationBlock). For example, when we calculate interpolation value of opacity property, the interpolated value sometimes exceeds 1.0 or falls below 0.0 if timing function produces extreme large values or negative values. The out of range values are still needed to be preserved during calculating AnimationValue in the case where there are other opacity animations on the top of the first one (i.e for additive or accumulative animations).

boris, I just realized your patch does not include opacity at all? We don't need to care it?

Comment 25

29 days ago
mozreview-review
Comment on attachment 8880704 [details]
Bug 1374233 - Part 9: Update w-p-t expectations for accumulation on filter.

https://reviewboard.mozilla.org/r/152052/#review157012
Attachment #8880704 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> I am not confident that introducing the new traits and clamp values in the
> traits.  I imagined the values to be clamped in from_computed_value().

Gah. I meant that introducing the new traits and clamp values in the traits is *a reasonable way*.
(Assignee)

Comment 27

29 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> I am not confident that introducing the new traits and clamp values in the
> traits.  I imagined the values to be clamped in from_computed_value().
> 
> :nox, could you please give us some advises here?  We need to preserve out
> of range values in AnimationValue and clamp them when we insert the
> AnimationValue into rule node tree (i.e. when we convert the AnimationValue
> into PropertyDeclarationBlock). For example, when we calculate interpolation
> value of opacity property, the interpolated value sometimes exceeds 1.0 or
> falls below 0.0 if timing function produces extreme large values or negative
> values. The out of range values are still needed to be preserved during
> calculating AnimationValue in the case where there are other opacity
> animations on the top of the first one (i.e for additive or accumulative
> animations).
> 
> boris, I just realized your patch does not include opacity at all? We don't
> need to care it?

I only check properties which have test failures in test_transitions_per_property.html. I will double-check opacity.
Yeah, we can easily miss properties if we rely on test cases.  We should check corresponding parsers, if the parser limits its input values we need to clamp AnimationValue as well, I think.
(Assignee)

Comment 29

29 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> Yeah, we can easily miss properties if we rely on test cases.  We should
> check corresponding parsers, if the parser limits its input values we need
> to clamp AnimationValue as well, I think.

Ya, I see. about the implementation. let's wait for nox's suggestion
(Assignee)

Updated

29 days ago
Flags: needinfo?(nox)

Comment 30

29 days ago
mozreview-review
Comment on attachment 8880694 [details]
Bug 1374233 - Part 1: Support restrictions in AnimatedValueAsComputed.

https://reviewboard.mozilla.org/r/152028/#review157014

Just a couple of nits which might not be relevant since it sounds like Hiro has some questions about the overall approach of these patches. I'll come back to review this again later based on what you both decide is the right approach here.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:2708
(Diff revision 1)
> +
> +

I assume this whitespace is not intentional?

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3385
(Diff revision 1)
> +    /// For values at least one.
> +    AtLeastOne

AtLeastOne sounds like a series which has to have at least one value (like '+' in a regular expression). GreaterThanOrEqualToOne?

The comment too might be more clear as "Value must be >= 1".

(And similarly, "Value must be >= 0" for the comment before)

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3403
(Diff revision 1)
> +                if self.is_negative() {
> +                    0
> +                } else {
> +                    self
> +                }

Isn't there a more concise way of writing this?

e.g. `self.max(0)`

::: servo/components/style/properties/helpers/animated_properties.mako.rs:3410
(Diff revision 1)
> +                if self < 1 {
> +                    1
> +                } else {
> +                    self
> +                }

Likewise, couldn't this just be:

  Restrictions::GreaterThanOrEqualToOne => self.max(1)
(Assignee)

Comment 31

29 days ago
(In reply to Brian Birtles (:birtles) from comment #30)
> Comment on attachment 8880694 [details]
> Bug 1374233 - Part 1: Introduce Restrictable and Restrictions.
> 
> https://reviewboard.mozilla.org/r/152028/#review157014
> 
> Just a couple of nits which might not be relevant since it sounds like Hiro
> has some questions about the overall approach of these patches. I'll come
> back to review this again later based on what you both decide is the right
> approach here.

Thanks, Brian. I didn't use max() for i32 because we don't have max() function for i32. It seems only f32 and f64 have max() and min() functions.
(Assignee)

Comment 32

29 days ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #24)
> boris, I just realized your patch does not include opacity at all? We don't
> need to care it?

About opacity, we clamped it in computed style, not during parsing/interpolation (e.g. Servo: [1]). That's why I didn't see the test failures of opacity. I will check other animatable properties which need restrictions (e.g. padding-{*} should also be clamped, but I didn't see its test failures). 

[1] http://searchfox.org/mozilla-central/rev/3291398f10dcbe192fb52e74974b172616c018aa/servo/components/style/values/specified/mod.rs#563
(Assignee)

Updated

29 days ago
Attachment #8880694 - Flags: review?(bbirtles)
Attachment #8880696 - Flags: review?(hikezoe)
Attachment #8880698 - Flags: review?(hikezoe)
Attachment #8880701 - Flags: review?(hikezoe)
Attachment #8880702 - Flags: review?(hikezoe)
Attachment #8880703 - Flags: review?(hikezoe)
(Assignee)

Updated

29 days ago
Attachment #8880697 - Flags: review?(hikezoe)
Attachment #8880699 - Flags: review?(hikezoe)
(Assignee)

Updated

29 days ago
Attachment #8880700 - Flags: review?(hikezoe)
(In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment #31)
> Thanks, Brian. I didn't use max() for i32 because we don't have max()
> function for i32. It seems only f32 and f64 have max() and min() functions.

Can you use std::cmp::max?
(Assignee)

Comment 34

29 days ago
(In reply to Brian Birtles (:birtles) from comment #33)
> (In reply to Boris Chiou [:boris] (away July 3rd ~ July 7th) from comment
> #31)
> > Thanks, Brian. I didn't use max() for i32 because we don't have max()
> > function for i32. It seems only f32 and f64 have max() and min() functions.
> 
> Can you use std::cmp::max?

Yes. Thanks. std::cmp::max could be used for all types with Ord trait (and i32 works obviously).
(Assignee)

Updated

29 days ago
Attachment #8880694 - Flags: review?(hikezoe)
Attachment #8880695 - Flags: review?(hikezoe)
I've been wanting to introduce a ToAnimatedValue trait which is similar to ToComputedValue, not sure if that answers the question, but I'm not even sure I understood it anyway.
Flags: needinfo?(nox)
Thank you nox. ToAnimatedValue trait sounds an attractive and the right thing to do.  Considering bug 1339334, I think we eventually need to convert specified values and AnimationValue and vice versa. I will think about it and discuss with Brian during all hands. Thank you!
I discussed with Brian.  We should introduce ToAnimationValue trait which has two functions that one converts specified values to AnimationValue and the other one converts AnimationValue to specified values. Also I think we can/should drop restriction_type flag for each properties.
I can do that tomorrow for the existing intermediate types we have already, I think, if you want that out of your plate.
That's great to know!  We can add some modifications to the trait to fix this bug. Thank you nox!
(Assignee)

Comment 40

22 days ago
ToAnimationValue trait is in servo repo now, so I will try to merge my implementation into it.
(Assignee)

Comment 41

5 days ago
(In reply to Boris Chiou [:boris] from comment #40)
> ToAnimationValue trait is in servo repo now, so I will try to merge my
> implementation into it.

Just back to work, and start to look at this again. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8880698 - Attachment is obsolete: true
(Assignee)

Updated

2 days ago
Attachment #8880701 - Attachment is obsolete: true
Could you make a Servo PR? I don't want to review it here and I can't really make sense of why you needed changes to #[derive(ToAnimatedValue)] given none of the commits explain it.
(Assignee)

Comment 52

2 days ago
(In reply to Anthony Ramine [:nox] from comment #51)
> Could you make a Servo PR? I don't want to review it here and I can't really
> make sense of why you needed changes to #[derive(ToAnimatedValue)] given
> none of the commits explain it.

OK, I will put make a Servo PR for the patches on Servo side.

For SimpleShadow and Filter, only some components needs to be clamped, so it seems I have to implement ToAnimatedValue manually. (I'm not familiar with custom derive. If possible, I'd like to use #[derive(ToAnimatedValue)])

Thanks for the suggestion.
I think I just shouldn't have looked at your patches when I just woke up, makes more sense now that I drank some coffee.

I have a few remarks but I think it looks pretty ok, will comment in the PR.
(Assignee)

Comment 54

2 days ago
Created attachment 8887859 [details] [review]
Servo PR, #17783
You need to log in before you can comment on or make changes to this bug.