Bug 235234 – [Web Inspector] Graphics tab should display pseudo-elements for more than ::before and ::after

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

[Antoine Quint]

[Web Inspector] animation tab should display pseudo-elements for more than ::before and ::after

[Antoine Quint]

Created attachment 449180 [details]
Patch

[EWS Watchlist]

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

[Antoine Quint]

Created attachment 449199 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 449199 [details]
Patch

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

Neat!

Is there a good way to test this? Maybe building off of:
LayoutTests/inspector/animation/targetChanged.html

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:662
> +    return pushNodePathToFrontend(errorString, element ? element : &styleable.element);

From the above, won't `element` always beat least `&styleable.element`? So this could be hopefully just be:

    return pushNodePathToFrontend(errorString, element);

[Antoine Quint]

(In reply to Joseph Pecoraro from comment #4)
> Comment on attachment 449199 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449199&action=review
> 
> Neat!
> 
> Is there a good way to test this? Maybe building off of:
> LayoutTests/inspector/animation/targetChanged.html

I wasn't aware of the testing strategy, will take a look.

> > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:662
> > +    return pushNodePathToFrontend(errorString, element ? element : &styleable.element);
> 
> From the above, won't `element` always beat least `&styleable.element`? So
> this could be hopefully just be:
> 
>     return pushNodePathToFrontend(errorString, element);

No, elementToPushForStyleable() could return null if somehow we ask for ::before or ::after and the backing PseudoElement hasn't been created yet.

[Antoine Quint]

Created attachment 449209 [details]
Patch

[Antoine Quint]

(In reply to Joseph Pecoraro from comment #4)
> Comment on attachment 449199 [details]
> Patch
> 
> Is there a good way to test this? Maybe building off of:
> LayoutTests/inspector/animation/targetChanged.html

I've modified this test to account for pseudoId.

[Devin Rousso]

Comment on attachment 449199 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/Animation.json:92
> +                { "name": "nodeId", "$ref": "DOM.NodeId" },
> +                { "name": "pseudoId", "$ref": "CSS.PseudoId", "optional": true }

I wonder if it'd be better to add a new type to DOM (or CSS) called `Styleable` so that this can be reused in other places too.  Something like
```
{
    "id": "Styleable",
    "type": "object",
    "properties": {
        { "name": "nodeId", "$ref": "NodeId" },
        { "name": "pseudoId", "$ref": "CSS.PseudoId", "optional": true }
    }
}
```
and then you could have this just be
```
"returns": [
    { "name": "effectTarget", "$ref": "CSS.Styleable" }
]
```
and `InspectorDOMAgent::pushStyleableToFrontend` would instead return a `Ref<Inspector::Protocol::DOM::Styleable>`.

> Source/WebCore/inspector/agents/InspectorAnimationAgent.cpp:309
> +    std::tuple<Inspector::Protocol::DOM::NodeId, std::optional<Inspector::Protocol::CSS::PseudoId>> result = { domAgent->pushStyleablePathToFrontend(errorString, *target), pseudoId };
> +    return result;

Can we inline this?

Tho I don't think this will be the same code given my above suggestion :)

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:65
> +    if (!node.isPseudoElement() && options.pseudoId)
> +        displayName = `::${options.pseudoId}`;

I think we should put this in `WI.DOMNode.prototype.get displayName` instead.  It already has

> Source/WebInspectorUI/UserInterface/Models/Animation.js:212
> +        target.AnimationAgent.requestEffectTarget(this._animationId, (error, nodeId, pseudoId) => {

Please add this comment before this line
```
// COMPATIBILITY (iOS 15): pseudoId did not exist yet.
```

Also, I think we may want to create a `WI.DOMStyleable` to match the `Inspector::Protocol::DOM::Styleable` that I suggested above.  Something simple like
```
WI.DOMStyleable = class DOMStyleable
{
    constructor(node, {pseudoId} = {})
    {
        console.assert(node instanceof WI.DOMNode, node);
        console.assert(!node.isPseudoElement(), node);
        console.assert(!pseudoId, Object.values(WI.CSSManager.PseudoSelectorNames).includes(pseudoId), pseudoId);

        this._node = node;
        this._pseudoId = pseudoId || null;
    }

    // Static

    static fromPayload({nodeId, pseudoId})
    {
        return new WI.DOMStyleable(WI.domManager.nodeForId(nodeId), {pseudoId});
    }

    // Public

    get node() { return this._node; }
    get pseudoId() { return this._pseudoId; }

    get displayName()
    {
        return this._node.displayName + (this._pseudoId ? WI.CSSManager.displayNameForPseudoId(this._pseudoId) : "");
    }
};
```

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:170
> +                this._subtitleElement.appendChild(WI.linkifyNodeReference(domNode, { pseudoId }));

Style: we don't add spaces inside `{` `}` in Web Inspector code :)

> Source/WebInspectorUI/UserInterface/Views/AnimationDetailsSidebarPanel.js:183
> +            this._targetRow.value = domNode ? WI.linkifyNodeReference(domNode, { pseudoId }) : null;

ditto (Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:170)

[Devin Rousso]

Comment on attachment 449199 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:65
>> +        displayName = `::${options.pseudoId}`;
> 
> I think we should put this in `WI.DOMNode.prototype.get displayName` instead.  It already has

Oh oops I meant to delete this.  This idea was "replaced" by `WI.DOMStyleable.prototype.get displayName`.

[Antoine Quint]

Created attachment 449283 [details]
Patch

[Antoine Quint]

Is there something specific I should be doing for Windows? I didn't add any new files, but it looks like the changes made to CSS.json and Animation.json didn't quite re-generate the required code.

[Antoine Quint]

Also, I think I should make the call sites to requestEffectTarget() in the front-end code deal with the old return type to correctly handle older backends, right?

[Devin Rousso]

(In reply to Antoine Quint from comment #11)
> Is there something specific I should be doing for Windows? I didn't add any new files, but it looks like the changes made to CSS.json and Animation.json didn't quite re-generate the required code.

Nah sadly the windows bot seems to have issues with changes to the inspector protocol for incremental (i.e. non-clean) builds.  I wouldn't worry about it :)


(In reply to Antoine Quint from comment #12)
> Also, I think I should make the call sites to requestEffectTarget() in the front-end code deal with the old return type to correctly handle older backends, right?
I think it'd be better to have the caller of the protocol `Animation.requestEffectTarget` handle older backends and auto-convert/auto-update to the new object so that callsites don't have to deal with it.

[Devin Rousso]

Comment on attachment 449283 [details]
Patch

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

r=me, nice work (and test)!

> Source/JavaScriptCore/inspector/protocol/CSS.json:270
> +            "id": "Styleable",

Can we add a `"description": "..."` for this?

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:558
> +    return pushNodeToFrontend(element ? element : &styleable.element);

NIT: `element ?: &styleable.element`

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:660
> +Ref<Protocol::CSS::Styleable> InspectorDOMAgent::pushStyleablePathToFrontend(Protocol::ErrorString errorString, const Styleable& styleable)

NIT: It's a bit odd for the `InspectorDOMAgent` to be dealing with things from the `CSS` protocol domain (especially since the `DOM` protocol domain is basically a dependency of the `CSS` protocol domain).  I think I'd either move this to `InspectorCSSAgent` (which would use `m_instrumentingAgents.persistentDOMAgent()`) or move the `Styleable` protocol object to the `DOM` domain (which should also change `WI.CSSStyleable` to `WI.DOMStyleable` btw).  Regardless, in either case, please make sure the naming is consistent in the frontend too :)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:663
> +    auto nodeId = pushNodePathToFrontend(errorString, element ? element : &styleable.element);

ditto (:558)

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:669
> +    if (styleable.pseudoId != PseudoId::None) {

NIT:  I wonder if instead of checking for `!= PseudoId::None` we should just remove the `ASSERT_NOT_REACHED` in `InspectorCSSAgent::protocolValueForPseudoId` 🤔

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:673
> +    }
> +    return protocolStyleable;

Style: I'd add a newline between these

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:60
> +WI.linkifyStyleable = function(styleable)

NIT: I'd add a `console.assert(styleable instanceof WI.CSSStyleable, styleable);` just in case :)

> Source/WebInspectorUI/UserInterface/Base/DOMUtilities.js:71
> +

oops?

> Source/WebInspectorUI/UserInterface/Models/Animation.js:43
> +        // COMPATIBILITY (iOS 15): effectTarget changed from DOM.NodeId to CSS.Styleable.

This should really be next to `requestEffectTarget` since that's actually where the difference is.  The callback given to it also needs to handle both `CSS.Styleable` and `DOM.NodeId`.

> Source/WebInspectorUI/UserInterface/Models/Animation.js:213
> +        target.AnimationAgent.requestEffectTarget(this._animationId, (error, styleable) => {
> +            this._effectTarget = !error ? WI.CSSStyleable.fromPayload(styleable) : null;

ditto (:43)
```
    target.AnimationAgent.requestEffectTarget(this._animationId, (error, styleable) => {
        // COMPATIBILITY (iOS 15): effectTarget changed from DOM.NodeId to CSS.Styleable.
        if (!isNaN(styleable))
            styleable = {nodeId: styleable};

        ...
    });
```

You can confirm that this works by using your locally ToT build to inspect an older iOS simulator (which shouldn't have these changes).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleable.js:1
> +WI.CSSStyleable = class CSSStyleable

I think this needs the copyright header 😅

> Source/WebInspectorUI/UserInterface/Models/CSSStyleable.js:8
> +        if (pseudoId)
> +            console.assert(Object.values(WI.CSSManager.PseudoSelectorNames).includes(pseudoId), pseudoId);

We strip `console.assert` from production builds, so this will cause some issues.  I'd do this instead:
```
    console.assert(node instanceof WI.DOMNode, node);
    console.assert(!pseudoId || Object.values(WI.CSSManager.PseudoSelectorNames).includes(pseudoId), pseudoId);
```

> Source/WebInspectorUI/UserInterface/Views/AnimationCollectionContentView.js:102
> +            styleable.node.highlight();

I wonder if we should make a `WI.CSSStyleable.prototype.highlight` and add a new `CSS.highlightStyleable` protocol command so that we can narrow down to the pseudo element (which we should already support).  This can probably be a followup tho :)

> Source/WebInspectorUI/UserInterface/Views/AnimationContentView.js:166
> +            this._animationTargetDOMNode = styleable.node;

I wonder if we should make this `_animationTarget` and create a `WI.appendContextMenuItemsForStyleable` so that we refer to the right node.  This can probably be a followup tho :)

[Radar WebKit Bug Importer]

<rdar://problem/87766777>

[Antoine Quint]

So on iOS 15, requestEffectTarget returns `undefined` for the `styleable` parameter in Animation.js and so we don't display the nodes in the Graphics tabs. Need to work out what's going on here.

[Antoine Quint]

Created attachment 450021 [details]
Patch for landing

[Antoine Quint]

(In reply to Antoine Quint from comment #16)
> So on iOS 15, requestEffectTarget returns `undefined` for the `styleable`
> parameter in Animation.js and so we don't display the nodes in the Graphics
> tabs. Need to work out what's going on here.

This was due to the change in Animation.json. We can't change the `name` key.

[EWS]

Committed r288623 (246442@main): <https://commits.webkit.org/246442@main>

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

[Ross Kirsling]

(In reply to Devin Rousso from comment #14)
> Comment on attachment 449283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449283&action=review
>
> > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:558
> > +    return pushNodeToFrontend(element ? element : &styleable.element);
> 
> NIT: `element ?: &styleable.element`

This is a GNU extension and must never be used (outside of files that are explicitly non-Windows).

[Fujii Hironori]

(In reply to Ross Kirsling from comment #20)
> (In reply to Devin Rousso from comment #14)
> > Comment on attachment 449283 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=449283&action=review
> >
> > > Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:558
> > > +    return pushNodeToFrontend(element ? element : &styleable.element);
> > 
> > NIT: `element ?: &styleable.element`
> 
> This is a GNU extension and must never be used (outside of files that are
> explicitly non-Windows).

Filed: Bug 235667 – REGRESSION(r288623) MSVC reports "InspectorDOMAgent.cpp(558,40): error C2059: syntax error: ':'"