From 13ac6c4fde8645babb61dd8588ba3dc61b8123cc Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Thu, 24 Apr 2025 16:00:14 -0400 Subject: [PATCH] LibWeb: Implement ariaActiveDescendantElement spiritually closer to spec We are meant to store a weak reference to the element indicated by this attribute, rather than a GC-protected strong reference. This also hoists the "get the attr-associated element" AO into its own function, rather than being hidden in IDL, to match "get the attr-associated elements". --- Libraries/LibWeb/ARIA/ARIAMixin.cpp | 18 +++-- Libraries/LibWeb/ARIA/ARIAMixin.h | 17 +++-- Libraries/LibWeb/DOM/Element.cpp | 49 +++++++++++-- Libraries/LibWeb/DOM/Element.h | 1 + .../BindingsGenerator/IDLGenerators.cpp | 69 +++++-------------- 5 files changed, 87 insertions(+), 67 deletions(-) diff --git a/Libraries/LibWeb/ARIA/ARIAMixin.cpp b/Libraries/LibWeb/ARIA/ARIAMixin.cpp index 6cb0da7b659..cb2d4a43a47 100644 --- a/Libraries/LibWeb/ARIA/ARIAMixin.cpp +++ b/Libraries/LibWeb/ARIA/ARIAMixin.cpp @@ -14,11 +14,6 @@ namespace Web::ARIA { ARIAMixin::ARIAMixin() = default; ARIAMixin::~ARIAMixin() = default; -void ARIAMixin::visit_edges(GC::Cell::Visitor& visitor) -{ - visitor.visit(m_aria_active_descendant_element); -} - // https://www.w3.org/TR/wai-aria-1.2/#introroles Optional ARIAMixin::role_from_role_attribute_value() const { @@ -230,6 +225,19 @@ Vector ARIAMixin::parse_id_reference_list(Optional const& id_lis return result; } +#define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ + GC::Ptr ARIAMixin::attribute() const \ + { \ + return m_##attribute.ptr(); \ + } \ + \ + void ARIAMixin::set_##attribute(GC::Ptr value) \ + { \ + m_##attribute = value.ptr(); \ + } +ENUMERATE_ARIA_ELEMENT_REFERENCING_ATTRIBUTES +#undef __ENUMERATE_ARIA_ATTRIBUTE + #define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ Optional>> const& ARIAMixin::attribute() const \ { \ diff --git a/Libraries/LibWeb/ARIA/ARIAMixin.h b/Libraries/LibWeb/ARIA/ARIAMixin.h index 773ab5ebac5..2a928feed04 100644 --- a/Libraries/LibWeb/ARIA/ARIAMixin.h +++ b/Libraries/LibWeb/ARIA/ARIAMixin.h @@ -15,6 +15,9 @@ namespace Web::ARIA { +#define ENUMERATE_ARIA_ELEMENT_REFERENCING_ATTRIBUTES \ + __ENUMERATE_ARIA_ATTRIBUTE(aria_active_descendant_element, aria_active_descendant) + #define ENUMERATE_ARIA_ELEMENT_LIST_REFERENCING_ATTRIBUTES \ __ENUMERATE_ARIA_ATTRIBUTE(aria_controls_elements, aria_controls) \ __ENUMERATE_ARIA_ATTRIBUTE(aria_described_by_elements, aria_described_by) \ @@ -57,8 +60,11 @@ public: // https://www.w3.org/TR/wai-aria-1.2/#valuetype_idref_list Vector parse_id_reference_list(Optional const&) const; - GC::Ptr aria_active_descendant_element() { return m_aria_active_descendant_element; } - void set_aria_active_descendant_element(GC::Ptr value) { m_aria_active_descendant_element = value; } +#define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ + GC::Ptr attribute() const; \ + void set_##attribute(GC::Ptr value); + ENUMERATE_ARIA_ELEMENT_REFERENCING_ATTRIBUTES +#undef __ENUMERATE_ARIA_ATTRIBUTE #define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ Optional>> const& attribute() const; \ @@ -69,12 +75,13 @@ public: protected: ARIAMixin(); - void visit_edges(GC::Cell::Visitor&); - virtual bool id_reference_exists(String const&) const = 0; private: - GC::Ptr m_aria_active_descendant_element; +#define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ + WeakPtr m_##attribute; + ENUMERATE_ARIA_ELEMENT_REFERENCING_ATTRIBUTES +#undef __ENUMERATE_ARIA_ATTRIBUTE #define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ Optional>> m_##attribute; diff --git a/Libraries/LibWeb/DOM/Element.cpp b/Libraries/LibWeb/DOM/Element.cpp index 41873234d7d..963e22c3789 100644 --- a/Libraries/LibWeb/DOM/Element.cpp +++ b/Libraries/LibWeb/DOM/Element.cpp @@ -105,7 +105,6 @@ void Element::visit_edges(Cell::Visitor& visitor) Base::visit_edges(visitor); SlottableMixin::visit_edges(visitor); Animatable::visit_edges(visitor); - ARIAMixin::visit_edges(visitor); visitor.visit(m_attributes); visitor.visit(m_inline_style); @@ -430,6 +429,39 @@ Vector Element::get_attribute_names() const return names; } +// https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#attr-associated-element +GC::Ptr Element::get_the_attribute_associated_element(FlyString const& content_attribute, GC::Ptr explicitly_set_attribute_element) const +{ + // 1. Let element be the result of running reflectedTarget's get the element. + auto const& element = *this; + + // 2. Let contentAttributeValue be the result of running reflectedTarget's get the content attribute. + auto content_attribute_value = element.get_attribute(content_attribute); + + // 3. If reflectedTarget's explicitly set attr-element is not null: + if (explicitly_set_attribute_element) { + // 1. If reflectedTarget's explicitly set attr-element is a descendant of any of element's shadow-including + // ancestors, then return reflectedTarget's explicitly set attr-element. + if (&explicitly_set_attribute_element->root() == &element.shadow_including_root()) + return *explicitly_set_attribute_element; + + // 2. Return null. + return {}; + } + + // 4. Otherwise, if contentAttributeValue is not null, return the first element candidate, in tree order, that meets + // the following criteria: + // * candidate's root is the same as element's root; + // * candidate's ID is contentAttributeValue; and + // * candidate implements T. + if (content_attribute_value.has_value()) + return element.document().get_element_by_id(*content_attribute_value); + + // 5. If no such element exists, then return null. + // 6. Return null. + return {}; +} + // https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#attr-associated-elements Optional>> Element::get_the_attribute_associated_elements(FlyString const& content_attribute, Optional>> const& explicitly_set_attribute_elements) const { @@ -3658,12 +3690,19 @@ void Element::attribute_changed(FlyString const& local_name, Optional co m_dir = Dir::Auto; else m_dir = {}; - } else if (local_name == ARIA::AttributeNames::aria_active_descendant) { - // https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:concept-element-attributes-change-ext - // Set element's explicitly set attr-element to null. - set_aria_active_descendant_element({}); } + // https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:concept-element-attributes-change-ext + // 1. If localName is not attr or namespace is not null, then return. + // 2. Set element's explicitly set attr-element to null. +#define __ENUMERATE_ARIA_ATTRIBUTE(attribute, referencing_attribute) \ + else if (local_name == ARIA::AttributeNames::referencing_attribute && !namespace_.has_value()) \ + { \ + set_##attribute({}); \ + } + ENUMERATE_ARIA_ELEMENT_REFERENCING_ATTRIBUTES +#undef __ENUMERATE_ARIA_ATTRIBUTE + // https://html.spec.whatwg.org/multipage/common-dom-interfaces.html#reflecting-content-attributes-in-idl-attributes:concept-element-attributes-change-ext-2 // 1. If localName is not attr or namespace is not null, then return. // 2. Set element's explicitly set attr-elements to null. diff --git a/Libraries/LibWeb/DOM/Element.h b/Libraries/LibWeb/DOM/Element.h index 155620ae1f6..0944eb77098 100644 --- a/Libraries/LibWeb/DOM/Element.h +++ b/Libraries/LibWeb/DOM/Element.h @@ -163,6 +163,7 @@ public: GC::Ptr get_attribute_node(FlyString const& name) const; GC::Ptr get_attribute_node_ns(Optional const& namespace_, FlyString const& name) const; + GC::Ptr get_the_attribute_associated_element(FlyString const& content_attribute, GC::Ptr explicitly_set_attribute_element) const; Optional>> get_the_attribute_associated_elements(FlyString const& content_attribute, Optional>> const& explicitly_set_attribute_elements) const; DOMTokenList* class_list(); diff --git a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp index 93fabd186c8..46dae457dec 100644 --- a/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp +++ b/Meta/Lagom/Tools/CodeGenerators/LibWeb/BindingsGenerator/IDLGenerators.cpp @@ -4025,53 +4025,16 @@ JS_DEFINE_NATIVE_FUNCTION(@class_name@::@attribute.getter_callback@) retval = MUST(Infra::convert_to_scalar_value_string(*content_attribute_value)); )~~~"); } - // If a reflected IDL attribute has the type T?, where T is either Element - // FIXME: or an interface that inherits from Element, - // then with attr being the reflected content attribute name: + // If a reflected IDL attribute has the type T?, where T is either Element or an interface that inherits + // from Element, then with attr being the reflected content attribute name: + // FIXME: Handle "an interface that inherits from Element". else if (attribute.type->is_nullable() && attribute.type->name() == "Element") { // The getter steps are to return the result of running this's get the attr-associated element. attribute_generator.append(R"~~~( - auto retval = GC::Ptr {}; -)~~~"); + static auto content_attribute = "@attribute.reflect_name@"_fly_string; - // 1. Let element be the result of running reflectedTarget's get the element. - // 2. Let contentAttributeValue be the result of running reflectedTarget's get the content attribute. - attribute_generator.append(R"~~~( - auto contentAttributeValue = impl->attribute("@attribute.reflect_name@"_fly_string); + auto retval = impl->get_the_attribute_associated_element(content_attribute, TRY(throw_dom_exception_if_needed(vm, [&] { return impl->@attribute.cpp_name@(); }))); )~~~"); - // 3. If reflectedTarget's explicitly set attr-element is not null: - // 1. If reflectedTarget's explicitly set attr-element is a descendant of any of element's shadow-including ancestors, then return reflectedTarget's explicitly set attr-element. - // 2. Return null. - attribute_generator.append(R"~~~( - auto const explicitly_set_attr = TRY(throw_dom_exception_if_needed(vm, [&] { return impl->@attribute.cpp_name@(); })); - if (explicitly_set_attr) { - if (&impl->shadow_including_root() == &explicitly_set_attr->root()) { - retval = explicitly_set_attr; - } else { - retval = GC::Ptr {}; - } - } -)~~~"); - // 4. Otherwise, if contentAttributeValue is not null, return the first element candidate, in tree order, that meets the following criteria: - // candidate's root is the same as element's root; - // candidate's ID is contentAttributeValue; and - // FIXME: candidate implements T. - // If no such element exists, then return null. - // 5. Return null. - - // FIXME: This works when T is Element but will need adjustment when we handle subtypes too - attribute_generator.append(R"~~~( - else if (contentAttributeValue.has_value()) { - impl->root().for_each_in_inclusive_subtree_of_type([&](auto& candidate) { - if (candidate.attribute(HTML::AttributeNames::id) == contentAttributeValue.value()) { - retval = &candidate; - return TraversalDecision::Break; - } - return TraversalDecision::Continue; - }); - } -)~~~"); - } // If a reflected IDL attribute has the type FrozenArray?, where T is either Element or an interface that // inherits from Element, then with attr being the reflected content attribute name: @@ -4209,29 +4172,31 @@ JS_DEFINE_NATIVE_FUNCTION(@class_name@::@attribute.setter_callback@) MUST(impl->set_attribute("@attribute.reflect_name@"_fly_string, String::number(cpp_value))); )~~~"); } - // If a reflected IDL attribute has the type T?, where T is either Element - // FIXME: or an interface that inherits from Element, - // then with attr being the reflected content attribute name: + // If a reflected IDL attribute has the type T?, where T is either Element or an interface that inherits + // from Element, then with attr being the reflected content attribute name: + // FIXME: Handle "an interface that inherits from Element". else if (attribute.type->is_nullable() && attribute.type->name() == "Element") { // The setter steps are: // 1. If the given value is null, then: - // 1. Set this's explicitly set attr-element to null. - // 2. Run this's delete the content attribute. - // 3. Return. + // 1. Set this's explicitly set attr-element to null. + // 2. Run this's delete the content attribute. + // 3. Return. attribute_generator.append(R"~~~( + static auto content_attribute = "@attribute.reflect_name@"_fly_string; + if (!cpp_value) { - TRY(throw_dom_exception_if_needed(vm, [&] { return impl->set_@attribute.cpp_name@(nullptr); })); - impl->remove_attribute("@attribute.reflect_name@"_fly_string); + TRY(throw_dom_exception_if_needed(vm, [&] { return impl->set_@attribute.cpp_name@({}); })); + impl->remove_attribute(content_attribute); return JS::js_undefined(); } )~~~"); // 2. Run this's set the content attribute with the empty string. attribute_generator.append(R"~~~( - MUST(impl->set_attribute("@attribute.reflect_name@"_fly_string, String {})); + MUST(impl->set_attribute(content_attribute, String {})); )~~~"); // 3. Set this's explicitly set attr-element to a weak reference to the given value. attribute_generator.append(R"~~~( - TRY(throw_dom_exception_if_needed(vm, [&] { return impl->set_@attribute.cpp_name@(cpp_value); })); + TRY(throw_dom_exception_if_needed(vm, [&] { return impl->set_@attribute.cpp_name@(*cpp_value); })); )~~~"); } // If a reflected IDL attribute has the type FrozenArray?, where T is either Element or an interface