Closed
Bug 1356941
Opened 8 years ago
Closed 8 years ago
stylo: Need a way to store/interpolate colors that RGBA component values exceed 255
Categories
(Core :: CSS Parsing and Computation, enhancement, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(3 files, 13 obsolete files)
While calculating interpolation value of colors, we shouldn't clamp each components, we should clamp the component values right before we insert animated values into cascade.
Assignee | ||
Comment 1•8 years ago
|
||
The basic idea what I am thinking is that
* Introduce IntermediateRGBA struct to store out of range component values
* Add animation_value_type option to Longhand something like this: animation_value_type="IntermediateRGBA"
* Specify the animation_value_type for properties that need color animation
* Use specified animation_value_type in AnimationValue instead of computed_value::T
* Implement From traits to convert (clamp) IntermediateRGBA to RGBA
* Use the From traits when we need to uncompute or some other functions
* Introduce some new structs which have IntermediateRGBA for some properties (e.g. box-shadow)
The reason why I did not re-use animation_type as the 'animation_value_type' is that if we re-use it, expected animation_type values are something like this:
["none", "normal", "discrete", "IntermediateRGBA"]
This is something weird. If we decided to use CamelCase for the animation_type, 'none' becomes 'None', it misleads to 'None' value of Option. Anyway, this way might conflict what Brian is going to do for SMIL, any feedback would be appreciated
A try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d88c6670fb61977ce1fd44ca747010f6ad59dd58
Comment 2•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)
> The reason why I did not re-use animation_type as the 'animation_value_type'
> is that if we re-use it, expected animation_type values are something like
> this:
>
> ["none", "normal", "discrete", "IntermediateRGBA"]
Why not just rename "normal" to "ComputedValue"? (And possibly rename animation_type to animation_value_type)
That seems better than having two partially overlapping properties?
Assignee | ||
Comment 3•8 years ago
|
||
You mean ["none", "discrete", "ComputedValue", "IntermediateRGBA"]?
Actually in the patch set in the try, I added:
"IntermediateColor", "Either<IntermediateColor, Auto>", "IntermediateBoxShadowList", "IntermediateTextShadowList"
We are now checking specifed animation_types in data.py [1], we will check only 'none' and 'discrete' there?
Currently I did add a check that animation_value_type is specified only if animation_type is 'normal'.
Anyway I don't have any strong preference for this, so I am OK with the change.
[1] https://hg.mozilla.org/mozilla-central/file/a374c3546993/servo/components/style/properties/data.py#l172
[2] https://hg.mozilla.org/try/rev/5970471c32860c1d2df7bf4295e82871fc4f0990#l1.32
Comment 4•8 years ago
|
||
Yes, one property seems preferable to two. Adding a separate a animation_value_type which only works with one type of animation_type seems to introduce another possibility for error.
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 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.
https://reviewboard.mozilla.org/r/131130/#review135534
::: commit-message-48c02:5
(Diff revision 1)
> +Bug 1356941 - Introduce IntermediateRGBA type. r?birtles
> +
> +We need to store color values that each components exceeds its normal range
> +(i.e. [0, 1]) during animation operations (e.g. interpolation). We clamp it
> +right before we insert animated values into cascade.
(Nit: into the cascade)
::: servo/components/style/properties/helpers/animated_properties.mako.rs:1979
(Diff revision 1)
> }
> }
> +
> +#[derive(Copy, Clone, Debug, PartialEq)]
> +#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
> +/// Unlike RGBA, each component value may exceeed its range [0.0, 1.0].
s/exceeed/exceed/
(Nit: s/exceeed its range/exceed the range/ might be more clear.)
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2009
(Diff revision 1)
> + let alpha = try!(self.alpha.interpolate(&other.alpha, progress));
> + if alpha == 0. {
> + Ok(IntermediateRGBA::transparent())
I'm curious about whether or not this is right. It seems to roughly match the behavior of AddWeightedColors in StyleAnimationValue and it makes sense because we can't divide by zero later on, but if you interpolate between transparent red and transparent red, i.e. { 1., 0., 0., 0. } and { 1., 0., 0., 0. }, then it seems like the result should be transparent red? But I guess that so long as we use premulitiplied alpha that won't happen?
If that's the case, then I guess we just need a comment, but I'm not sure exactly what it is.
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8859102 [details]
Bug 1356941 - Rename animation_type to animation_value_type.
https://reviewboard.mozilla.org/r/131132/#review135538
Attachment #8859102 -
Flags: review?(bbirtles) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8859103 [details]
Bug 1356941 - Check animation_value_type is not 'discrete' instead of 'normal' for ComputeDistance.
https://reviewboard.mozilla.org/r/131134/#review135540
Attachment #8859103 -
Flags: review?(bbirtles) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8859105 [details]
Bug 1356941 - Introduce IntermediateColor to store currentcolor or IntermeditateRGBA.
https://reviewboard.mozilla.org/r/131138/#review135546
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2077
(Diff revision 1)
> +impl Interpolate for IntermediateColor {
> + #[inline]
> + fn interpolate(&self, other: &Self, progress: f64) -> Result<Self, ()> {
> + match (*self, *other) {
> + (IntermediateColor::IntermediateRGBA(ref this), IntermediateColor::IntermediateRGBA(ref other)) => {
> + this.interpolate(other, progress).map(IntermediateColor::IntermediateRGBA)
> + }
> + _ => Err(()),
> + }
Are we going to implement interpolation of currentColor in a different bug? Do we need to put a comment here?
Attachment #8859105 -
Flags: review?(bbirtles) → review+
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8859104 [details]
Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation.
https://reviewboard.mozilla.org/r/131136/#review135544
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2001
(Diff revision 1)
> +impl <'a> From<<&'a IntermediateRGBA> for RGBA {
> + fn from(extended_rgba: &IntermediateRGBA) -> RGBA {
> + RGBA::from_floats(extended_rgba.red,
> + extended_rgba.green,
> + extended_rgba.blue,
> + extended_rgba.alpha)
> + }
> +}
Should we add a comment here saying that we will clamp these values later and explaining why we don't do that here? (I haven't got to that part of the patch series yet so I'm not sure what the reason is yet.)
Attachment #8859104 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.
https://reviewboard.mozilla.org/r/131130/#review135534
> I'm curious about whether or not this is right. It seems to roughly match the behavior of AddWeightedColors in StyleAnimationValue and it makes sense because we can't divide by zero later on, but if you interpolate between transparent red and transparent red, i.e. { 1., 0., 0., 0. } and { 1., 0., 0., 0. }, then it seems like the result should be transparent red? But I guess that so long as we use premulitiplied alpha that won't happen?
>
> If that's the case, then I guess we just need a comment, but I'm not sure exactly what it is.
This is somewhat related a bug I filed before (I couldn't find the bug though). I prefer to use red transparent { 1., 0., 0., 0. } for this case but, yes this is what gecko does. I will add a comment there.
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8859106 [details]
Bug 1356941 - Use IntermediateColor for caret-color.
https://reviewboard.mozilla.org/r/131140/#review135566
Attachment #8859106 -
Flags: review?(bbirtles) → review+
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8859107 [details]
Bug 1356941 - Add box-shadow interpolation tests.
https://reviewboard.mozilla.org/r/131142/#review135568
::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:943
(Diff revision 1)
> + test(function(t) {
> + var idlName = propertyToIDL(property);
> + var target = createTestElement(t, setup);
> + var animation =
> + target.animate({ [idlName]: [ 'none',
> + 'rgb(100, 100, 100) 10px 10px 10px 0px'] },
> + { duration: 1000, fill: 'both' });
> + testAnimationSamples(animation, idlName,
> + // Premultiplied
> + [{ time: 500, expected: 'rgba(100, 100, 100, 0.5) 5px 5px 5px 0px' }]);
> + }, property + ': none');
Is it worth testing the opposite combination too? i.e. something -> none ?
::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-types.js:980
(Diff revision 1)
> + test(function(t) {
> + var idlName = propertyToIDL(property);
> + var target = createTestElement(t, setup);
> + var animation =
> + target.animate({ [idlName]: [ 'rgb(200, 200, 200) 20px 20px 20px 20px',
> + 'rgb(100, 100, 100) 10px 10px 10px 0px, '
> + + 'rgb(100, 100, 100) 10px 10px 10px 0px'] },
> + { duration: 1000, fill: 'both' });
> + testAnimationSamples(animation, idlName,
> + [{ time: 500, expected: 'rgb(150, 150, 150) 15px 15px 15px 10px, '
> + + 'rgba(100, 100, 100, 0.5) 5px 5px 5px 0px' }]);
> + }, property + ': shadow list (mismatched list length)');
Likewise, is it worth testing the opposite combinatio here too?
i.e. shorter list -> longer list?
Attachment #8859107 -
Flags: review?(bbirtles) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8859108 [details]
Bug 1356941 - Add text-shadow interpolation tests.
https://reviewboard.mozilla.org/r/131144/#review135570
Looks good but if you decide to add the extra tests I mentioned for the previous patch them we should add them here as well.
Attachment #8859108 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 25•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8859104 [details]
Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation.
https://reviewboard.mozilla.org/r/131136/#review135544
> Should we add a comment here saying that we will clamp these values later and explaining why we don't do that here? (I haven't got to that part of the patch series yet so I'm not sure what the reason is yet.)
Actually this is the place that we clamp the component values. This From trait is used when we uncompute AnimationValue (from_floats clamps each components values). I will add a comment here.
Assignee | ||
Comment 26•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8859105 [details]
Bug 1356941 - Introduce IntermediateColor to store currentcolor or IntermeditateRGBA.
https://reviewboard.mozilla.org/r/131138/#review135546
> Are we going to implement interpolation of currentColor in a different bug? Do we need to put a comment here?
Yeah, I think it will be handled in bug 1345709 (or followup?).
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8859109 [details]
Bug 1356941 - Introduce a macro to implement Interpolate trait for box-shadow and text-shadow.
https://reviewboard.mozilla.org/r/131146/#review135576
::: servo/components/style/properties/helpers/animated_properties.mako.rs:949
(Diff revision 1)
> - Ok(TextShadow {
> - offset_x: try!(self.offset_x.interpolate(&other.offset_x, progress)),
> - offset_y: try!(self.offset_y.interpolate(&other.offset_y, progress)),
> + % if "Box" in item:
> + if self.inset != other.inset {
> + return Err(());
> - blur_radius: try!(self.blur_radius.interpolate(&other.blur_radius, progress)),
> - color: try!(self.color.interpolate(&other.color, progress)),
> - })
> - }
> -}
> + }
> + % endif
Can we add a comment explaining this part?
Attachment #8859109 -
Flags: review?(bbirtles) → review+
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8859110 [details]
Bug 1356941 - Use IntermediateColor for box-shadow.
https://reviewboard.mozilla.org/r/131148/#review135578
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2131
(Diff revision 1)
> + #[allow(missing_docs)]
> + pub struct Intermediate${type}Shadow {
Should we add docs for this? A one sentence description would be fine.
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2145
(Diff revision 1)
> + #[allow(missing_docs)]
> + pub struct Intermediate${type}ShadowList(pub Vec<Intermediate${type}Shadow>);
Likewise, should we add docs for this?
Attachment #8859110 -
Flags: review?(bbirtles) → review+
Comment 29•8 years ago
|
||
mozreview-review |
Comment on attachment 8859111 [details]
Bug 1356941 - Use IntermediateColor for text-shadow.
https://reviewboard.mozilla.org/r/131150/#review135580
Attachment #8859111 -
Flags: review?(bbirtles) → review+
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8859101 [details]
Bug 1356941 - Introduce IntermediateRGBA type.
https://reviewboard.mozilla.org/r/131130/#review135582
::: servo/components/style/properties/helpers/animated_properties.mako.rs:2009
(Diff revision 1)
> + let alpha = try!(self.alpha.interpolate(&other.alpha, progress));
> + if alpha == 0. {
> + Ok(IntermediateRGBA::transparent())
Ok, r=me once we have a suitable comment explaining this behavior.
Attachment #8859101 -
Flags: review?(bbirtles) → review+
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 47•8 years ago
|
||
mozreview-review |
Comment on attachment 8859103 [details]
Bug 1356941 - Check animation_value_type is not 'discrete' instead of 'normal' for ComputeDistance.
https://reviewboard.mozilla.org/r/131134/#review135604
Attachment #8859103 -
Flags: review?(boris.chiou) → review+
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 64•8 years ago
|
||
mozreview-review |
Comment on attachment 8860807 [details]
Bug 1356941 ComputedDistance for IntermediateRGBA.
https://reviewboard.mozilla.org/r/132780/#review135610
Attachment #8860807 -
Flags: review?(boris.chiou) → review+
Comment 65•8 years ago
|
||
mozreview-review |
Comment on attachment 8860808 [details]
Bug 1356941 - ComputedDistance for IntermediateColor.
https://reviewboard.mozilla.org/r/132782/#review135614
Attachment #8860808 -
Flags: review?(boris.chiou) → review+
Assignee | ||
Comment 66•8 years ago
|
||
mozreview-review |
Comment on attachment 8860806 [details]
Bug 1356941 - Rename 'normal' of animation_value_type to 'ComputedValue'.
https://reviewboard.mozilla.org/r/132778/#review135612
Hey MozReview, why do you think this patch is needed to be reviwed again?
Comment 67•8 years ago
|
||
mozreview-review |
Comment on attachment 8860809 [details]
Bug 1356941 - ComputedDistance for IntermediateBoxShadow.
https://reviewboard.mozilla.org/r/132784/#review135620
Attachment #8860809 -
Flags: review?(boris.chiou) → review+
Comment 68•8 years ago
|
||
mozreview-review |
Comment on attachment 8860806 [details]
Bug 1356941 - Rename 'normal' of animation_value_type to 'ComputedValue'.
https://reviewboard.mozilla.org/r/132778/#review135618
I'm pretty sure I already reviewed this. I guess MozReview got confused again.
Attachment #8860806 -
Flags: review?(bbirtles) → review+
Comment 69•8 years ago
|
||
mozreview-review |
Comment on attachment 8860810 [details]
Bug 1356941 - ComputedDistance for IntermediateTextShadow.
https://reviewboard.mozilla.org/r/132786/#review135622
Attachment #8860810 -
Flags: review?(boris.chiou) → review+
Assignee | ||
Comment 70•8 years ago
|
||
Assignee | ||
Comment 71•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8859101 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859102 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859103 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8860806 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859104 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8860807 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859105 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8860808 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859109 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859110 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8860809 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8859111 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8860810 -
Attachment is obsolete: true
Comment 75•8 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3b8389a53ca
Use IntermediateColor for caret-color. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5714c00effca
Add box-shadow interpolation tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d1b02c96cec5
Add text-shadow interpolation tests. r=birtles
Comment 76•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3b8389a53ca
https://hg.mozilla.org/mozilla-central/rev/5714c00effca
https://hg.mozilla.org/mozilla-central/rev/d1b02c96cec5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•