Bug 1838450 - Refine attribute handling. r=emilio

whatwg/dom#1176 https://dom.spec.whatwg.org/#concept-element-attributes-change-ext "attribute change steps" at https://html.spec.whatwg.org/multipage/popover.html#attr-popover Differential Revision: https://phabricator.services.mozilla.com/D181880
mozilla · Jul 4, 2023 · e789f9a · e789f9a
1 parent b7646d1
commit e789f9a
Show file tree

Hide file tree

Showing 2 changed files with 25 additions and 70 deletions.
diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp
@@ -668,45 +668,42 @@ constexpr PopoverAttributeState ToPopoverAttributeState(
 }  // namespace
 
 void nsGenericHTMLElement::AfterSetPopoverAttr() {
-  const nsAttrValue* newValue = GetParsedAttr(nsGkAtoms::popover);
-
-  const PopoverAttributeState newState = [&newValue]() {
-    if (newValue) {
-      MOZ_ASSERT(newValue->Type() == nsAttrValue::eEnum);
+  auto mapPopoverState = [](const nsAttrValue* value) -> PopoverAttributeState {
+    if (value) {
+      MOZ_ASSERT(value->Type() == nsAttrValue::eEnum);
       const auto popoverAttributeKeyword =
-          static_cast<PopoverAttributeKeyword>(newValue->GetEnumValue());
+          static_cast<PopoverAttributeKeyword>(value->GetEnumValue());
       return ToPopoverAttributeState(popoverAttributeKeyword);
     }
 
     // The missing value default is the no popover state, see
     // <https://html.spec.whatwg.org/multipage/popover.html#attr-popover>.
     return PopoverAttributeState::None;
-  }();
+  };
+
+  PopoverAttributeState newState =
+      mapPopoverState(GetParsedAttr(nsGkAtoms::popover));
 
   const PopoverAttributeState oldState = GetPopoverAttributeState();
 
   if (newState != oldState) {
-    EnsurePopoverData().SetPopoverAttributeState(newState);
-
-    HidePopoverInternal(/* aFocusPreviousElement = */ true,
-                        /* aFireEvents = */ true, IgnoreErrors());
-
-    // In case `HidePopoverInternal` changed the state, keep the corresponding
-    // changes and don't overwrite anything here.
-    if (newState == GetPopoverAttributeState()) {
-      if (newState == PopoverAttributeState::None) {
-        // `HidePopoverInternal` above didn't remove the element from the top
-        // layer, because in that call, the element's popover attribute state
-        // was already `None`. Revisit this, when the spec is corrected
-        // (bug 1835811).
-        OwnerDoc()->RemovePopoverFromTopLayer(*this);
-
-        ClearPopoverData();
-        RemoveStates(ElementState::POPOVER_OPEN);
-      } else {
-        // TODO: what if `HidePopoverInternal` called `ShowPopup()`?
-        PopoverPseudoStateUpdate(false, true);
-      }
+    PopoverPseudoStateUpdate(false, true);
+
+    if (IsPopoverOpen()) {
+      HidePopoverInternal(/* aFocusPreviousElement = */ true,
+                          /* aFireEvents = */ true, IgnoreErrors());
+      // Event handlers could have removed the popover attribute, or changed
+      // its value.
+      newState = mapPopoverState(GetParsedAttr(nsGkAtoms::popover));
+    }
+
+    if (newState == PopoverAttributeState::None) {
+      OwnerDoc()->RemovePopoverFromTopLayer(*this);
+      ClearPopoverData();
+      RemoveStates(ElementState::POPOVER_OPEN);
+    } else {
+      // TODO: what if `HidePopoverInternal` called `ShowPopup()`?
+      EnsurePopoverData().SetPopoverAttributeState(newState);
     }
   }
 }

diff --git a/testing/web-platform/meta/html/semantics/popovers/popover-attribute-basic.html.ini b/testing/web-platform/meta/html/semantics/popovers/popover-attribute-basic.html.ini
@@ -14,30 +14,12 @@
   [Changing a popover from auto to undefined (via attr), and then auto during 'beforetoggle' works]
     expected: FAIL
 
-  [Changing a popover from auto to undefined (via attr), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from auto to undefined (via attr), and then invalid during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from auto to undefined (via attr), and then null during 'beforetoggle' works]
-    expected: FAIL
-
   [Changing a popover from manual to auto (via attr), and then auto during 'beforetoggle' works]
     expected: FAIL
 
   [Changing a popover from manual to undefined (via attr), and then auto during 'beforetoggle' works]
     expected: FAIL
 
-  [Changing a popover from manual to undefined (via attr), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to undefined (via attr), and then invalid during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to undefined (via attr), and then null during 'beforetoggle' works]
-    expected: FAIL
-
   [Changing a popover from auto to manual (via idl), and then auto during 'beforetoggle' works]
     expected: FAIL
 
@@ -58,27 +40,3 @@
 
   [Changing a popover from manual to undefined (via idl), and then auto during 'beforetoggle' works]
     expected: FAIL
-
-  [Changing a popover from auto to null (via idl), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from auto to null (via idl), and then invalid during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from auto to undefined (via idl), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from auto to undefined (via idl), and then invalid during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to null (via idl), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to null (via idl), and then invalid during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to undefined (via idl), and then manual during 'beforetoggle' works]
-    expected: FAIL
-
-  [Changing a popover from manual to undefined (via idl), and then invalid during 'beforetoggle' works]
-    expected: FAIL