This is related to bug #196843 and it'll cover reflection of attributes that refer to a single Element as a first step. For multiple elements we'll report a separated issue.
Created attachment 458534 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 458541 [details] Patch
Created attachment 458576 [details] Patch
Created attachment 458580 [details] Patch
Comment on attachment 458580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458580&action=review > Source/WebCore/ChangeLog:15 > + This implementation is done following the propsed spec text defined at: typo: propsed > Source/WebCore/dom/Document.cpp:9175 > +ExplicitlySetAttrElementsMap* Document::GetExplicitlySetAttrElementsMap(const Element& element) Getters in WebKit never start with a "get" prefix and never start with a capital letter either. Also, this should return a reference since it cannot return nullptr. Also, given the implementation, maybe this should use the "ensure" prefix, it actually is non-const and populates a map so not a true getter. > Source/WebCore/dom/Document.cpp:9179 > + auto result = m_elementExplicitlySetAttrElementsMap.add(element, WTFMove(map)); this could all be on one line: return m_elementExplicitlySetAttrElementsMap.add(element, ExplicitlySetAttrElementsMap()).iterator->value; > Source/WebCore/dom/Document.h:2285 > + WeakHashMap<Element, ExplicitlySetAttrElementsMap> m_elementExplicitlySetAttrElementsMap; Naming is super confusing, especially in DOM code. "Attr" is a type of Node is DOM and it is NOT an Element. Another comment: What about the case where an Element is adopted into a new Document. That's a thing in DOM but I didn't see code in this patch to properly move the Element from one document's map to another? It is a bit unclear to me why we need a map on the Document and why it cannot simply be in ElementRareData for e.g. > Source/WebCore/dom/Element.cpp:1906 > + if (name == HTMLNames::aria_activedescendantAttr || name == HTMLNames::aria_errormessageAttr) Could all be on one line: return name == HTMLNames::aria_activedescendantAttr || name == HTMLNames::aria_errormessageAttr; > Source/WebCore/dom/Element.cpp:2003 > + auto* element = explicitlySetAttrElementsMap->find(attributeName)->value[0].get(); This does 2 hash table lookups, once for the contains() above and here again for the find. This should be a single find(), resulting in a single lookup. > Source/WebCore/dom/Element.cpp:2010 > + auto id = getAttribute(attributeName); auto& > Source/WebCore/dom/Element.cpp:2029 > + auto id = element->getIdAttribute(); auto& > Source/WebCore/dom/Element.cpp:2037 > + explicitlySetAttrElementsMap->set(attributeName, WTFMove(elements)); explicitlySetAttrElementsMap->set(attributeName, Vector<WeakPtr<Element>> { element }); should work.
Comment on attachment 458580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458580&action=review >> Source/WebCore/dom/Element.cpp:2010 >> + auto id = getAttribute(attributeName); > > auto& As a general rule, we want to be ref'ing an object when it's later used as an argument to another non-trivial function so auto here is good. >> Source/WebCore/dom/Element.cpp:2029 >> + auto id = element->getIdAttribute(); > > auto& Ditto.
Comment on attachment 458580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458580&action=review Thank you very much for the quick reviews, I've applied most changes and uploaded a new version. There's still the question about if this should be in Document or ElementRareData, please let me know your thoughts. >> Source/WebCore/ChangeLog:15 >> + This implementation is done following the propsed spec text defined at: > > typo: propsed Fixed. >> Source/WebCore/dom/Document.cpp:9175 >> +ExplicitlySetAttrElementsMap* Document::GetExplicitlySetAttrElementsMap(const Element& element) > > Getters in WebKit never start with a "get" prefix and never start with a capital letter either. > > Also, this should return a reference since it cannot return nullptr. > > Also, given the implementation, maybe this should use the "ensure" prefix, it actually is non-const and populates a map so not a true getter. Changed to a reference and "ensure" prefix. >> Source/WebCore/dom/Document.cpp:9179 >> + auto result = m_elementExplicitlySetAttrElementsMap.add(element, WTFMove(map)); > > this could all be on one line: > return m_elementExplicitlySetAttrElementsMap.add(element, ExplicitlySetAttrElementsMap()).iterator->value; Done. >> Source/WebCore/dom/Document.h:2285 >> + WeakHashMap<Element, ExplicitlySetAttrElementsMap> m_elementExplicitlySetAttrElementsMap; > > Naming is super confusing, especially in DOM code. "Attr" is a type of Node is DOM and it is NOT an Element. > > Another comment: What about the case where an Element is adopted into a new Document. That's a thing in DOM but I didn't see code in this patch to properly move the Element from one document's map to another? > > It is a bit unclear to me why we need a map on the Document and why it cannot simply be in ElementRareData for e.g. The name comes from the spec text (https://whatpr.org/html/3917/common-dom-interfaces.html): "explicitly set attr-element". Though we might look for a better name indeed. Yeah, there's no test case for the case when we move an Element to a different document, and that's indeed a bug of this implementation. I'll add a new test case checking that. Anyway if we end up moving this to Element that would get fixed. I did this in Document to avoid increasing the size of Element. I didn't thought about ElementRareData initially, I could move this there indeed, but again we'll be increasing the size when we have an static_assert "ElementRareData should stay small". Would that be ok? >> Source/WebCore/dom/Element.cpp:1906 >> + if (name == HTMLNames::aria_activedescendantAttr || name == HTMLNames::aria_errormessageAttr) > > Could all be on one line: > return name == HTMLNames::aria_activedescendantAttr || name == HTMLNames::aria_errormessageAttr; Done. >> Source/WebCore/dom/Element.cpp:2003 >> + auto* element = explicitlySetAttrElementsMap->find(attributeName)->value[0].get(); > > This does 2 hash table lookups, once for the contains() above and here again for the find. This should be a single find(), resulting in a single lookup. Sure, fixed. >> Source/WebCore/dom/Element.cpp:2037 >> + explicitlySetAttrElementsMap->set(attributeName, WTFMove(elements)); > > explicitlySetAttrElementsMap->set(attributeName, Vector<WeakPtr<Element>> { element }); > should work. Done.
Created attachment 458675 [details] Patch
Created attachment 458676 [details] Patch
(In reply to Manuel Rego Casasnovas from comment #8) > Yeah, there's no test case for the case when we move an Element to a > different document, and that's indeed a bug of this implementation. I'll add > a new test case checking that. Anyway if we end up moving this to Element > that would get fixed. I've added a test case for this on the last version of the patch, which is of course failing for now.
Comment on attachment 458676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458676&action=review > Source/WebCore/dom/Document.h:2285 > + WeakHashMap<Element, ExplicitlySetAttrElementsMap> m_elementExplicitlySetAttrElementsMap; Ryosuke, Antti, would it be OK to have a ExplicitlySetAttrElementsMap on ElementRareData instead of having this WeakHashMap here on the Document? This is clearly data that is associated with an Element and I believe it belongs on ElementRareData, not on the Document.
(In reply to Chris Dumez from comment #12) > Comment on attachment 458676 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458676&action=review > > > Source/WebCore/dom/Document.h:2285 > > + WeakHashMap<Element, ExplicitlySetAttrElementsMap> m_elementExplicitlySetAttrElementsMap; > > Ryosuke, Antti, would it be OK to have a ExplicitlySetAttrElementsMap on > ElementRareData instead of having this WeakHashMap here on the Document? > This is clearly data that is associated with an Element and I believe it > belongs on ElementRareData, not on the Document. I discussed it with Antti. Given that it is data associated with the Element and given that it is rare data, ElementRareData is a good match. Let's move it (and update the compile-time assertion about the size, accordingly).
Created attachment 458721 [details] Patch
Created attachment 458730 [details] Patch
(In reply to Chris Dumez from comment #13) > (In reply to Chris Dumez from comment #12) > > Comment on attachment 458676 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=458676&action=review > > > > > Source/WebCore/dom/Document.h:2285 > > > + WeakHashMap<Element, ExplicitlySetAttrElementsMap> m_elementExplicitlySetAttrElementsMap; > > > > Ryosuke, Antti, would it be OK to have a ExplicitlySetAttrElementsMap on > > ElementRareData instead of having this WeakHashMap here on the Document? > > This is clearly data that is associated with an Element and I believe it > > belongs on ElementRareData, not on the Document. > > I discussed it with Antti. Given that it is data associated with the Element > and given that it is rare data, ElementRareData is a good match. Let's move > it (and update the compile-time assertion about the size, accordingly). Ok. Thanks for the help here. I've uploaded a new version using ElementRareData for the map. Which fixes the new test case for adoptNode().
Comment on attachment 458730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458730&action=review This is much better. Still have a few comments (or need clarifications). > Source/WebCore/dom/Element.cpp:1946 > + synchronizeElementReflectionAttribute(name); Could you explain to me in what case this is used? Also, it seems synchronizeElementReflectionAttribute() only removes the attribute name from the map. This is probably correct but I don't understand this part yet. > Source/WebCore/dom/Element.cpp:1989 > + ensureExplicitlySetAttrElementsMap().remove(attributeName); Calling "ensure" just to remove something seems like a waste? > Source/WebCore/dom/Element.cpp:2034 > + auto& map = ensureExplicitlySetAttrElementsMap(); Calling "ensure" in the case where element is null and we'll try to remove something from the map seems like a waste. > Source/WebCore/dom/Element.h:96 > +using ExplicitlySetAttrElementsMap = HashMap<QualifiedName, Vector<WeakPtr<Element>>>; Why is the value a Vector? Looking at the patch, I don't ever see us storing more than one Element in that Vector. Or did I miss something? > Source/WebCore/dom/ElementRareData.h:111 > + void setExplicitlySetAttrElementsMap(std::unique_ptr<ExplicitlySetAttrElementsMap>&& data) { m_explicitlySetAttrElementsMap = WTFMove(data); } Should not be needed. > Source/WebCore/dom/ElementRareData.h:188 > + std::unique_ptr<ExplicitlySetAttrElementsMap> m_explicitlySetAttrElementsMap; This should be a ExplicitlySetAttrElementsMap, not a std::unique_ptr<ExplicitlySetAttrElementsMap>. A HashMap is already just a pointer internally. Using a std::unique_ptr<> here has no benefit on size, is worth for perf and usability in general.
Created attachment 458757 [details] Patch
Comment on attachment 458730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458730&action=review Thanks for the review, uploaded new version. >> Source/WebCore/dom/Element.cpp:1946 >> + synchronizeElementReflectionAttribute(name); > > Could you explain to me in what case this is used? Also, it seems synchronizeElementReflectionAttribute() only removes the attribute name from the map. > This is probably correct but I don't understand this part yet. This is used when you set the attribute manually in JavaScript, like: foo.setAttribute("aria-activedescendant", "bar"); To avoid getting the attribute and the map out of sync, we just remove the map entry here. See the following spec text: > 2. Set element's explicitly set attr-element to null. Anyway I agree that having a method synchronizeElementReflectionAttribute() that just removes things is not easy to understand, so I'm getting rid of that method and just remove the map entry here directly. Note that the map is only filled when the IDL property is used directly like: foo.ariaActiveDescendantElement = bar; >> Source/WebCore/dom/Element.cpp:1989 >> + ensureExplicitlySetAttrElementsMap().remove(attributeName); > > Calling "ensure" just to remove something seems like a waste? Yes, right. I've just removed this method and void the "ensure" call in the new code. >> Source/WebCore/dom/Element.cpp:2034 >> + auto& map = ensureExplicitlySetAttrElementsMap(); > > Calling "ensure" in the case where element is null and we'll try to remove something from the map seems like a waste. True thing, I'll change that. >> Source/WebCore/dom/Element.h:96 >> +using ExplicitlySetAttrElementsMap = HashMap<QualifiedName, Vector<WeakPtr<Element>>>; > > Why is the value a Vector? Looking at the patch, I don't ever see us storing more than one Element in that Vector. Or did I miss something? I thought I explained this in the ChangeLog (sorry I didn't), anyway I'll add some info in the ChangeLog and I can add a FIXME here too. So far this patch is only adding reflection for Element IDL properties: ariaActiveDescendantElement & ariaErrorMessageElement. But we'll be adding this for FrozenArray<Element> properties in a follow-up patch (e.g. ariaDescribedByElements). So it's true that for now we only need to store one element, and we do have an assert about the size at Element::getElementAttribute(). But in the future we could also store a list of elements, that's why I'm adding the Vector already. I wanted to come up with a proper map that would be useful for the future. We could add this in the 2nd patch if you think that's better, but that's why. >> Source/WebCore/dom/ElementRareData.h:111 >> + void setExplicitlySetAttrElementsMap(std::unique_ptr<ExplicitlySetAttrElementsMap>&& data) { m_explicitlySetAttrElementsMap = WTFMove(data); } > > Should not be needed. Removed. >> Source/WebCore/dom/ElementRareData.h:188 >> + std::unique_ptr<ExplicitlySetAttrElementsMap> m_explicitlySetAttrElementsMap; > > This should be a ExplicitlySetAttrElementsMap, not a std::unique_ptr<ExplicitlySetAttrElementsMap>. A HashMap is already just a pointer internally. Using a std::unique_ptr<> here has no benefit on size, is worth for perf and usability in general. Ok, done. Let's see if I got this correctly.
Comment on attachment 458757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458757&action=review > Source/WebCore/dom/ElementRareData.cpp:45 > + void* explicitlySetAttrElementsMap; Right, this will only work on release builds. On Debug builds, HashMaps have more data members because of CHECK_HASHTABLE_ITERATORS. Maybe we should just use HashMap<> here. Otherwise, we won't be able to do the size check in Debug...
Comment on attachment 458757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458757&action=review > Source/WebCore/dom/Element.cpp:1976 > + return rareData.explicitlySetAttrElementsMap(); nit: seems the local is not terribly useful, we should just do: return ensureElementRareData().explicitlySetAttrElementsMap(); > Source/WebCore/dom/Element.cpp:2007 > + ASSERT(element); I am a little unclear why this is true. What makes sure that an element is removed from this Vector before getting destroyed? > Source/WebCore/dom/Element.cpp:2028 > + if (map) nit: I would have done this here: if (auto* map = explicitlySetAttrElementsMap()) and drop the map initialization before the if check. > Source/WebCore/dom/Element.cpp:2042 > + map->set(attributeName, Vector<WeakPtr<Element>> { element }); then `ensureExplicitlySetAttrElementsMap().set()` here. > Source/WebCore/dom/ElementRareData.h:152 > + if (m_explicitlySetAttrElementsMap) How does this work? Shouldn't this be !m_explicitlySetAttrElementsMap.isEmpty() ?
Created attachment 458777 [details] Patch
Comment on attachment 458757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458757&action=review Thanks for the quick review again. Applied suggested changes, PTAL. >> Source/WebCore/dom/Element.cpp:1976 >> + return rareData.explicitlySetAttrElementsMap(); > > nit: seems the local is not terribly useful, we should just do: > return ensureElementRareData().explicitlySetAttrElementsMap(); Done. >> Source/WebCore/dom/Element.cpp:2007 >> + ASSERT(element); > > I am a little unclear why this is true. What makes sure that an element is removed from this Vector before getting destroyed? Right, I guess this could have been destroyed at that point. I'm removing the ASSERT and checking if |element| is not null at that point. >> Source/WebCore/dom/Element.cpp:2028 >> + if (map) > > nit: I would have done this here: > if (auto* map = explicitlySetAttrElementsMap()) > > and drop the map initialization before the if check. Done. >> Source/WebCore/dom/Element.cpp:2042 >> + map->set(attributeName, Vector<WeakPtr<Element>> { element }); > > then `ensureExplicitlySetAttrElementsMap().set()` here. Done. >> Source/WebCore/dom/ElementRareData.cpp:45 >> + void* explicitlySetAttrElementsMap; > > Right, this will only work on release builds. On Debug builds, HashMaps have more data members because of CHECK_HASHTABLE_ITERATORS. > Maybe we should just use HashMap<> here. > > Otherwise, we won't be able to do the size check in Debug... Fixed this. >> Source/WebCore/dom/ElementRareData.h:152 >> + if (m_explicitlySetAttrElementsMap) > > How does this work? Shouldn't this be !m_explicitlySetAttrElementsMap.isEmpty() ? Good catch, this was a leftover from when it was using a pointer.
Comment on attachment 458777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458777&action=review r=me > Source/WebCore/dom/Element.cpp:1904 > +static bool isElementReflectionAttribute(const QualifiedName& name) nit: I would have used `inline` to be extra safe. > Source/WebCore/dom/ElementRareData.h:152 > + if (m_explicitlySetAttrElementsMap.isEmpty()) Shouldn't this be the opposite? if (!m_explicitlySetAttrElementsMap.isEmpty())
Created attachment 458792 [details] Patch
Comment on attachment 458777 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458777&action=review Thanks for the review, uploading patch for landing applying the suggestions. >> Source/WebCore/dom/Element.cpp:1904 >> +static bool isElementReflectionAttribute(const QualifiedName& name) > > nit: I would have used `inline` to be extra safe. Done. >> Source/WebCore/dom/ElementRareData.h:152 >> + if (m_explicitlySetAttrElementsMap.isEmpty()) > > Shouldn't this be the opposite? > if (!m_explicitlySetAttrElementsMap.isEmpty()) True, my bad. Sorry.
Created attachment 458795 [details] Patch
Comment on attachment 458795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458795&action=review > Source/WebCore/dom/Element.cpp:1983 > +bool Element::isDescendantOfShadowIncludingAncestors(const Element& element) const This is just Node::isDescendantOrShadowDescendantOf. Please use that instead. > Source/WebCore/dom/Element.cpp:1986 > + if (!isInTreeScope() || !element.isInTreeScope()) > + return false; So this returns true if and only if a node is in a document tree or a shadow tree? That doesn't make much sense. DOM API typically cares connected-ness. Where in the spec PR does it say such an odd behavior? I don't think it does.
Comment on attachment 458795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458795&action=review > Source/WebCore/dom/Element.cpp:2007 > + if (element && isDescendantOfShadowIncludingAncestors(*element)) > + return element; This check isn't right. We need to be checking that the root node of element matches the root node of this element. Not necessarily that the shadow root matches that of this element. r- because of this. > Source/WebCore/dom/Element.cpp:2032 > + if (!id.isNull() && &treeScope() == &element->treeScope() && treeScope().getElementById(id) == element) We need to check that we're inside a tree scope, or more precisely share the same root node. treeScope() of a disconnected node is Document so this check will work even if both nodes aren't connected. That is, when both nodes are disconnected, this check will succeed even if they don't share the same root node. r- because of this.
Created attachment 458872 [details] Patch
Comment on attachment 458795 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458795&action=review Thanks for the review rniwa, uploaded a new version PTAL. >> Source/WebCore/dom/Element.cpp:1983 >> +bool Element::isDescendantOfShadowIncludingAncestors(const Element& element) const > > This is just Node::isDescendantOrShadowDescendantOf. Please use that instead. Thanks, I didn't found that one before. I'm using that now. >> Source/WebCore/dom/Element.cpp:1986 >> + return false; > > So this returns true if and only if a node is in a document tree or a shadow tree? > That doesn't make much sense. DOM API typically cares connected-ness. > Where in the spec PR does it say such an odd behavior? I don't think it does. This is gone now. >> Source/WebCore/dom/Element.cpp:2007 >> + return element; > > This check isn't right. We need to be checking that the root node of element matches the root node of this element. > Not necessarily that the shadow root matches that of this element. > r- because of this. I've modified this condition and used isDescendantOrShadowDescendantOf() now. >> Source/WebCore/dom/Element.cpp:2032 >> + if (!id.isNull() && &treeScope() == &element->treeScope() && treeScope().getElementById(id) == element) > > We need to check that we're inside a tree scope, or more precisely share the same root node. > treeScope() of a disconnected node is Document so this check will work even if both nodes aren't connected. > That is, when both nodes are disconnected, this check will succeed even if they don't share the same root node. > r- because of this. I'm now comparing rootNode() as the spec says (https://whatpr.org/html/3917/common-dom-interfaces.html): > candidate's root is the same as element's root,
<rdar://problem/92797728>
Comment on attachment 458872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458872&action=review > Source/WebCore/dom/Element.h:679 > + ExplicitlySetAttrElementsMap& ensureExplicitlySetAttrElementsMap(); > + ExplicitlySetAttrElementsMap* explicitlySetAttrElementsMap() const; > + The agreed upon naming convention is: ExplicitlySetAttrElementsMap& explicitlySetAttrElementsMap(); ExplicitlySetAttrElementsMap* explicitlySetAttrElementsMapIfExists() const;
Created attachment 458904 [details] Patch
Comment on attachment 458872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458872&action=review Thanks for the quick review. >> Source/WebCore/dom/Element.h:679 >> + > > The agreed upon naming convention is: > ExplicitlySetAttrElementsMap& explicitlySetAttrElementsMap(); > ExplicitlySetAttrElementsMap* explicitlySetAttrElementsMapIfExists() const; Done.
Committed r293860 (250325@main): <https://commits.webkit.org/250325@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458904 [details].
Reopening to attach new patch.
Created attachment 458917 [details] Patch
Filed a new bug: Bug 240218 – [WinCairo][WK1] accessibility/aria-combobox-control-owns-elements.html is crashing after 250325@main