Skip to content
Permalink
Browse files
[Web Inspector] Graphics tab should display pseudo-elements for more …
…than ::before and ::after

https://bugs.webkit.org/show_bug.cgi?id=235234
<rdar://87766777>

Reviewed by Devin Rousso.

Source/JavaScriptCore:

Add a new `DOM.Styleable` type to be used as the parameter type for `requestEffectTarget()` callbacks.

* inspector/protocol/Animation.json:
* inspector/protocol/DOM.json:

Source/WebCore:

Until now, we would pass the result of KeyframeEffect::targetElementOrPseudoElement() to
InspectorAnimationAgent::willApplyKeyframeEffect(). This meant that the inspector would only
be told of an animation target as an Element or a PseudoElement but not as an Element / PseudoId
pair, thus only allowing ::before and ::after to be represented since only those pseudo-elements
create a PseudoElement.

We now pass a Styleable, which encapsulate an Element / PseudoId pair, to
InspectorAnimationAgent::willApplyKeyframeEffect(). Additionally, the Styleable target is read from
the effect for callbacks provided to requestEffectTarget().

Sadly, we still rely on PseudoElement, but this patch at least removes all use of PseudoElement
from KeyframeEffect and pushes it down to InspectorDOMAgent with a new static method
elementToPushForStyleable() which the new pushStyleableElementToFrontend() and pushStyleablePathToFrontend()
methods use to turn the Styleable to an Element or PseudoElement.

In the future, we would need to further remove PseudoElement from Web Inspector and expose something
similar to Styleable throughout the codebase, or find some other way to encapsulate an Element / PseudoId
pair.

* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::apply):
(WebCore::KeyframeEffect::targetElementOrPseudoElement const): Deleted.
* animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::target const):
* inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::willApplyKeyframeEffectImpl):
* inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::willApplyKeyframeEffect):
* inspector/agents/InspectorAnimationAgent.cpp:
(WebCore::InspectorAnimationAgent::requestEffectTarget):
(WebCore::InspectorAnimationAgent::willApplyKeyframeEffect):
* inspector/agents/InspectorAnimationAgent.h:
* inspector/agents/InspectorCSSAgent.cpp:
(WebCore::InspectorCSSAgent::protocolValueForPseudoId):
(WebCore::protocolValueForPseudoId): Deleted.
* inspector/agents/InspectorCSSAgent.h:
* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::elementToPushForStyleable):
(WebCore::InspectorDOMAgent::pushStyleableElementToFrontend):
(WebCore::InspectorDOMAgent::pushStyleablePathToFrontend):
* inspector/agents/InspectorDOMAgent.h:

Source/WebInspectorUI:

Add a new `DOMStyleable` model class to match the new `DOM.Styleable` protocol type.

When calling Animation.requestEffectTarget(), we now use this new DOMStyleable class
to display pseudo-elements other than ::before or ::after in the Graphics tab.

* UserInterface/Base/DOMUtilities.js:
(WI.linkifyStyleable):
* UserInterface/Main.html:
* UserInterface/Models/Animation.js:
(WI.Animation.prototype.requestEffectTarget):
* UserInterface/Models/DOMStyleable.js: Added.
(WI.DOMStyleable.prototype.fromPayload):
(WI.DOMStyleable.prototype.get node):
(WI.DOMStyleable.prototype.get pseudoId):
(WI.DOMStyleable.prototype.get displayName):
(WI.DOMStyleable):
* UserInterface/Test.html:
* UserInterface/Views/AnimationCollectionContentView.js:
(WI.AnimationCollectionContentView.prototype._handleContentViewMouseEnter):
* UserInterface/Views/AnimationContentView.js:
(WI.AnimationContentView.prototype._refreshSubtitle):
* UserInterface/Views/AnimationDetailsSidebarPanel.js:
(WI.AnimationDetailsSidebarPanel.prototype._refreshIdentitySection):

LayoutTests:

Check that requestEffectTarget() returns the correct Styleable by checking whether it's defined
or null, and checking its `node` and `pseudoId` members.

* inspector/animation/targetChanged-expected.txt:
* inspector/animation/targetChanged.html:



Canonical link: https://commits.webkit.org/246442@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288623 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
graouts committed Jan 26, 2022
1 parent 1a39bc1 commit bd5219f744a96f80ae03d1cd14bef1361eed0a34
Showing 26 changed files with 340 additions and 53 deletions.
@@ -1,3 +1,17 @@
2022-01-26 Antoine Quint <graouts@webkit.org>

[Web Inspector] Graphics tab should display pseudo-elements for more than ::before and ::after
https://bugs.webkit.org/show_bug.cgi?id=235234
<rdar://87766777>

Reviewed by Devin Rousso.

Check that requestEffectTarget() returns the correct Styleable by checking whether it's defined
or null, and checking its `node` and `pseudoId` members.

* inspector/animation/targetChanged-expected.txt:
* inspector/animation/targetChanged.html:

2022-01-26 J Pascoe <j_pascoe@apple.com>

[WebAuthn] Add authenticator attachment used during authentication to credential payload
@@ -3,16 +3,33 @@ Tests for the Animation.targetChanged event.

== Running test suite: Animation.targetChanged
-- Running test case: Animation.targetChanged.NewTarget
PASS: Animation should have a target.
PASS: Animation effect should have a target element.
PASS: Animation effect should not have a target pseudo-element.
Changing target...

PASS: Animation should have a target.
PASS: Animation effect should have changed.
PASS: Animation effect should have a target element.
PASS: Animation effect should not have a target pseudo-element.
PASS: Animation effect target should have changed.
PASS: Animation effect target element should have changed.

-- Running test case: Animation.targetChanged.NullTarget
PASS: Animation should have a target.
PASS: Animation effect should have a target element.
PASS: Animation effect should not have a target pseudo-element.
Changing target...

PASS: Animation should not have a target.
PASS: Animation effect should have changed.
PASS: Animation effect should not have a target element.
PASS: Animation effect target should have changed.

-- Running test case: Animation.targetChanged.NewTargetWithPseudoId
PASS: Animation effect should not have a target.
Changing target...

PASS: Animation effect should have a target element.
PASS: Animation effect should not have a target pseudo-element.
Changing target again...

PASS: Animation effect should have a target element.
PASS: Animation effect should have a target pseudo-element.
PASS: Animation effect target should have changed.
PASS: Animation effect target element should not have changed.

@@ -29,7 +29,8 @@
InspectorTest.assert(animation.animationType === WI.Animation.Type.WebAnimation, "Animation should be a web animation.");

let [oldTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectThat(oldTarget instanceof WI.DOMNode, "Animation should have a target.");
InspectorTest.expectThat(oldTarget.node instanceof WI.DOMNode, "Animation effect should have a target element.");
InspectorTest.expectNull(oldTarget.pseudoId, "Animation effect should not have a target pseudo-element.");

InspectorTest.log("Changing target...\n");
await Promise.all([
@@ -38,9 +39,11 @@
]);

let [newTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectThat(newTarget instanceof WI.DOMNode, "Animation should have a target.");
InspectorTest.expectThat(newTarget.node instanceof WI.DOMNode, "Animation effect should have a target element.");
InspectorTest.expectNull(newTarget.pseudoId, "Animation effect should not have a target pseudo-element.");

InspectorTest.expectNotEqual(newTarget, oldTarget, "Animation effect should have changed.");
InspectorTest.expectNotEqual(newTarget, oldTarget, "Animation effect target should have changed.");
InspectorTest.expectNotEqual(newTarget.node, oldTarget.node, "Animation effect target element should have changed.");
},
});

@@ -60,7 +63,8 @@
InspectorTest.assert(animation.animationType === WI.Animation.Type.WebAnimation, "Animation should be a web animation.");

let [oldTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectThat(oldTarget instanceof WI.DOMNode, "Animation should have a target.");
InspectorTest.expectThat(oldTarget.node instanceof WI.DOMNode, "Animation effect should have a target element.");
InspectorTest.expectNull(oldTarget.pseudoId, "Animation effect should not have a target pseudo-element.");

InspectorTest.log("Changing target...\n");
await Promise.all([
@@ -69,9 +73,52 @@
]);

let [newTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectNull(newTarget, "Animation should not have a target.");
InspectorTest.expectNull(newTarget, "Animation effect should not have a target element.");

InspectorTest.expectNotEqual(newTarget, oldTarget, "Animation effect should have changed.");
InspectorTest.expectNotEqual(newTarget, oldTarget, "Animation effect target should have changed.");
},
});

suite.addTestCase({
name: "Animation.targetChanged.NewTargetWithPseudoId",
description: "Should return a valid object for the given animation identifier.",
async test() {
let animations = Array.from(WI.animationManager.animationCollection);
InspectorTest.assert(animations.length === 1, "There should only be one animation.");

let animation = animations[0];
if (!animation) {
throw `Missing animation.`;
return;
}

InspectorTest.assert(animation.animationType === WI.Animation.Type.WebAnimation, "Animation should be a web animation.");

let [oldTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectNull(oldTarget, "Animation effect should not have a target.");

InspectorTest.log("Changing target...\n");
await Promise.all([
animation.awaitEvent(WI.Animation.Event.TargetChanged),
InspectorTest.evaluateInPage(`window.animation.effect.target = document.createElement("div")`),
]);

let [newTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectThat(newTarget.node instanceof WI.DOMNode, "Animation effect should have a target element.");
InspectorTest.expectNull(newTarget.pseudoId, "Animation effect should not have a target pseudo-element.");

InspectorTest.log("Changing target again...\n");
await Promise.all([
animation.awaitEvent(WI.Animation.Event.TargetChanged),
InspectorTest.evaluateInPage(`window.animation.effect.pseudoElement = "::before"`),
]);

let [newestTarget] = await promisify((callback) => { animation.requestEffectTarget(callback); });
InspectorTest.expectThat(newestTarget.node instanceof WI.DOMNode, "Animation effect should have a target element.");
InspectorTest.expectEqual(newestTarget.pseudoId, "before", "Animation effect should have a target pseudo-element.");

InspectorTest.expectNotEqual(newTarget, newestTarget, "Animation effect target should have changed.");
InspectorTest.expectEqual(newTarget.node, newestTarget.node, "Animation effect target element should not have changed.");
},
});

@@ -1,3 +1,16 @@
2022-01-26 Antoine Quint <graouts@webkit.org>

[Web Inspector] Graphics tab should display pseudo-elements for more than ::before and ::after
https://bugs.webkit.org/show_bug.cgi?id=235234
<rdar://87766777>

Reviewed by Devin Rousso.

Add a new `DOM.Styleable` type to be used as the parameter type for `requestEffectTarget()` callbacks.

* inspector/protocol/Animation.json:
* inspector/protocol/DOM.json:

2022-01-26 Yusuke Suzuki <ysuzuki@apple.com>

[JSC] Do not run testPingPongStackOverflow while running multithreaded MultithreadedMultiVMExecutionTest
@@ -88,7 +88,7 @@
{ "name": "animationId", "$ref": "AnimationId" }
],
"returns": [
{ "name": "nodeId", "$ref": "DOM.NodeId" }
{ "name": "nodeId", "$ref": "DOM.Styleable" }
]
},
{
@@ -168,6 +168,15 @@
{ "name": "borderColor", "$ref": "RGBAColor", "optional": true, "description": "The border highlight fill color (default: transparent)." },
{ "name": "marginColor", "$ref": "RGBAColor", "optional": true, "description": "The margin highlight fill color (default: transparent)." }
]
},
{
"id": "Styleable",
"type": "object",
"properties": [
{ "name": "nodeId", "$ref": "NodeId" },
{ "name": "pseudoId", "$ref": "CSS.PseudoId", "optional": true }
],
"description": "An object referencing a node and a pseudo-element, primarily used to identify an animation effect target."
}
],
"commands": [
@@ -1,3 +1,53 @@
2022-01-26 Antoine Quint <graouts@webkit.org>

[Web Inspector] Graphics tab should display pseudo-elements for more than ::before and ::after
https://bugs.webkit.org/show_bug.cgi?id=235234
<rdar://87766777>

Reviewed by Devin Rousso.

Until now, we would pass the result of KeyframeEffect::targetElementOrPseudoElement() to
InspectorAnimationAgent::willApplyKeyframeEffect(). This meant that the inspector would only
be told of an animation target as an Element or a PseudoElement but not as an Element / PseudoId
pair, thus only allowing ::before and ::after to be represented since only those pseudo-elements
create a PseudoElement.

We now pass a Styleable, which encapsulate an Element / PseudoId pair, to
InspectorAnimationAgent::willApplyKeyframeEffect(). Additionally, the Styleable target is read from
the effect for callbacks provided to requestEffectTarget().

Sadly, we still rely on PseudoElement, but this patch at least removes all use of PseudoElement
from KeyframeEffect and pushes it down to InspectorDOMAgent with a new static method
elementToPushForStyleable() which the new pushStyleableElementToFrontend() and pushStyleablePathToFrontend()
methods use to turn the Styleable to an Element or PseudoElement.

In the future, we would need to further remove PseudoElement from Web Inspector and expose something
similar to Styleable throughout the codebase, or find some other way to encapsulate an Element / PseudoId
pair.

* animation/KeyframeEffect.cpp:
(WebCore::KeyframeEffect::apply):
(WebCore::KeyframeEffect::targetElementOrPseudoElement const): Deleted.
* animation/KeyframeEffect.h:
(WebCore::KeyframeEffect::target const):
* inspector/InspectorInstrumentation.cpp:
(WebCore::InspectorInstrumentation::willApplyKeyframeEffectImpl):
* inspector/InspectorInstrumentation.h:
(WebCore::InspectorInstrumentation::willApplyKeyframeEffect):
* inspector/agents/InspectorAnimationAgent.cpp:
(WebCore::InspectorAnimationAgent::requestEffectTarget):
(WebCore::InspectorAnimationAgent::willApplyKeyframeEffect):
* inspector/agents/InspectorAnimationAgent.h:
* inspector/agents/InspectorCSSAgent.cpp:
(WebCore::InspectorCSSAgent::protocolValueForPseudoId):
(WebCore::protocolValueForPseudoId): Deleted.
* inspector/agents/InspectorCSSAgent.h:
* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::elementToPushForStyleable):
(WebCore::InspectorDOMAgent::pushStyleableElementToFrontend):
(WebCore::InspectorDOMAgent::pushStyleablePathToFrontend):
* inspector/agents/InspectorDOMAgent.h:

2022-01-26 J Pascoe <j_pascoe@apple.com>

[WebAuthn] Add authenticator attachment used during authentication to credential payload
@@ -48,7 +48,6 @@
#include "KeyframeEffectStack.h"
#include "Logging.h"
#include "PropertyAllowlist.h"
#include "PseudoElement.h"
#include "RenderBox.h"
#include "RenderBoxModelObject.h"
#include "RenderElement.h"
@@ -1182,19 +1181,6 @@ bool KeyframeEffect::targetsPseudoElement() const
return m_target.get() && m_pseudoId != PseudoId::None;
}

Element* KeyframeEffect::targetElementOrPseudoElement() const
{
if (m_target) {
if (m_pseudoId == PseudoId::Before)
return m_target->beforePseudoElement();

if (m_pseudoId == PseudoId::After)
return m_target->afterPseudoElement();
}

return m_target.get();
}

void KeyframeEffect::setTarget(RefPtr<Element>&& newTarget)
{
if (m_target == newTarget)
@@ -1288,7 +1274,7 @@ void KeyframeEffect::apply(RenderStyle& targetStyle, const Style::ResolutionCont
auto computedTiming = getComputedTiming(startTime);
if (!startTime) {
m_phaseAtLastApplication = computedTiming.phase;
if (auto* target = targetElementOrPseudoElement())
if (auto target = targetStyleable())
InspectorInstrumentation::willApplyKeyframeEffect(*target, *this, computedTiming);
}

@@ -101,7 +101,6 @@ class KeyframeEffect : public AnimationEffect
const Vector<ParsedKeyframe>& parsedKeyframes() const { return m_parsedKeyframes; }

Element* target() const { return m_target.get(); }
Element* targetElementOrPseudoElement() const;
void setTarget(RefPtr<Element>&&);

bool targetsPseudoElement() const;
@@ -1120,7 +1120,7 @@ bool InspectorInstrumentation::isWebGLProgramHighlightedImpl(InstrumentingAgents
}
#endif

void InspectorInstrumentation::willApplyKeyframeEffectImpl(InstrumentingAgents& instrumentingAgents, Element& target, KeyframeEffect& effect, ComputedEffectTiming computedTiming)
void InspectorInstrumentation::willApplyKeyframeEffectImpl(InstrumentingAgents& instrumentingAgents, const Styleable& target, KeyframeEffect& effect, ComputedEffectTiming computedTiming)
{
if (auto* animationAgent = instrumentingAgents.trackingAnimationAgent())
animationAgent->willApplyKeyframeEffect(target, effect, computedTiming);
@@ -99,6 +99,8 @@ class WebKitNamedFlow;
class WebSocketChannel;
class WorkerOrWorkletGlobalScope;

struct Styleable;

#if ENABLE(WEBGL)
class WebGLProgram;
#endif
@@ -299,7 +301,7 @@ class InspectorInstrumentation {
static bool isWebGLProgramHighlighted(WebGLRenderingContextBase&, WebGLProgram&);
#endif

static void willApplyKeyframeEffect(Element&, KeyframeEffect&, ComputedEffectTiming);
static void willApplyKeyframeEffect(const Styleable&, KeyframeEffect&, ComputedEffectTiming);
static void didChangeWebAnimationName(WebAnimation&);
static void didSetWebAnimationEffect(WebAnimation&);
static void didChangeWebAnimationEffectTiming(WebAnimation&);
@@ -502,7 +504,7 @@ class InspectorInstrumentation {
static bool isWebGLProgramHighlightedImpl(InstrumentingAgents&, WebGLProgram&);
#endif

static void willApplyKeyframeEffectImpl(InstrumentingAgents&, Element&, KeyframeEffect&, ComputedEffectTiming);
static void willApplyKeyframeEffectImpl(InstrumentingAgents&, const Styleable&, KeyframeEffect&, ComputedEffectTiming);
static void didChangeWebAnimationNameImpl(InstrumentingAgents&, WebAnimation&);
static void didSetWebAnimationEffectImpl(InstrumentingAgents&, WebAnimation&);
static void didChangeWebAnimationEffectTimingImpl(InstrumentingAgents&, WebAnimation&);
@@ -1455,10 +1457,10 @@ inline bool InspectorInstrumentation::isWebGLProgramHighlighted(WebGLRenderingCo
}
#endif

inline void InspectorInstrumentation::willApplyKeyframeEffect(Element& target, KeyframeEffect& effect, ComputedEffectTiming computedTiming)
inline void InspectorInstrumentation::willApplyKeyframeEffect(const Styleable& target, KeyframeEffect& effect, ComputedEffectTiming computedTiming)
{
FAST_RETURN_IF_NO_FRONTENDS(void());
if (auto* agents = instrumentingAgents(target.document()))
if (auto* agents = instrumentingAgents(target.element.document()))
willApplyKeyframeEffectImpl(*agents, target, effect, computedTiming);
}

@@ -38,6 +38,7 @@
#include "Event.h"
#include "FillMode.h"
#include "Frame.h"
#include "InspectorCSSAgent.h"
#include "InspectorDOMAgent.h"
#include "InstrumentingAgents.h"
#include "JSExecState.h"
@@ -47,6 +48,7 @@
#include "Page.h"
#include "PlaybackDirection.h"
#include "RenderElement.h"
#include "Styleable.h"
#include "TimingFunction.h"
#include "WebAnimation.h"
#include <JavaScriptCore/IdentifiersFactory.h>
@@ -279,7 +281,7 @@ Protocol::ErrorStringOr<void> InspectorAnimationAgent::disable()
return { };
}

Protocol::ErrorStringOr<Protocol::DOM::NodeId> InspectorAnimationAgent::requestEffectTarget(const Protocol::Animation::AnimationId& animationId)
Protocol::ErrorStringOr<Ref<Protocol::DOM::Styleable>> InspectorAnimationAgent::requestEffectTarget(const Protocol::Animation::AnimationId& animationId)
{
Protocol::ErrorString errorString;

@@ -297,11 +299,11 @@ Protocol::ErrorStringOr<Protocol::DOM::NodeId> InspectorAnimationAgent::requestE

auto& keyframeEffect = downcast<KeyframeEffect>(*effect);

auto* target = keyframeEffect.targetElementOrPseudoElement();
auto target = keyframeEffect.targetStyleable();
if (!target)
return makeUnexpected("Animation for given animationId does not have a target"_s);

return domAgent->pushNodePathToFrontend(errorString, target);
return domAgent->pushStyleablePathToFrontend(errorString, *target);
}

Protocol::ErrorStringOr<Ref<Protocol::Runtime::RemoteObject>> InspectorAnimationAgent::resolveAnimation(const Protocol::Animation::AnimationId& animationId, const String& objectGroup)
@@ -371,7 +373,7 @@ static bool isDelayed(ComputedEffectTiming& computedTiming)
return computedTiming.localTime.value() < (computedTiming.endTime - computedTiming.activeDuration);
}

void InspectorAnimationAgent::willApplyKeyframeEffect(Element& target, KeyframeEffect& keyframeEffect, ComputedEffectTiming computedTiming)
void InspectorAnimationAgent::willApplyKeyframeEffect(const Styleable& target, KeyframeEffect& keyframeEffect, ComputedEffectTiming computedTiming)
{
auto* animation = keyframeEffect.animation();
if (!is<DeclarativeAnimation>(animation))
@@ -421,7 +423,7 @@ void InspectorAnimationAgent::willApplyKeyframeEffect(Element& target, KeyframeE

if (ensureResult.isNewEntry) {
if (auto* domAgent = m_instrumentingAgents.persistentDOMAgent()) {
if (auto nodeId = domAgent->pushNodeToFrontend(&target))
if (auto nodeId = domAgent->pushStyleableElementToFrontend(target))
event->setNodeId(nodeId);
}

0 comments on commit bd5219f

Please sign in to comment.