From 5ec76331e8dc5080abe547550b0bcad5e91e27c4 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Sat, 2 Sep 2023 10:05:15 -0400 Subject: [PATCH] LibWeb: Invoke our internal attribute change handler from Attr Currently, every public DOM::Element method which changes an attribute fires this handler itself. This was missed in commit 720f7ba, so any user of that API would not fire the internal handler. To fix this, and prevent any missing invocations in the future, invoke the handler from from Attr::handle_attribute_changes. This method is reached for all attribute changes, including adding/removing attributes. This ensures the handler will always be fired, and reduces the footprint of this ad-hoc behavior. Note that our ad-hoc handler is not the "attribute change steps" noted by the spec. Those are overridden only by a couple of specific elements, e.g. HTMLSlotElement. However, we could easily make our ad-hoc handler hook into those steps in the future. --- Userland/Libraries/LibWeb/DOM/Attr.cpp | 3 +- Userland/Libraries/LibWeb/DOM/Element.cpp | 41 ++++++++++------------- Userland/Libraries/LibWeb/DOM/Element.h | 2 ++ 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/Userland/Libraries/LibWeb/DOM/Attr.cpp b/Userland/Libraries/LibWeb/DOM/Attr.cpp index f7210c9e11..9f35f36b67 100644 --- a/Userland/Libraries/LibWeb/DOM/Attr.cpp +++ b/Userland/Libraries/LibWeb/DOM/Attr.cpp @@ -110,7 +110,8 @@ void Attr::handle_attribute_changes(Element& element, DeprecatedString const& ol element.enqueue_a_custom_element_callback_reaction(HTML::CustomElementReactionNames::attributeChangedCallback, move(arguments)); } - // FIXME: 3. Run the attribute change steps with element, attribute’s local name, oldValue, newValue, and attribute’s namespace. + // 3. Run the attribute change steps with element, attribute’s local name, oldValue, newValue, and attribute’s namespace. + element.run_attribute_change_steps(local_name(), old_value, new_value, namespace_uri()); } } diff --git a/Userland/Libraries/LibWeb/DOM/Element.cpp b/Userland/Libraries/LibWeb/DOM/Element.cpp index 2ef6572fdd..b6e8412576 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.cpp +++ b/Userland/Libraries/LibWeb/DOM/Element.cpp @@ -145,27 +145,17 @@ WebIDL::ExceptionOr Element::set_attribute(DeprecatedFlyString const& name // 3. Let attribute be the first attribute in this’s attribute list whose qualified name is qualifiedName, and null otherwise. auto* attribute = m_attributes->get_attribute(name); - DeprecatedString old_value; - - // 4. If attribute is null, create an attribute whose local name is qualifiedName, value is value, and node document is this’s node document, then append this attribute to this, and then return. + // 4. If attribute is null, create an attribute whose local name is qualifiedName, value is value, and node document + // is this’s node document, then append this attribute to this, and then return. if (!attribute) { auto new_attribute = Attr::create(document(), insert_as_lowercase ? name.to_lowercase() : name, value); m_attributes->append_attribute(new_attribute); - attribute = new_attribute.ptr(); + return {}; } // 5. Change attribute to value. - else { - old_value = attribute->value(); - attribute->change_attribute(value); - } - - attribute_changed(attribute->local_name(), value); - - if (value != old_value) { - invalidate_style_after_attribute_change(name); - } + attribute->change_attribute(value); return {}; } @@ -264,9 +254,8 @@ WebIDL::ExceptionOr> Element::set_attribute_node_ns(Attr& attr) // https://dom.spec.whatwg.org/#dom-element-removeattribute void Element::remove_attribute(DeprecatedFlyString const& name) { + // The removeAttribute(qualifiedName) method steps are to remove an attribute given qualifiedName and this, and then return undefined. m_attributes->remove_attribute(name); - attribute_changed(name, {}); - invalidate_style_after_attribute_change(name); } // https://dom.spec.whatwg.org/#dom-element-hasattribute @@ -303,15 +292,12 @@ WebIDL::ExceptionOr Element::toggle_attribute(DeprecatedFlyString const& n // 4. If attribute is null, then: if (!attribute) { - // 1. If force is not given or is true, create an attribute whose local name is qualifiedName, value is the empty string, and node document is this’s node document, then append this attribute to this, and then return true. + // 1. If force is not given or is true, create an attribute whose local name is qualifiedName, value is the empty + // string, and node document is this’s node document, then append this attribute to this, and then return true. if (!force.has_value() || force.value()) { auto new_attribute = Attr::create(document(), insert_as_lowercase ? name.to_lowercase() : name, ""); m_attributes->append_attribute(new_attribute); - attribute_changed(new_attribute->local_name(), ""); - - invalidate_style_after_attribute_change(name); - return true; } @@ -322,9 +308,6 @@ WebIDL::ExceptionOr Element::toggle_attribute(DeprecatedFlyString const& n // 5. Otherwise, if force is not given or is false, remove an attribute given qualifiedName and this, and then return false. if (!force.has_value() || !force.value()) { m_attributes->remove_attribute(name); - attribute_changed(name, {}); - invalidate_style_after_attribute_change(name); - return false; } @@ -410,6 +393,16 @@ CSS::CSSStyleDeclaration const* Element::inline_style() const return m_inline_style.ptr(); } +void Element::run_attribute_change_steps(DeprecatedFlyString const& local_name, DeprecatedString const&, DeprecatedString const& value, DeprecatedFlyString const&) +{ + // FIXME: Implement the element's attribute change steps: + // https://dom.spec.whatwg.org/#concept-element-attributes-change-ext + + // AD-HOC: Run our own internal attribute change handler. + attribute_changed(local_name, value); + invalidate_style_after_attribute_change(local_name); +} + void Element::attribute_changed(DeprecatedFlyString const& name, DeprecatedString const& value) { if (name == HTML::AttributeNames::class_) { diff --git a/Userland/Libraries/LibWeb/DOM/Element.h b/Userland/Libraries/LibWeb/DOM/Element.h index 9e3821ca12..512f578889 100644 --- a/Userland/Libraries/LibWeb/DOM/Element.h +++ b/Userland/Libraries/LibWeb/DOM/Element.h @@ -128,6 +128,8 @@ public: Vector const& class_names() const { return m_classes; } virtual void apply_presentational_hints(CSS::StyleProperties&) const { } + + void run_attribute_change_steps(DeprecatedFlyString const& local_name, DeprecatedString const& old_value, DeprecatedString const& value, DeprecatedFlyString const& namespace_); virtual void attribute_changed(DeprecatedFlyString const& name, DeprecatedString const& value); struct [[nodiscard]] RequiredInvalidationAfterStyleChange {