Bug 205434 – Web Inspector: add instrumentation for showing existing Web Animations

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

I'm thinking we expand the scope of the Canvas Tab and make it a Graphics Tab instead, with four different sections:
 - Canvases (horizontally scrollable)
 - Web Animations
 - CSS Animations
 - CSS Transitions

[Devin Rousso]

<rdar://problem/28328087>

[Devin Rousso]

Created attachment 386064 [details]
Patch

Needs tests.  Too tired.  Will write tomorrow :/

[Devin Rousso]

Created attachment 386065 [details]
[Image] After Patch is applied

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 386066 [details]
Patch

Minor dark mode adjustments

[Devin Rousso]

Created attachment 386124 [details]
Patch

Add a Keyframes section to the Animation panel in the details sidebar of the Graphics Tab that contains a CodeMirror for the CSS style of each keyframe

[Devin Rousso]

Created attachment 386149 [details]
Patch

Add tests.\nSend millsecond values from the backend, not seconds.\nFix canvas tests from the changes made to `WI.CanvasManager`.

[Devin Rousso]

(In reply to Devin Rousso from comment #7)
> Created attachment 386149 [details]
> Patch
> 
> Add tests.
> Send millsecond values from the backend, not seconds.
> Fix canvas tests from the changes made to `WI.CanvasManager`.
This should've been formatted better -.-

[Devin Rousso]

Created attachment 386154 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 386176 [details]
Patch

Switch back to a vertical scrolling list for canvases.

[Blaze Burg]

Windows build failure:

C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector\agents\InspectorAnimationAgent.h(63,10): error C3668: 'WebCore::InspectorAnimationAgent::resolveAnimation': method with override specifier 'override' did not override any base class methods (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-6.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]


Mac WK1 test failures:

Regressions: Unexpected text-only failures (2)
  inspector/animation/lifecycle-css-animation.html [ Failure ]
  inspector/animation/lifecycle-css-transition.html [ Failure ]

[Blaze Burg]

Comment on attachment 386176 [details]
Patch

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

Finished reviewing the backend changes. Looks great so far.

> Source/JavaScriptCore/inspector/protocol/Animation.json:44
> +                { "name": "iterations", "type": "number", "optional": true },

Nit: iterationCount?

> Source/JavaScriptCore/inspector/protocol/Animation.json:46
> +                { "name": "iterationDuration", "type": "number", "optional": true },

Please include labels for the units. I assume it's seconds as a double?

> Source/JavaScriptCore/inspector/protocol/Animation.json:47
> +                { "name": "timingFunction", "type": "string", "optional": true },

Is this a free-form string or should it be an enum?

> Source/JavaScriptCore/inspector/protocol/Animation.json:57
> +                { "name": "offset", "type": "number" },

Needs units. Is the range [0,1] or [0, 100]?

> Source/JavaScriptCore/inspector/protocol/Animation.json:59
> +                { "name": "style", "type": "string", "optional": true }

Ditto: are these enums, free-form, or what?

EDIT: this is a CSS rule body text fragment with multiple property names and values. Please state that.

> Source/JavaScriptCore/inspector/protocol/Animation.json:84
> +            "name": "requestNode",

Nit: requestAffectedNode? requestNodeForAnimation? requestNodeForAnimationEffect?

This could probably match the verb of 'resolveAnimation' (which I'm more agreeable to using as-is).

Do all animations have a target? Is it associated with the effect or the animation? I'm not sure what the practical difference is, if Effects do not stack into one Animation.

> Source/JavaScriptCore/inspector/protocol/Animation.json:131
> +            "description": "Dispatched whenever the target of any effect of any animation is changed in any way.",

Since the target is not a field of Animation or Effect, I'm a little confused (without reading onward) as to what this would be used for. Does this indicate that the node that would be resolved via Animation.resolveNode is now different than before?

> Source/WebCore/animation/KeyframeEffect.cpp:1108
> +    m_shouldRunAccelerated = m_blendingKeyframes.size();

I'd do !!m_blendingKeyframes.size(); Just to make it super obvious.

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:108
> +        return document->page() == &m_inspectedPage;

Neat!

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:181
> +    result = injectedScript.wrapObject(value, objectGroupName);

V. nice.

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:354
> +    return seconds.milliseconds();

... I see, it's milliseconds. Why not just seconds? It's a double, it doesn't matter either way except milliseconds are harder to read.

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:380
> +    switch (effect->direction()) {

Since these are 1:1 with protocol enum values, can you extract it to a static helper protocolValueForPlaybackDirection()?

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:396
> +    case FillMode::None:

Ditto (extract it)

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:415
> +

Nit: extra line

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:418
> +        if (is<DeclarativeAnimation>(keyframeEffect.animation())) {

This build method is huge. I think the keyframe payload creation could go into its own helper function, as well as the aforementioned enum conversions.

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:432
> +                    .setOffset(blendingKeyframe.key())

Still not sure what the units are here (for key()/offset)

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:436
> +                    keyframePayload->setEasing(timingFunction->cssText());

I see, it's a CSS timing function (which can be user defined, right?)

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:441
> +                for (auto cssPropertyId : cssPropertyIds) {

Needs a comment: "The frontend expects a CSS rule body per each keyframe. Synthesize CSS style property text for each property of the keyframe."

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:184
> +    int pushNodePathToFrontend(ErrorString, Node*);

I think this change could safely go into another patch, but I won't force it.

[Devin Rousso]

Comment on attachment 386176 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/Animation.json:44
>> +                { "name": "iterations", "type": "number", "optional": true },
> 
> Nit: iterationCount?

That's much better :)

>> Source/JavaScriptCore/inspector/protocol/Animation.json:46
>> +                { "name": "iterationDuration", "type": "number", "optional": true },
> 
> Please include labels for the units. I assume it's seconds as a double?

Milliseconds, as that's what the Web Animations API uses.

>> Source/JavaScriptCore/inspector/protocol/Animation.json:47
>> +                { "name": "timingFunction", "type": "string", "optional": true },
> 
> Is this a free-form string or should it be an enum?

This is a CSS string, like "ease" or "cubic-bezier(...)".  I'll add a description describing it as such.

>> Source/JavaScriptCore/inspector/protocol/Animation.json:57
>> +                { "name": "offset", "type": "number" },
> 
> Needs units. Is the range [0,1] or [0, 100]?

It's a decimal percentage, so [0, 1].

>> Source/JavaScriptCore/inspector/protocol/Animation.json:59
>> +                { "name": "style", "type": "string", "optional": true }
> 
> Ditto: are these enums, free-form, or what?
> 
> EDIT: this is a CSS rule body text fragment with multiple property names and values. Please state that.

Yes, this is a CSS style declaration.

>> Source/JavaScriptCore/inspector/protocol/Animation.json:84
>> +            "name": "requestNode",
> 
> Nit: requestAffectedNode? requestNodeForAnimation? requestNodeForAnimationEffect?
> 
> This could probably match the verb of 'resolveAnimation' (which I'm more agreeable to using as-is).
> 
> Do all animations have a target? Is it associated with the effect or the animation? I'm not sure what the practical difference is, if Effects do not stack into one Animation.

I was trying to match the naming of `Canvas.requestNode`, but the semantics are different enough that we should really call it `requestTarget`.  The target (DOM node) is associated with the effect of the animation, both (the target and the effect) of which are nullable.  It is possible for the target of an animation to change after it has been created, hence the `Animation.targetChanged` event.

>> Source/JavaScriptCore/inspector/protocol/Animation.json:131
>> +            "description": "Dispatched whenever the target of any effect of any animation is changed in any way.",
> 
> Since the target is not a field of Animation or Effect, I'm a little confused (without reading onward) as to what this would be used for. Does this indicate that the node that would be resolved via Animation.resolveNode is now different than before?

Yes.  See above.

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:354
>> +    return seconds.milliseconds();
> 
> ... I see, it's milliseconds. Why not just seconds? It's a double, it doesn't matter either way except milliseconds are harder to read.

This matches the Web Animations API, as all of the duration values are in milliseconds.  I personally would rather use milliseconds as well, as integers (e.g. 400ms) are easier to deal with than decimals (e.g. 0.4s).

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:380
>> +    switch (effect->direction()) {
> 
> Since these are 1:1 with protocol enum values, can you extract it to a static helper protocolValueForPlaybackDirection()?

Sure.

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:396
>> +    case FillMode::None:
> 
> Ditto (extract it)

Ditto sure

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:418
>> +        if (is<DeclarativeAnimation>(keyframeEffect.animation())) {
> 
> This build method is huge. I think the keyframe payload creation could go into its own helper function, as well as the aforementioned enum conversions.

Sounds good

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:436
>> +                    keyframePayload->setEasing(timingFunction->cssText());
> 
> I see, it's a CSS timing function (which can be user defined, right?)

Yes, it is user defined.

>> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:441
>> +                for (auto cssPropertyId : cssPropertyIds) {
> 
> Needs a comment: "The frontend expects a CSS rule body per each keyframe. Synthesize CSS style property text for each property of the keyframe."

Good call.

[Devin Rousso]

Created attachment 386313 [details]
Patch

Addressed Brian's feedback.

[Blaze Burg]

Comment on attachment 386313 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Base/Main.js:178
> +    let canvasIndex = WI._openTabsSetting.value.indexOf("canvas");

Nit: please add an expiration date for this code (Summer 2020?)

> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Nit: 2019

> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:26
> +// FIXME: AnimationManager lacks advanced multi-target support. (Animations per-target)

Here's the problem with renaming the protocol method to Animation.requestTarget: I can't tell whether this comment refers to multiple effect targets or multiple Page targets. Calling it something more verbose like .requestEffectTarget may help reduce this semantic overloading.

> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:326
> +        return this._idToDOMNode[nodeId] || null;

Okay, but do other callers rely on this returning undefined if the lookup fails?

> Source/WebInspectorUI/UserInterface/Images/Graphics.svg:2
> +<!-- Copyright © 2017 Apple Inc. All rights reserved. -->

Nit: 2019

> Source/WebInspectorUI/UserInterface/Main.html:355
> +    <script src="Models/Collection.js"></script>

Nit: ordering is wrong

> Source/WebInspectorUI/UserInterface/Models/Animation.js:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:185
> +    requestTarget()

Oh boy 😵

[Devin Rousso]

Comment on attachment 386313 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/Main.js:178
>> +    let canvasIndex = WI._openTabsSetting.value.indexOf("canvas");
> 
> Nit: please add an expiration date for this code (Summer 2020?)

I think we should keep this for at least a full release, as without this the Canvas Tab could seem to disappear with no replacement for users who haven't used Web Inspector in a while.  I'll make this into a FIXME comment with a bug link.

>> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:2
>> + * Copyright (C) 2017 Apple Inc. All rights reserved.
> 
> Nit: 2019

Oops :P

>> Source/WebInspectorUI/UserInterface/Controllers/AnimationManager.js:26
>> +// FIXME: AnimationManager lacks advanced multi-target support. (Animations per-target)
> 
> Here's the problem with renaming the protocol method to Animation.requestTarget: I can't tell whether this comment refers to multiple effect targets or multiple Page targets. Calling it something more verbose like .requestEffectTarget may help reduce this semantic overloading.

Good point!  `Animation.requestEffectTarget` makes the most sense and is the most technically accurate too.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMManager.js:326
>> +        return this._idToDOMNode[nodeId] || null;
> 
> Okay, but do other callers rely on this returning undefined if the lookup fails?

Not that I could tell.

>> Source/WebInspectorUI/UserInterface/Images/Graphics.svg:2
>> +<!-- Copyright © 2017 Apple Inc. All rights reserved. -->
> 
> Nit: 2019

AFAIU, we only update copyright when making major modifications.  Since this is just a rename, I didn't think it warranted an update, but I don't really care either way so I'll update it.

>> Source/WebInspectorUI/UserInterface/Main.html:355
>> +    <script src="Models/Collection.js"></script>
> 
> Nit: ordering is wrong

Oops :P

>> Source/WebInspectorUI/UserInterface/Models/Animation.js:2
>> + * Copyright (C) 2017 Apple Inc. All rights reserved.
> 
> Ditto.

Oops :P

>> Source/WebInspectorUI/UserInterface/Models/Animation.js:185
>> +    requestTarget()
> 
> Oh boy 😵

Yeah I see how this could be confusing, especially if we added a `get target()` 😅

[Devin Rousso]

Created attachment 386496 [details]
Patch

[Devin Rousso]

(In reply to Brian Burg from comment #11)
> Windows build failure:
> 
> C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\inspector\agents\InspectorAnimationAgent.h(63,10): error C3668: 'WebCore::InspectorAnimationAgent::resolveAnimation': method with override specifier 'override' did not override any base class methods (compiling source file C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\WebCore\unified-sources\UnifiedSource-84c9f43f-6.cpp) [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
I have no idea why this is happening.  It builds just fine on all other platforms.  Perhaps there is something funky with the generated output of InspectorBackendDispatchers.h?  Is there any way for me to see the generated output file?

> Mac WK1 test failures:
> 
> Regressions: Unexpected text-only failures (2)
>   inspector/animation/lifecycle-css-animation.html [ Failure ]
>   inspector/animation/lifecycle-css-transition.html [ Failure ]
Apparently, Web Animations are not used for CSS animations/transitions in WK1.  As such, I've skipped these tests for mac-wk1.

[Antoine Quint]

(In reply to Devin Rousso from comment #18)
> (In reply to Brian Burg from comment #11)
> Apparently, Web Animations are not used for CSS animations/transitions in
> WK1.  As such, I've skipped these tests for mac-wk1.

That shouldn't be the case! I filed https://bugs.webkit.org/show_bug.cgi?id=205791 to address that.

[Antoine Quint]

(In reply to Antoine Quint from comment #19)
> (In reply to Devin Rousso from comment #18)
> > (In reply to Brian Burg from comment #11)
> > Apparently, Web Animations are not used for CSS animations/transitions in
> > WK1.  As such, I've skipped these tests for mac-wk1.
> 
> That shouldn't be the case! I filed
> https://bugs.webkit.org/show_bug.cgi?id=205791 to address that.

And now it's fixed, so you shouldn't have to skip your tests on WK1 anymore.

[Devin Rousso]

Created attachment 386879 [details]
Patch

[Blaze Burg]

Comment on attachment 386879 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/CanvasManager.js:303
> +        this._canvasCollection.clear();

I really like this refactor. It did seem kind of strange for the manager to not have a collection of canvas instances and it was being handled by the tab view class.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:75
> +        case WI.Animation.PlaybackDirection.Normal:

I think these should have the optional description to say they are animation playback directions.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:92
> +        case WI.Animation.FillMode.None:

I think these should have the optional description to say they are animation fill modes.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:187
> +        if (!this._requestEffectTargetPromise) {

As we discussed its a little weird to have an infallible promise. That said, I don't think that there is anything to do in case the AnimationAgent.requestEffectTarget command fails.

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:100
> +            return WI.UIString("Bitmap Renderer");

Definitely needs a comment for localization. "Bitmap Renderer is a type of context associated with a <canvas> element" or something.

> Source/WebInspectorUI/UserInterface/Models/Recording.js:156
> +        case Recording.Type.CanvasBitmapRenderer:

Ditto the localization comment. "Bitmap Renderer (Recording Type)", "Bitmap Renderer", "A type of canvas recording in the Graphics Tab."

> Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:61
> +    handleRefreshButtonClicked()

Per our discussion, this should go away and instead invoke the normal WI.View needsLayout. This isn't really performance sensitive as it only draws once in response to user input.

> Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:100
> +            if (!node || !node.ownerDocument)

Have you checked whether this mouse-driven lookup feels responsive enough when inspecting a device?

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:59
> +    content: "\00A0\2014\00A0"; /* &nbsp;&mdash;&nbsp; */;

Fancy. Does it look correct in RTL layout? We try to do something similar with Source Locations but it needed extra care for RTL.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:68
> +    opacity: 1;

Is this needed given that it also has a huge focus ring when selected? I would expect the background to change on hover but not the border. Not a big deal either way.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

2020, please

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:160
> +

is squeezeYStart related to the markerHead size? I would have guessed squeezeYStart = markerHeadRadius + markerHeadPadding*2

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:162
> +        const adjustEasingY = 0.5;

This looks good.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:191
> +            addTitle(startDelayElement, WI.UIString("Start Delay %s").format(Number.secondsToString(this.representedObject.startDelay / 1000)));

Localizers will need help to find out what this refers to.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:208
> +            addTitle(endDelayElement, WI.UIString("End Delay %s").format(Number.secondsToString(this.representedObject.endDelay / 1000)));

Ditto.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:239
> +                easingPath.setAttribute("d", `M 0 ${height + adjustEasingY} C ${x1} ${y1} ${x2} ${y2} ${width} ${adjustEasingY} V ${height + adjustEasingY} Z`);

🥵

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:279
> +        this._refreshSubtitle();

I can't think of any reason these need to be different than calling .needsLayout(), and it may cut down the number of relayouts if we load a bunch of animations quickly.

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:289
> +                const text = WI.UIString("Selected Animation");

Nit: add localizer note that this is a label for an animation that's logged to console.

[Blaze Burg]

Comment on attachment 386879 [details]
Patch

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

r=me, excellent work. Please address smaller items immediately such as localizer comments, other changes such as moving to .needsLayout should come in a followup to minimize breakage.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:40
> +    {

What happens in the case of multiple animations being selected? Is that not supported?

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:82
> +        this._targetRow = new WI.DetailsSectionSimpleRow(WI.UIString("Target"));

Needs a better localizer comment.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:83
> +        let identitySection = new WI.DetailsSection("animation-identity", WI.UIString("Identity"), [new WI.DetailsSectionGroup([this._nameRow, this._typeRow, this._targetRow])]);

Needs a better localizer comment.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:89
> +        let iterationsGroup = new WI.DetailsSectionGroup([this._iterationCountRow, this._iterationStartRow, this._iterationDurationRow]);

(throughout) needs a better localizer comment.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:112
> +        let backtraceRow = new WI.DetailsSectionRow;

This looks weird in the screenshot. Is each frame clickable? I would have expected more SourceLocation information (file, line number) especially if the frames are not selectable.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:142
> +        this._animation.requestEffectTarget().then((domNode) => {

Is this a good idea under layout()? Seems it will cause another repaint sometime later even if we have the data. I think it would be best to cache the effect inside set animation() or in response to the effecteUpdated event and make this function synchronous.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:163
> +        for (let section of this._codeMirrorSectionMap.keys())

I think it would be clearer to clean up the old elements we don't want at the top of the function.

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:178
> +                subtitle.textContent = ` ${emDash} ${keyframe.easing.toString()}`;

Does this look right in RTL layout?

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:202
> +                createCodeMirrorCubicBezierTextMarkers(codeMirror, range, optionsForType(WI.InlineSwatch.Type.Bezier));

It's kind of a bummer this won't be shown for the easing function since that's in the header. I don't think it needs to be there other than to save space. What about prepending the easing property name+value to the keyframe's style text?

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:86
> +    handleRefreshButtonClicked()

Per previous comment this can just trigger a new layout directly if it's not going to fetch new data.

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:98
> +        if (item) {

Was this just a latent bug or are we now expecting to pass null to set selectedItem()?

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:100
> +            console.assert(contentView, "Missing contet view for item.", item);

Nit: content view

> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:304
> +            this._selectItem(null);

I see, now we deselect the item if the hit test failed.

> Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js:61
> +        if (!WI.animationManager.supported) {

I don't understand. Don't we always want these canvas related navigation items? Or is this supposed to be items from the animation overview content view?

EDIT: Oh, I see, if Animations are not supported then there are no sub-sections in the main content area and [ ] Record First [n] Frames needs to be added to the main bar in that case.

[Devin Rousso]

Comment on attachment 386879 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:100
>> +            if (!node || !node.ownerDocument)
> 
> Have you checked whether this mouse-driven lookup feels responsive enough when inspecting a device?

It's never been a problem with canvases, and they can potentially highlight multiple nodes at once (e.g. `-webkit-canvas` CSS canvas clients).  This is effectively the same logic.

>> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:59
>> +    content: "\00A0\2014\00A0"; /*  —  */;
> 
> Fancy. Does it look correct in RTL layout? We try to do something similar with Source Locations but it needed extra care for RTL.

I tested it using our debug setting, but not changing the system language.  It looked fine with our debug setting.

>> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.css:68
>> +    opacity: 1;
> 
> Is this needed given that it also has a huge focus ring when selected? I would expect the background to change on hover but not the border. Not a big deal either way.

This matches what we do with the navigation bar in the canvas cards.

>> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:160
>> +
> 
> is squeezeYStart related to the markerHead size? I would have guessed squeezeYStart = markerHeadRadius + markerHeadPadding*2

Oh lol good point 😂

>> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:40
>> +    {
> 
> What happens in the case of multiple animations being selected? Is that not supported?

No, that is not supported.  This is just our usual logic for details sidebars ¯\_(ツ)_/¯

>> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:112
>> +        let backtraceRow = new WI.DetailsSectionRow;
> 
> This looks weird in the screenshot. Is each frame clickable? I would have expected more SourceLocation information (file, line number) especially if the frames are not selectable.

Yes, they are all clickable.  The screenshot is a bad example (sorry) because it was created by a Console evaluation (also in the screenshot), not from some script.

>> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:142
>> +        this._animation.requestEffectTarget().then((domNode) => {
> 
> Is this a good idea under layout()? Seems it will cause another repaint sometime later even if we have the data. I think it would be best to cache the effect inside set animation() or in response to the effecteUpdated event and make this function synchronous.

The main reason why we use the `Promise` approach is that something in the UI has to be the first caller, as the effect target is never sent to the frontend.  I don't want to eagerly fetch the effect target, as we may not need that info (such as if the Graphics Tab isn't even visible).  A better solution might be to not use a `Promise`, and instead take a callback function directly so that it can be called synchronously if the value is already there.  That way, the majority of calls won't trigger another layout, but some still might instead of all.

>> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:178
>> +                subtitle.textContent = ` ${emDash} ${keyframe.easing.toString()}`;
> 
> Does this look right in RTL layout?

It looks fine to me using the debug setting.

>> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:202
>> +                createCodeMirrorCubicBezierTextMarkers(codeMirror, range, optionsForType(WI.InlineSwatch.Type.Bezier));
> 
> It's kind of a bummer this won't be shown for the easing function since that's in the header. I don't think it needs to be there other than to save space. What about prepending the easing property name+value to the keyframe's style text?

That could be confusing, as the style text could itself contain a `transition-timing-function` or `animation-timing-function` property.  I put it in the header not primarily as a way to save space, but more to avoid this confusion.

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:98
>> +        if (item) {
> 
> Was this just a latent bug or are we now expecting to pass null to set selectedItem()?

Yes, we can now pass `null` as a way to deselect.

>> Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js:304
>> +            this._selectItem(null);
> 
> I see, now we deselect the item if the hit test failed.

Yup!

>> Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js:61
>> +        if (!WI.animationManager.supported) {
> 
> I don't understand. Don't we always want these canvas related navigation items? Or is this supposed to be items from the animation overview content view?
> 
> EDIT: Oh, I see, if Animations are not supported then there are no sub-sections in the main content area and [ ] Record First [n] Frames needs to be added to the main bar in that case.

Yup!

[Devin Rousso]

Created attachment 388923 [details]
Patch

[Devin Rousso]

Created attachment 389028 [details]
Patch

Add debug logging

[Devin Rousso]

Created attachment 389047 [details]
Patch

[Devin Rousso]

Created attachment 389049 [details]
Patch

Rebase

[Devin Rousso]

Created attachment 389108 [details]
[Patch] test modifying JavaScriptCore/inspector/scripts for win

Modify 'Source/JavaScriptCore/inspector/scripts/*' so that it generates a fresh build

[Devin Rousso]

Created attachment 389152 [details]
[Patch] test modifying WTF/wtf/Assertions.h for win

[Devin Rousso]

Comment on attachment 389049 [details]
Patch

My suspicion is that the windows build failure can be solved by a clean build, which isn't possible to do via bugzilla.  The build failure suggests that 'DerivedSources/JavaScript/inspector/InspectorProtocolObjects.h' is not being regenerated correctly, which is why only _some_ of the `override` functions added to `WebCore::InspectorAnimationAgent` are erroring (e.g. `startTracking` vs `resolveAnimation`), as well as why only _some_ values used from `Inspector::Protocol::Animation` are erroring (e.g. `Inspector::Protocol::Animation::AnimationState` vs `Inspector::Protocol::Animation::FillMode`).

[Devin Rousso]

Comment on attachment 389049 [details]
Patch

Oh wait oops there's a test expectation to update now that <https://webkit.org/b/206899> landed 😅

[Devin Rousso]

Created attachment 389170 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 389170 [details]
Patch

Rejecting attachment 389170 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 389170, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
ews/CanvasSidebarPanel.js
patching file Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.css
rm 'Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.css'
patching file Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js
rm 'Source/WebInspectorUI/UserInterface/Views/CanvasTabContentView.js'
patching file Source/WebInspectorUI/UserInterface/Views/CollectionContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/CollectionContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/ContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/DetailsSectionSimpleRow.js
patching file Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/GraphicsOverviewContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/GraphicsTabContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/GraphicsTabContentView.js
patching file Source/WebInspectorUI/UserInterface/Views/Main.css
patching file Source/WebInspectorUI/UserInterface/Views/RecordingContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.css
patching file Source/WebInspectorUI/UserInterface/Views/ResourceCollectionContentView.js
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/animation/effectChanged-expected.txt
patching file LayoutTests/inspector/animation/effectChanged.html
patching file LayoutTests/inspector/animation/lifecycle-css-animation-expected.txt
patching file LayoutTests/inspector/animation/lifecycle-css-animation.html
patching file LayoutTests/inspector/animation/lifecycle-css-transition-expected.txt
patching file LayoutTests/inspector/animation/lifecycle-css-transition.html
patching file LayoutTests/inspector/animation/lifecycle-web-animation-expected.txt
patching file LayoutTests/inspector/animation/lifecycle-web-animation.html
patching file LayoutTests/inspector/animation/resolveAnimation-expected.txt
patching file LayoutTests/inspector/animation/resolveAnimation.html
patching file LayoutTests/inspector/animation/resources/lifecycle-utilities.js
patching file LayoutTests/inspector/animation/targetChanged-expected.txt
patching file LayoutTests/inspector/animation/targetChanged.html
patching file LayoutTests/inspector/canvas/context-attributes.html
patching file LayoutTests/inspector/canvas/create-context-webgpu.html
patching file LayoutTests/inspector/canvas/extensions.html
patching file LayoutTests/inspector/canvas/memory.html
patching file LayoutTests/inspector/canvas/recording.html
patching file LayoutTests/inspector/canvas/requestClientNodes.html
patching file LayoutTests/inspector/canvas/requestContent-2d.html
patching file LayoutTests/inspector/canvas/requestContent-bitmaprenderer.html
patching file LayoutTests/inspector/canvas/requestContent-webgl.html
patching file LayoutTests/inspector/canvas/requestContent-webgl2.html
patching file LayoutTests/inspector/canvas/requestNode.html
patching file LayoutTests/inspector/canvas/resolveContext-2d.html
patching file LayoutTests/inspector/canvas/resolveContext-bitmaprenderer.html
patching file LayoutTests/inspector/canvas/resolveContext-webgl.html
patching file LayoutTests/inspector/canvas/resolveContext-webgl2.html
patching file LayoutTests/inspector/canvas/resolveContext-webgpu.html
patching file LayoutTests/inspector/canvas/resources/create-context-utilities.js
patching file LayoutTests/inspector/canvas/resources/recording-utilities.js
patching file LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgl.js
patching file LayoutTests/inspector/canvas/resources/shaderProgram-utilities-webgpu.js
patching file LayoutTests/inspector/canvas/setRecordingAutoCaptureFrameCount.html
patching file LayoutTests/inspector/canvas/shaderProgram-add-remove-webgpu.html
patching file LayoutTests/inspector/canvas/updateShader-webgpu-sharedVertexFragment.html
patching file LayoutTests/inspector/unit-tests/test-harness-expect-functions-expected.txt
patching file LayoutTests/inspector/unit-tests/test-harness-expect-functions.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From git://git.webkit.org/WebKit
   e110b9e0de2..70a545afe2e  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 255380 = e110b9e0de226ce9b36848479e11e6aa48a27847
r255381 = c500666fd0e34bf281fc4b2df948ba89bf9c2e84
r255382 = e3f7f0f52a0ff209edb9ff506247ecc6e27917be
r255383 = 0071f13e56d8e5b774c7a9bbb2d09f3d16fedbbc
r255384 = 70a545afe2e24be94b9af1d18bf1b77b9a6c76eb
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/13313584

[Devin Rousso]

Created attachment 389188 [details]
Patch

Rebase

[WebKit Commit Bot]

Comment on attachment 389188 [details]
Patch

Clearing flags on attachment: 389188

Committed r255396: <https://trac.webkit.org/changeset/255396>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.