Bug 204580 - [css-grid] Support transitions/animations on grid-template-columns|rows
Summary: [css-grid] Support transitions/animations on grid-template-columns|rows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Woodrow
URL:
Keywords: InRadar
Depends on: 202259 226174 240620
Blocks: 235794 235795
  Show dependency treegraph
 
Reported: 2019-11-25 07:56 PST by Manuel Rego Casasnovas
Modified: 2022-05-18 20:03 PDT (History)
18 users (show)

See Also:


Attachments
Patch (140.54 KB, patch)
2022-04-04 15:23 PDT, Matt Woodrow
graouts: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (140.58 KB, patch)
2022-04-05 13:52 PDT, Matt Woodrow
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2019-11-25 07:56:25 PST
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.
Comment 1 Oriol Brufau 2021-03-09 08:22:34 PST
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).
Comment 2 Antoine Quint 2022-01-28 13:11:38 PST
Any update on this? This is blocking bug 235794 and bug 235795.
Comment 3 Radar WebKit Bug Importer 2022-03-16 19:47:25 PDT
<rdar://problem/90406140>
Comment 4 Matt Woodrow 2022-04-04 15:23:38 PDT
Created attachment 456639 [details]
Patch
Comment 5 Antoine Quint 2022-04-05 01:17:08 PDT
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?
Comment 6 Matt Woodrow 2022-04-05 13:50:08 PDT
(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!
Comment 7 Matt Woodrow 2022-04-05 13:52:20 PDT
Created attachment 456740 [details]
Patch for landing
Comment 8 EWS 2022-04-05 15:32:51 PDT
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].
Comment 9 Kiet Ho 2022-05-17 23:08:52 PDT
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)
Comment 10 Matt Woodrow 2022-05-18 13:45:33 PDT
(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.