Closed Bug 1576866 Opened 5 years ago Closed 5 years ago

border-image-{outset, slice, width} properties should interpolate smoothly

Categories

(Core :: CSS Transitions and Animations, defect, P3)

70 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

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.

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Component: Layout → CSS Transitions and Animations
Priority: -- → P3

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.

(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,161

Hopefully this is as simple as just changing those animation_value_type declarations. I'll have a look tomorrow.

Cool. Thanks.

(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.

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.

I went to fix the tests for this but am getting blocked by failing assertions from bug 1271788 / bug 1578125.

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.)

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.

(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.

(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).

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.

I don't think those are equivalent.... width: 0 is width: 0px, border-image-outset: 0 is not border-image-outset: 0px...

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

I don't think those are equivalent.... width: 0 is width: 0px, border-image-outset: 0 is not border-image-outset: 0px...

Because it's border-image-outset: 0 0 0 0?

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.

Looks like there is already a Chromium bug for this: https://crbug.com/898203

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.

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 ;)

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!

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.)

(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).

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?)

(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.

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:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fe42b6116fbccec61d36f072ad9ec25b0021bd0&selectedJob=267852049

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.

Anyway, I notice that GenericLengthOrNumber deliberately puts the Number before the Length precisely for this reason so presumably GenericBorderImageSideWidth needs to do the same.

(In reply to Brian Birtles (:birtles) from comment #26)

Anyway, I notice that GenericLengthOrNumber deliberately puts the Number before the Length precisely for this reason so presumably GenericBorderImageSideWidth 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.

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

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: nobody → brian
Status: NEW → ASSIGNED
Pushed by bbirtles.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e643d93373f Fix border-image-outset-interpolation.html to use correct initial value; r=emilio https://hg.mozilla.org/integration/autoland/rev/9e3056eb1c86 Parse '0' as a number for border-image-width; r=emilio https://hg.mozilla.org/integration/autoland/rev/c10306cc1b86 Make various border-image-* properties interpolable; r=emilio
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19213 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot

Guessing this can ride the trains. Feel free to reset status flags and request uplift if not.

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

Attachment

General

Created:
Updated:
Size: