Initial attempt at implementing referenceTarget. #3

alice · 2025-01-14T05:45:22Z

https://bugs.webkit.org/show_bug.cgi?id=285634

This change adds support for a referenceTarget property on ShadowRoot and ShadowRootInit, and a shadowrootreferencetarget property on Templates. It also adds support code to ensure that the referenceTarget is respected when computing values for internal use, such as creating the accessibility tree.

The commits in this branch hopefully represent separate PRs to be submitted later on; the first three refactor code without making any functionality changes, and the last change actually adds the feature and tests.

alice · 2025-01-16T05:21:35Z

Source/WebCore/html/parser/HTMLConstructionSite.cpp

@@ -568,9 +569,11 @@ void HTMLConstructionSite::insertHTMLTemplateElement(AtomHTMLToken&& token)
                clonable = ShadowRootClonable::Yes;
            else if (attribute.name() == HTMLNames::shadowrootserializableAttr)
                serializable = ShadowRootSerializable::Yes;
+            else if (document().settings().shadowRootReferenceTargetEnabled() && attribute.name() == HTMLNames::shadowrootreferencetargetAttr)


Is a settings check superfluous here?

This feels like a good one to keep to me, but I'm not as familiar with this part of the codebase, so might be a good question to carry forward.

alice · 2025-01-16T05:22:13Z

Source/WebCore/html/HTMLTemplateElement.cpp

@@ -105,6 +105,16 @@ void HTMLTemplateElement::setShadowRootMode(const AtomString& value)
    setAttribute(HTMLNames::shadowrootmodeAttr, value);
 }

+const AtomString& HTMLTemplateElement::shadowRootReferenceTarget() const
+{
+    return attributeWithoutSynchronization(HTMLNames::shadowrootreferencetargetAttr);


Should this have a settings check?

My gut reaction is no, since the attribute is settings-dependent:

[EnabledBySetting=ShadowRootReferenceTargetEnabled] attribute [AtomString] DOMString referenceTarget;

But I'm not super familiar with how that works, so maybe a good question to carry forward to your review request.

alice · 2025-01-16T05:28:47Z

Source/WebCore/dom/ShadowRoot.h

@@ -181,6 +186,8 @@ class ShadowRoot final : public DocumentFragment, public TreeScope {
    std::unique_ptr<Style::Scope> m_styleScope;
    std::unique_ptr<SlotAssignment> m_slotAssignment;
    mutable std::optional<PartMappings> m_partMappings;
+
+    AtomString m_referenceTarget;


This is potentially controversial to add to every shadow root for a feature likely to be relatively seldom used. Is there any good alternative?

WebKit has something called NodeRareData for this scenario. But it may be good for someone who is more knowledgeable about WebKit's DOM code to weigh in on whether that's appropriate. Maybe transfer this comment to your full-fledged review PR?

alice · 2025-01-16T05:29:56Z

Source/WebCore/dom/Element.cpp

@@ -3256,6 +3298,26 @@ RefPtr<ShadowRoot> Element::shadowRootForBindings(JSC::JSGlobalObject& lexicalGl
    return nullptr;
 }

+RefPtr<Element> Element::deepShadowRootReferenceTargetOrSelf() {
+    return const_cast<Element*>(const_cast<const Element*>(this)->deepShadowRootReferenceTargetOrSelf().get());


Is it a good idea to create const and non-const versions like this?

alice · 2025-01-16T05:30:52Z

Source/WebCore/dom/Element.cpp

@@ -3220,11 +3259,13 @@ ExceptionOr<ShadowRoot&> Element::attachShadow(const ShadowRootInit& init)
        init.serializable ? ShadowRoot::Serializable::Yes : ShadowRoot::Serializable::No,
        isPrecustomizedOrDefinedCustomElement() ? ShadowRoot::AvailableToElementInternals::Yes : ShadowRoot::AvailableToElementInternals::No,
        WTFMove(registry), init.registry ? ShadowRoot::ScopedCustomElementRegistry::Yes : ShadowRoot::ScopedCustomElementRegistry::No);
+    if (document().settings().shadowRootReferenceTargetEnabled())


Is this settings check superfluous?

I think so, since setReferenceTarget rightfully does a settings check.

alice · 2025-01-16T05:32:12Z

Source/WebCore/dom/CustomElementDefaultARIA.cpp

@@ -91,6 +91,7 @@ RefPtr<Element> CustomElementDefaultARIA::elementForAttribute(const Element& thi

    RefPtr<Element> result;
    std::visit(WTF::makeVisitor([&](const AtomString& stringValue) {
+        // FIXME: This path shouldn't be necessary?


I'll need to fix this one way or another, since if we want to leave this path in, it should also respect referenceTarget. However, I can't imagine how it could be exercised, so perhaps this should be a NOT_REACHED?

twilco · 2025-01-16T15:47:44Z

Source/WebCore/accessibility/AXObjectCache.cpp

+    if (elementId.isNull())
+        return nullptr;
+
+    RefPtr<Element> element = treeScope.getElementById(elementId);
+    return element->deepShadowRootReferenceTargetOrSelf();


TreeScope::getElementById(AtomString&) already checks for a null element ID (technically it calls isEmpty, which returns true for both null and empty "" strings), so I think we can get away with not doing that here.

Do we want to null-check the result of treeScope.getElementById? Presumably that would crash if given a non-empty id that doesn't point to any element, unless there's some reason we know that can't happen here.

One alternative:

RefPtr element = treeScope.getElementById(elementId); return element ? element->deepShadowRootReferenceTargetOrSelf() : nullptr;

Oh that's much better, and yikes on the potential crash.

twilco · 2025-01-16T15:54:47Z

Source/WebCore/accessibility/AccessibilityObject.cpp

    auto element = this->element();
-    if (!element)
+    if (!element || !is<HTMLInputElement>(element))
        return false;

-    auto datalist = element->treeScope().getElementById(datalistId);
-    return is<HTMLDataListElement>(datalist);
+    return dynamicDowncast<HTMLInputElement>(element)->hasDataList();


dynamicDowncast<T> also does an is<T> check, so we could be slightly more efficient with something like this:

RefPtr input = dynamicDowncast<Element>(element()); return input && input->hasDataList();

That also fixes auto element = this->element(); above (which I know you didn't write 🙂), which is not ideal because it stores a raw pointer in auto rather than auto*.

twilco · 2025-01-16T15:57:19Z

Source/WebCore/dom/Element.cpp

 {
-    if (id.isEmpty())
+    if (id.isNull() || id.isEmpty())


Just id.isEmpty() should be fine, as AtomString::isEmpty() calls out to String::isEmpty, which is defined as:

// WTFString.h bool isNull() const { return !m_impl; } bool isEmpty() const { return !m_impl || m_impl->isEmpty(); }

Ah great, done.

twilco · 2025-01-16T15:58:27Z

Source/WebCore/dom/Element.cpp

        }
    }
+
+    if (!hasExplicitlySetElement) {
+    const AtomString& id = getAttribute(attributeName);


This may be missing one level of indentation (sorry, wish there was a good auto-formatting solution).

Argh, I don't know why my indentation keeps getting messed up. Thanks for spotting.

twilco · 2025-01-16T16:00:58Z

Source/WebCore/dom/Element.cpp

+RefPtr<Element> Element::getElementAttributeForBindings(const QualifiedName& attributeName) const
+{
+    ASSERT(isElementReflectionAttribute(document().settings(), attributeName));
+    RefPtr<Element> element = getElementForAttributeInternal(attributeName);


I believe it's more common to not include the <Element> type specification when you can do so, i.e. RefPtr instead of RefPtr<Element>

Do you know the reasoning for that?

I think for conciseness when the context makes it reasonably clear what type the RefPtr is going to hold. But I looked and this isn't officially codified in our style guidelines, so I think it could be fine to leave as-is too.

twilco · 2025-01-16T16:28:01Z

Source/WebCore/html/FormAssociatedElement.cpp

+    HTMLFormElement* formElement = form();
+    if (!formElement)
+        return nullptr;
+
+    if (asHTMLElement().document().settings().shadowRootReferenceTargetEnabled() && &formElement->treeScope() != &asHTMLElement().treeScope())
+        return nullptr;
+
+    return formElement;


Another way to write this could be:

if (asHTMLElement().document().settings().shadowRootReferenceTargetEnabled()) { auto* form = this->form(); if (!form || &form->treeScope() != &asHTMLElement().treeScope()) return nullptr; return form; } return form();

I think there's a couple other places where this pattern could apply to.

Is there a reason for the general preference for auto rather than using the explicit type?

This question has been discussed in the past, and there are proponents for both sides. I personally lean more towards explicit type specification outside of complicated types like lambdas / iterators / etc, but the project has aligned on auto, so I go with that 🙂

twilco · 2025-01-16T16:32:55Z

Source/WebCore/html/HTMLLabelElement.cpp

+                RefPtr<const Element> referenceTarget = descendant.deepShadowRootReferenceTargetOrSelf();
+                if (!referenceTarget || !is<HTMLElement>(referenceTarget))
+                    continue;
+                const HTMLElement* htmlReferenceTarget = dynamicDowncast<HTMLElement>(referenceTarget.get());
+                if (htmlReferenceTarget->isLabelable())
+                    return const_cast<HTMLElement*>(htmlReferenceTarget);


Another option for this that is a bit shorter and avoids an unnecessary is<T> check would be:

RefPtr referenceTarget = dynamicDowncast<HTMLElement>(descendant.deepShadowRootReferenceTargetOrSelf()); return referenceTarget && referenceTarget->isLabelable() ? const_cast<HTMLElement*>(referenceTarget.get()) : nullptr;

I think that would be a behaviour change as written, since it would return nullptr instead of continuing with the loop, but I think I've adapted it to avoid the is<T> as you suggest.

twilco · 2025-01-16T16:37:38Z

Source/WebCore/html/HTMLLabelElement.cpp

+            } else {
+                if (descendant.isLabelable())
+                    return const_cast<HTMLElement*>(&descendant);
+            }


Could this be:

} else if (descendant.isLabelable()) return const_cast<HTMLElement*>(&descendant);

Yes! I've left the braces in because the if clause has them.

twilco · 2025-01-16T16:40:42Z

Source/WebCore/html/HTMLTemplateElement.cpp

@@ -105,6 +105,16 @@ void HTMLTemplateElement::setShadowRootMode(const AtomString& value)
    setAttribute(HTMLNames::shadowrootmodeAttr, value);
 }

+const AtomString& HTMLTemplateElement::shadowRootReferenceTarget() const
+{
+    return attributeWithoutSynchronization(HTMLNames::shadowrootreferencetargetAttr);


My gut reaction is no, since the attribute is settings-dependent:

[EnabledBySetting=ShadowRootReferenceTargetEnabled] attribute [AtomString] DOMString referenceTarget;

But I'm not super familiar with how that works, so maybe a good question to carry forward to your review request.

twilco · 2025-01-16T16:42:39Z

Source/WebCore/html/parser/HTMLConstructionSite.cpp

@@ -568,9 +569,11 @@ void HTMLConstructionSite::insertHTMLTemplateElement(AtomHTMLToken&& token)
                clonable = ShadowRootClonable::Yes;
            else if (attribute.name() == HTMLNames::shadowrootserializableAttr)
                serializable = ShadowRootSerializable::Yes;
+            else if (document().settings().shadowRootReferenceTargetEnabled() && attribute.name() == HTMLNames::shadowrootreferencetargetAttr)


This feels like a good one to keep to me, but I'm not as familiar with this part of the codebase, so might be a good question to carry forward.

These methods currently delegate directly to form() and control() respectively, but will later be modified to ensure that nodes don't leak out of shadow roots once referenceTarget is supported.

This reserves list() for use in bindings.

Also use these methods in place of TreeScope::getElementById() for non-bindings code.

twilco · 2025-01-22T16:13:19Z

Sorry for the delayed replies!

…pector rdar://98891055 https://bugs.webkit.org/show_bug.cgi?id=283092 Reviewed by Ryosuke Niwa and BJ Burg. There currently exists a message WebInspectorUIProxy::OpenLocalInspectorFrontend, which the web process sends to the UI process to show Web Inspector for the current web page. This introduces security risks as a compromised website may find its way to send arbitrary messages to the UI process, opening Web Inspector and weakening the web content sandbox. The reason this message exists is because there are useful ways the web process needs to open Web Inspector with initiative. Normally, Web Inspector is opened via one of the Develop menu's items, which is controlled by the UI process. However, Web Inspector can also be opened without being prompted by the UI process first, in these places: 1. In a web page's context menu, the "Inspect Element" option 2. Inside Web Inspector, if the Debug UI is enabled, on the top right corner, a button to open inspector^2 3. In WebKitTestRunner, via the TestRunner::showWebInspector function This patch makes it so that web process can no longer send a message to a UI process to open Web Inspector. This means web process cannot open Web Inspector at will -- it must be either due to the UI process's demand, or it's in one of the above three cases. More details below. I have tested that this change preserves the above three special cases and does prevent the web page from opening Web Inspector at will. - Cases #1 and #2 can be tested from the UI. - Case #3 can be tested with a WebKit test involving Web Inspector. I ran the test LayoutTests/inspector/console/js-completions.html, where I saw the test crashing without special treatment for this case. - To verify that the web page can't open Web Inspector, I followed the reproduction steps from the Radar and saw Web Inspector no longer opens, and opening the external URL also failed as expected. * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.messages.in: * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.h: * Source/WebKit/UIProcess/Inspector/WebInspectorUIProxy.cpp: (WebKit::WebInspectorUIProxy::connect): - If the UI process wants to open Web Inspector, it sends a WebInspector::Show command to the web process. This patch makes that command take an async reply, so that the anticipated WebInspectorUIProxy::OpenLocalInspectorFrontend message from the web process can now be delivered through that async reply instead. This ensures that OpenLocalInspectorFrontend can only be done when initiated from the UI process (due to user interaction). (WebKit::WebInspectorUIProxy::markAsUnderTest): (WebKit::WebInspectorUIProxy::openLocalInspectorFrontend): (WebKit::WebInspectorUIProxy::closeFrontendPageAndWindow): - To avoid relying on the web process for potentially sensitive parameters, I reworked and removed the canAttach and underTest arguments from openLocalInspectorFrontend. These two values are now stored and managed in the UI process instead, instead of being passed from the web process all the time. - For canAttach, I noticed that the WebInspectorUIProxyMac::platformCanAttach method already implements the same logic as the web process's WebInspector::canAttachWindow. I filed https://webkit.org/b/283435 as a follow-up to clean up the webProcessCanAttach parameter, the canAttachWindow function in the web process, and potentially the m_attached field too, which all become obsolete due to this change. - I couldn't figure out what the `if (m_attached)` in canAttachWindow check does, and to me it had no effect, as this function is not called while inspector is open. - For underTest, I'm now letting the test runner directly set the flag on the WebInspectorUIProxy, as part of my fix to address case #3 from above. (WebKit::WebInspectorUIProxy::showConsole): (WebKit::WebInspectorUIProxy::showResources): (WebKit::WebInspectorUIProxy::showMainResourceForFrame): (WebKit::WebInspectorUIProxy::togglePageProfiling): - As the web process can longer call OpenLocalInspectorFrontend, call show/connect/openLocalInspectorFrontend here in the UI process instead. (WebKit::WebInspectorUIProxy::requestOpenLocalInspectorFrontend): - To preserve the open inspector^2 button (case #2 from above), we still maintain this message, but we ignore it unless it's for opening inspector^2, thus renaming the message as a request. This is all assuming that the Web Inspector is not a compromised web process, so we allow that message from it to come through. * Source/WebKit/WebProcess/Inspector/WebInspector.messages.in: * Source/WebKit/WebProcess/Inspector/WebInspector.h: * Source/WebKit/WebProcess/Inspector/WebInspector.cpp: (WebKit::WebInspector::show): - The Show message now takes an async reply, which is used to replace sending WebInspectorUIProxy::OpenLocalInspectorFrontend later. (WebKit::WebInspector::showConsole): (WebKit::WebInspector::showResources): (WebKit::WebInspector::showMainResourceForFrame): (WebKit::WebInspector::startPageProfiling): (WebKit::WebInspector::stopPageProfiling): - Calling inspectorController()->show() no longer does anything, since it's now the UI process's job to show Web Inspector first, for these functions to merely switch to the appropriate tabs. * Source/WebKit/WebProcess/Inspector/WebInspector.cpp: (WebKit::WebInspector::openLocalInspectorFrontend): * Source/WebKit/WebProcess/Inspector/WebInspectorClient.cpp: (WebKit::WebInspectorClient::openLocalFrontend): - Adapt to the command's reworked version. - This is maintained to allow the opening of inspector^2 from the web process (case #2 from above). For opening inspector^1, this message will be ignored by the UI process. * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::contextMenuItemSelected): - When the "Inspect Element" context menu item is selected (case #1 from above), since the web process may not be privileged to open Web Inspector, handle the showing of inspector here in UI process. * Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp: (WTR::TestRunner::showWebInspector): * Tools/WebKitTestRunner/TestInvocation.cpp: (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): * Source/WebKit/UIProcess/API/C/WKPagePrivate.h: * Source/WebKit/UIProcess/API/C/WKPage.cpp: (WKPageShowWebInspectorForTesting): - Preserve letting the WebKitTestRunner open Web Inspector (case #3 from above). - Adapt to the change that we now also let the UI process know about the underTest flag for case #3, rather than letting UI process rely on the value reported by the web process. * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h: * Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp: (WKBundlePageShowInspectorForTest): Deleted. - No longer used due to my special fix for case #3. Originally-landed-as: 283286.537@safari-7620-branch (694a9b5). rdar://144667626 Canonical link: https://commits.webkit.org/290260@main

…n addFloatsToNewParent https://bugs.webkit.org/show_bug.cgi?id=290898 <rdar://143296265> Reviewed by Antti Koivisto. In this patch 1. we let m_floatingObjects go stale on the skipped root (we already do that for the skipped subtree by not running layout) 2. we descend into skipped subtrees while cleaning up floats even when m_floatingObjects is stale/empty Having up-to-date m_floatingObjects on the skipped root, while stale m_floatingObjects on the skipped subtree can lead to issues when (#1) a previously intrusive float (#2) becomes non-intrusive and (#3) eventually gets deleted prevents us from being able to cleanup m_floatingObjects in skipped subtree(s). at #1 m_floatingObjects is populated with the intrusive float (both skipped root and renderers in skipped subtree) and at #2 since we only run layout on the skipped root, m_floatingObjects gets updated by removing this previously intrusive float (skipped subtree becomes stale) and at #3 we don't descend into the skipped subtree to cleanup m_floatingObjects since the skipped root does not have this float anymore (removed at #2). * Source/WebCore/rendering/RenderBlockFlow.cpp: (WebCore::RenderBlockFlow::markSiblingsWithFloatsForLayout): Canonical link: https://commits.webkit.org/293119@main

alice force-pushed the referenceTarget-stages branch 6 times, most recently from 6e120a3 to 9a5437d Compare January 16, 2025 05:25

alice commented Jan 16, 2025

View reviewed changes

twilco reviewed Jan 16, 2025

View reviewed changes

alice added 5 commits January 22, 2025 14:17

Add formForBindings() and controlForBindings() methods.

028f2f1

These methods currently delegate directly to form() and control() respectively, but will later be modified to ensure that nodes don't leak out of shadow roots once referenceTarget is supported.

Use datalist() instead of list() internally.

483d3bf

This reserves list() for use in bindings.

Split getElement{,sArray}Attribute into internal/bindings versions.

74bbfe4

Also use these methods in place of TreeScope::getElementById() for non-bindings code.

Add referenceTarget support and WPT tests.

fe0cce5

Address review comments

23e800a

alice force-pushed the referenceTarget-stages branch from e5c3a14 to 23e800a Compare January 30, 2025 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial attempt at implementing referenceTarget. #3

Initial attempt at implementing referenceTarget. #3

alice commented Jan 14, 2025

alice Jan 16, 2025

twilco Jan 16, 2025

alice Jan 16, 2025

twilco Jan 16, 2025

alice Jan 16, 2025

twilco Jan 16, 2025

alice Jan 16, 2025

alice Jan 16, 2025

twilco Jan 16, 2025

alice Jan 16, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 22, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 22, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

alice Jan 17, 2025

twilco Jan 16, 2025

twilco Jan 16, 2025

twilco commented Jan 22, 2025

Initial attempt at implementing referenceTarget. #3

Are you sure you want to change the base?

Initial attempt at implementing referenceTarget. #3

Conversation

alice commented Jan 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twilco commented Jan 22, 2025