Closed Bug 1374233 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(2 files, 15 obsolete files)

Bug 1374233 - Update w-p-t expectations for accumulation on filter.
59 bytes, text/x-review-board-request
hiro
: review+
Details
Servo PR, #17783
41 bytes, text/x-github-pull-request
Details | Review 
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
See Also: → 1374151
Depends on: 1374151
See Also: 1374151 → 
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
Status: NEW → ASSIGNED
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.
Blocks: 1374151
No longer depends on: 1374151
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 on attachment 8880704 [details]
Bug 1374233 - 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*.
(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.
(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
Flags: needinfo?(nox)
Comment on attachment 8880694 [details]
Bug 1374233 - Part 1: Add NonNegativeNumber and GreaterThanOrEqualToOneNumber.

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)
(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.
(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
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)
Attachment #8880697 - Flags: review?(hikezoe)
Attachment #8880699 - Flags: review?(hikezoe)
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?
(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).
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!
ToAnimationValue trait is in servo repo now, so I will try to merge my implementation into it.
(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. :)
Attachment #8880698 - Attachment is obsolete: true
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.
(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.
Attached file Servo PR, #17783 — 

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8880694 -
        Attachment is obsolete: true

    
  

        Attachment #8880695 -
        Attachment is obsolete: true

    
  

        Attachment #8880697 -
        Attachment is obsolete: true

    
  

        Attachment #8880699 -
        Attachment is obsolete: true

    
  

        Attachment #8880700 -
        Attachment is obsolete: true

    
  

        Attachment #8880696 -
        Attachment is obsolete: true

    
  

        Attachment #8880702 -
        Attachment is obsolete: true

    
  

        Attachment #8880703 -
        Attachment is obsolete: true

    
  

        Attachment #8889791 -
        Attachment is obsolete: true

    
  

        Attachment #8889792 -
        Attachment is obsolete: true

    
  

        Attachment #8889793 -
        Attachment is obsolete: true

    
  

        Attachment #8889794 -
        Attachment is obsolete: true

    
  

        Attachment #8889795 -
        Attachment is obsolete: true
Priority: P1 → --

    
    

    
  
Blocks turning on important regression test, test_transitions_per_property.html, P1.
Priority: -- → P1

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5592422676ca
Update w-p-t expectations for accumulation on filter. r=hiro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5592422676ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
  
Blocks: 1424671

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: