border-image-{outset, slice, width} properties should interpolate smoothly
Categories
(Core :: CSS Transitions and Animations, defect, P3)
Tracking
()
People
(Reporter: smcgruer, Assigned: birtles)
Details
Attachments
(3 files)
User Agent: Mozilla/5.0 (X11; CrOS x86_64 12105.100.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/75.0.3770.144 Safari/537.36
Steps to reproduce:
See https://output.jsbin.com/vorejak for reproduction. Firefox appears to animate border-image-outset, border-image-slice, and border-image-width discretely. All three properties are spec'd with smooth interpolation:
https://drafts.csswg.org/css-backgrounds-3/#border-image-outset
https://drafts.csswg.org/css-backgrounds-3/#border-image-slice
https://drafts.csswg.org/css-backgrounds-3/#border-image-width
In Firefox Nightly 70, the output of the reproduction is:
border-image-outset
At 0.25, expected 1px, got 0px
At 0.75, expected 3px, got 4px
border-image-slice
At 0.25, expected 10%, got 0%
At 0.75, expected 30%, got 40%
border-image-width
At 0.25, expected 10px, got 0px
At 0.75, expected 30px, got 40px
In Chrome stable 75, it is:
border-image-outset
At 0.25, expected 1px, got 1px
At 0.75, expected 3px, got 3px
border-image-slice
At 0.25, expected 10%, got 10%
At 0.75, expected 30%, got 30%
border-image-width
At 0.25, expected 10px, got 10px
At 0.75, expected 30px, got 30px
Note: This bug was found as a result of mass-porting interpolation tests from Chromium's internal web_tests to the shared WPT suite. It is possible the actual bug here is with Chromium - though in this case I doubt it since the spec says they are interpolable.
Comment 1•5 years ago
|
||
Reproducible on Firefox Nightly 70.0a1, Firefox 69.0b16 and on Firefox 68.0.2 on Windows 10 x64, Mac OS X 10.14 and on Ubuntu 18.04 x64.
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
These three are indeed marked as discrete in Gecko:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/servo/components/style/properties/longhands/border.mako.rs#129,150,161
Hopefully this is as simple as just changing those animation_value_type
declarations. I'll have a look tomorrow.
Comment 3•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #2)
These three are indeed marked as discrete in Gecko:
https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/servo/components/style/properties/longhands/border.mako.rs#129,150,161Hopefully this is as simple as just changing those
animation_value_type
declarations. I'll have a look tomorrow.
Cool. Thanks.
Assignee | ||
Comment 4•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #2)
I'll have a look tomorrow.
Sorry, I didn't get time to look at this today.
Assignee | ||
Comment 5•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf41acfbb9e50efd4fbe539799c62e0148e126b
No idea if this is right.
Comment 6•5 years ago
|
||
IIRC, we need to update a database in devtools directory. I don't recall the place though. We also need to update testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js.
Assignee | ||
Comment 7•5 years ago
|
||
I went to fix the tests for this but am getting blocked by failing assertions from bug 1271788 / bug 1578125.
Assignee | ||
Comment 8•5 years ago
|
||
I wasn't able to test this locally, but for now I pushed some speculative changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=268f7f00385644273b41214db68930cba1f8efaa
(Ignore the redundant commit messages, while pushing ./mach try chooser
had a fit and made a mess of my repo.)
Assignee | ||
Comment 9•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=134253e4572f93531cc7fec0ae00b7ab11a31c1f
This should address most of the issues but it doesn't pass some border-image-outset tests because we don't interpolate the initial unit-less '0' value with lengths. I can't remember where we normally do that coercion.
Comment 10•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #9)
This should address most of the issues but it doesn't pass some border-image-outset tests because we don't interpolate the initial unit-less '0' value with lengths. I can't remember where we normally do that coercion.
Why should it? Numbers are percentages mean different things, right? Where is it specified that you can interpolate some numbers but not others?
Anyhow, you'd need to manually implement the Animate
trait for LengthOrNumber
, which is derived here right now.
Assignee | ||
Comment 11•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)
(In reply to Brian Birtles (:birtles) from comment #9)
This should address most of the issues but it doesn't pass some border-image-outset tests because we don't interpolate the initial unit-less '0' value with lengths. I can't remember where we normally do that coercion.
Why should it? Numbers are percentages mean different things, right? Where is it specified that you can interpolate some numbers but not others?
I thought zero was special since '0' was allowed to represent a length (even though it is parsed as a number in contexts that allow both) and we allow interpolating between '0' and '5px' (example) but I guess we only do that in contexts that take a length (but not a number).
Assignee | ||
Comment 12•5 years ago
|
||
Given that we allow interpolating between width: 0
and width: 100px
, I guess we probably should allow interpolating between border-image-outset: 0
and border-image-outset: 100px
. The reason being that if a property's type was extended from<length>
to <length> | <number>
we wouldn't want the first case (unitless-zero ⇔ length) to stop working.
Comment 13•5 years ago
|
||
I don't think those are equivalent.... width: 0
is width: 0px
, border-image-outset: 0
is not border-image-outset: 0px
...
Assignee | ||
Comment 14•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)
I don't think those are equivalent....
width: 0
iswidth: 0px
,border-image-outset: 0
is notborder-image-outset: 0px
...
Because it's border-image-outset: 0 0 0 0
?
Assignee | ||
Comment 15•5 years ago
|
||
Just spoke to Cameron about this and I think I agree we should not allow interpolating between '0' and a length in this case.
Looking at the Chrome behavior, it doesn't either. However, it seems Chrome is using an initial value of '0px' instead of '0' so I think the test might be in error here.
Assignee | ||
Comment 16•5 years ago
|
||
Looks like there is already a Chromium bug for this: https://crbug.com/898203
Reporter | ||
Comment 17•5 years ago
|
||
It's very possible the test is wrong; this comes from a porting job I am doing from Chromium's internal test suite and we are finding multiple cases where we have incorrectly written tests to our implementation rather than the spec (which is awesome, I'm so excited to find these interop failures and get to address them! :D)
The underlying test is https://wpt.fyi/results/css/css-backgrounds/animations/border-image-outset-interpolation.html?label=experimental&label=master&aligned, and I'm happy to own fixing the initial value test case if we determine if it is against spec.
Assignee | ||
Comment 18•5 years ago
|
||
Thanks Stephen!
Yes, I'm confident that initial value test case is against the spec and once Blink bug 898203 is fixed, it will start failing in Chrome too. If you don't mind, I'll just fix the test as part of this bug. That means the test will start failing in Chrome once the merge to/from WPT completes, but hopefully that provides extra incentive for bug 898203 ;)
Reporter | ||
Comment 19•5 years ago
|
||
If you don't mind, I'll just fix the test as part of this bug
Someone else do work for me? I don't mind at all :D. Thanks!
Assignee | ||
Comment 20•5 years ago
|
||
I was debugging a few failures in border-image-slice on Windows and oddly they only occur for CSS animations / CSS transitions.
I'm not sure of the details but it appears to come about because the test harness uses an animation/transition duration of '2e9s' and delay of '-1e8s' and on Windows that is sometimes ending up being recognized as an infinite time (since our TimeStamp
implementation code differs there). If I simply change those times to 2e8s
and -1e8s
it passes.
I don't think we should be using these values for all our interpolation tests. I don't see anything in CSS values requiring this range.
For CSS animations we should just pause the animation to avoid any progress. For CSS transitions, I'm not sure, but we we might be able to adapt createEasing
to always produce an appropriate step function.
Alternatively we could just reduce the times to 2e8s
and -1e8s
and hope it doesn't introduce intermittents anywhere.
(That said, I'm not sure why we don't have more failures than we do given this.)
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #20)
For CSS transitions, I'm not sure, but we we might be able to adapt
createEasing
to always produce an appropriate step function.
Specifically, I imagine we could make a step timing function with 1000 steps, set an animation duration of 100,000s and still have 100s per step without having a duration as long as 2e9s = 2,000,000,000. That would give a precision for offsets up to three decimal places which is probably enough for all the existing tests (and we could probably add a check for when it is not).
Reporter | ||
Comment 22•5 years ago
|
||
Specifically, I imagine we could make a step timing function with 1000 steps, set an animation duration of 100,000s and still have 100s per step without having a duration as long as 2e9s = 2,000,000,000. That would give a precision for offsets up to three decimal places which is probably enough for all the existing tests (and we could probably add a check for when it is not).
SGTM, assuming it works and is stable enough against time changing. You would then just use different delays to produce the different progress values? (e.g. transition-delay: -2.5e4s for 0.25 progress?)
Assignee | ||
Comment 23•5 years ago
|
||
(In reply to smcgruer from comment #22)
Specifically, I imagine we could make a step timing function with 1000 steps, set an animation duration of 100,000s and still have 100s per step without having a duration as long as 2e9s = 2,000,000,000. That would give a precision for offsets up to three decimal places which is probably enough for all the existing tests (and we could probably add a check for when it is not).
SGTM, assuming it works and is stable enough against time changing. You would then just use different delays to produce the different progress values? (e.g. transition-delay: -2.5e4s for 0.25 progress?)
I've actually gone with a different set of changes over in bug 1578125.
The current set of changes looks like this: https://hg.mozilla.org/try/rev/4d170573b5d584717b822da24067147ab6d77a7e
But to that I need to also make Web Animations use a duration of 100s and seek to 50s to avoid floating-point error on some Windows architectures.
Assignee | ||
Comment 24•5 years ago
|
||
Looks like our parsing for border-image-width
is wrong.
$0.style.borderImageWidth = '0';
console.log(getComputedStyle($0).borderImageWidth);
// --> Gives "0px"
But border-image-width
can take a <number>
and CSS & Values and Units has:
However, if a 0 could be parsed as either a <number> or a <length> in a property (such as line-height), it must parse as a <number>.
That's causing some of the interpolation tests on these patches to fail:
Assignee | ||
Comment 25•5 years ago
|
||
And now searchfox is broken. If I navigate to:
https://searchfox.org/mozilla-central/source/servo/components/style/values/generics/border.rs#27
in either Firefox or Chrome, it drops the searchbar and scrollbar... odd.
Assignee | ||
Comment 26•5 years ago
|
||
Anyway, I notice that GenericLengthOrNumber
deliberately puts the Number
before the Length
precisely for this reason so presumably GenericBorderImageSideWidth
needs to do the same.
Comment 27•5 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #26)
Anyway, I notice that
GenericLengthOrNumber
deliberately puts theNumber
before theLength
precisely for this reason so presumablyGenericBorderImageSideWidth
needs to do the same.
Yeah, that's the right fix.
(In reply to Brian Birtles (:birtles) from comment #25)
And now searchfox is broken. [..]
I filed bug 1583031 for that. I believe this is a regression from bug 1556234.
Assignee | ||
Comment 28•5 years ago
|
||
This test assumes that the initial value of border-image-outset
is '0px' but it is '0'.
'0' does not interpolate with '2px' in a <number> | <length> context since it is treated
as a <number> per spec.
This issue arose because Blink treats the initial value of border-image-outset
as '0'.
This is a known bug: https://crbug.com/898203
Assignee | ||
Comment 29•5 years ago
|
||
As per CSS Values & Units:
"However, if a 0 could be parsed as either a <number> or a <length> in a
property (such as line-height), it must parse as a <number>."
(https://drafts.csswg.org/css-values-4/#lengths)
Depends on D46722
Assignee | ||
Comment 30•5 years ago
|
||
Depends on D46723
Assignee | ||
Updated•5 years ago
|
Comment 31•5 years ago
|
||
Comment 34•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9e643d93373f
https://hg.mozilla.org/mozilla-central/rev/9e3056eb1c86
https://hg.mozilla.org/mozilla-central/rev/c10306cc1b86
Comment 36•5 years ago
|
||
Guessing this can ride the trains. Feel free to reset status flags and request uplift if not.
Description
•