Bug 204580 – [css-grid] Support transitions/animations on grid-template-columns|rows

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Manuel Rego Casasnovas]

Transitions/animations on grid-template-columns|rows are not working on WebKit, neither on Chromium.
They only work on Firefox so far.
At some point we should add support for that on WebKit too.

There are a bunch of tests under http://w3c-test.org/css/css-grid/animation/ regarding that.

[Oriol Brufau]

It's worth noting that repeat() needs to be handled carefully:
> If two repeat() notations that have the same first argument (repetition count) and the same number of tracks in their second argument (the track listing), they are combined by combining each component of their computed track lists by computed value (just like combining a top-level track list). They otherwise combine discretely.
But this is not currently possible, since non-auto repeat() is expanded at computed-value time (bug 202259).

[Antoine Quint]

Any update on this? This is blocking bug 235794 and bug 235795.

[Radar WebKit Bug Importer]

<rdar://problem/90406140>

[Matt Woodrow]

Created attachment 456639 [details]
Patch

[Antoine Quint]

Comment on attachment 456639 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456639&action=review

So glad this is being done!

> Source/WebCore/animation/CSSPropertyAnimation.cpp:662
> +        GridLength length = blendFunc(from.minTrackBreadth(), to.minTrackBreadth(), context);

Can you use `auto` here?

> Source/WebCore/animation/CSSPropertyAnimation.cpp:667
> +        GridLength minTrackBreadth = blendFunc(from.minTrackBreadth(), to.minTrackBreadth(), context);
> +        GridLength maxTrackBreadth = blendFunc(from.maxTrackBreadth(), to.maxTrackBreadth(), context);

Maybe use `auto` here as well.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:671
> +    GridLength fitContentBreadth = blendFunc(from.fitContentTrackBreadth(), to.fitContentTrackBreadth(), context);

And here too.

> Source/WebCore/animation/CSSPropertyAnimation.cpp:894
> +        return WebCore::canInterpolate(this->value(from), this->value(to));

Why not have the body of that function here rather than using a static function?

[Matt Woodrow]

(In reply to Antoine Quint from comment #5) 
> > Source/WebCore/animation/CSSPropertyAnimation.cpp:894
> > +        return WebCore::canInterpolate(this->value(from), this->value(to));
> 
> Why not have the body of that function here rather than using a static
> function?

It has two callers, the blendFunc implementation also uses it to check if we're able to compare the two lists piece-wise.

Thanks for the review!

[Matt Woodrow]

Created attachment 456740 [details]
Patch for landing

[EWS]

Committed r292432 (249291@main): <https://commits.webkit.org/249291@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456740 [details].

[Kiet Ho]

In canInterpolate, there's dead code (marked with ">") in the GridTrackEntryAutoRepeat visitor:

    auto visitor = WTF::makeVisitor([&](const GridTrackSize&) {
        return std::holds_alternative<GridTrackSize>(to[i]);
    }, [&](const Vector<String>&) {
        return std::holds_alternative<Vector<String>>(to[i]);
    }, [&](const GridTrackEntryRepeat& repeat) {
        if (!std::holds_alternative<GridTrackEntryRepeat>(to[i]))
            return false;
        const auto& toEntry = std::get<GridTrackEntryRepeat>(to[i]);
        return repeat.repeats == toEntry.repeats && repeat.list.size() == toEntry.list.size();
    }, [&](const GridTrackEntryAutoRepeat& repeat) {
        return false;
>       if (!std::holds_alternative<GridTrackEntryAutoRepeat>(to[i]))
>           return false;
>       const auto& toEntry = std::get<GridTrackEntryAutoRepeat>(to[i]);
>       return repeat.type == toEntry.type && repeat.list.size() == toEntry.list.size();
    }, [&](const GridTrackEntrySubgrid&) {
        return false;
    });

I suppose those can be safely removed, because repeat() with auto is not interpolable so the visitor can just return false? (sorry in advance if I'm wrong, I read the spec just now)

[Matt Woodrow]

(In reply to Kiet Ho from comment #9)
> 
> I suppose those can be safely removed, because repeat() with auto is not
> interpolable so the visitor can just return false? (sorry in advance if I'm
> wrong, I read the spec just now)

Whoops, yes, you're correct. Returning early is the right behaviour (since we can't interpolate repeat(auto)), the rest is code from before I read the spec fully and forgot to remove.