From 08eca17319718d38c9054df603f144bf98ff59b3 Mon Sep 17 00:00:00 2001 From: Ziran Sun Date: Fri, 6 Oct 2023 08:06:54 +0000 Subject: [PATCH] Bug 1838450 - Refine attribute handling. r=emilio https://github.com/whatwg/dom/pull/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 --- dom/html/nsGenericHTMLElement.cpp | 59 ++++++------ .../popovers/popover-attribute-basic.html.ini | 90 ------------------- 2 files changed, 28 insertions(+), 121 deletions(-) diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp index 68d7d255131c..6a2a5b08822e 100644 --- a/dom/html/nsGenericHTMLElement.cpp +++ b/dom/html/nsGenericHTMLElement.cpp @@ -652,45 +652,47 @@ 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(newValue->GetEnumValue()); + static_cast(value->GetEnumValue()); return ToPopoverAttributeState(popoverAttributeKeyword); } // The missing value default is the no popover state, see // . return PopoverAttributeState::None; - }(); + }; + + PopoverAttributeState newState = + mapPopoverState(GetParsedAttr(nsGkAtoms::popover)); const PopoverAttributeState oldState = GetPopoverAttributeState(); if (newState != oldState) { - EnsurePopoverData().SetPopoverAttributeState(newState); + PopoverPseudoStateUpdate(false, true); - HidePopoverInternal(/* aFocusPreviousElement = */ true, - /* aFireEvents = */ true, IgnoreErrors()); + if (IsPopoverOpen()) { + HidePopoverInternal(/* aFocusPreviousElement = */ true, + /* aFireEvents = */ true, IgnoreErrors()); + // Event handlers could have removed the popover attribute, or changed + // its value. + // https://github.com/whatwg/html/issues/9034 + newState = mapPopoverState(GetParsedAttr(nsGkAtoms::popover)); + } - // 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). + if (newState == PopoverAttributeState::None) { + // HidePopoverInternal above could have removed the popover from the top + // layer. + if (GetPopoverData()) { OwnerDoc()->RemovePopoverFromTopLayer(*this); - - ClearPopoverData(); - RemoveStates(ElementState::POPOVER_OPEN); - } else { - // TODO: what if `HidePopoverInternal` called `ShowPopup()`? - PopoverPseudoStateUpdate(false, true); } + ClearPopoverData(); + RemoveStates(ElementState::POPOVER_OPEN); + } else { + // TODO: what if `HidePopoverInternal` called `ShowPopup()`? + EnsurePopoverData().SetPopoverAttributeState(newState); } } } @@ -3165,17 +3167,12 @@ bool nsGenericHTMLElement::PopoverOpen() const { bool nsGenericHTMLElement::CheckPopoverValidity( PopoverVisibilityState aExpectedState, Document* aExpectedDocument, ErrorResult& aRv) { - const PopoverData* data = GetPopoverData(); - if (!data || - data->GetPopoverAttributeState() == PopoverAttributeState::None) { - MOZ_ASSERT(!HasAttr(nsGkAtoms::popover)); + if (GetPopoverAttributeState() == PopoverAttributeState::None) { aRv.ThrowNotSupportedError("Element is in the no popover state"); return false; } - MOZ_ASSERT(HasAttr(nsGkAtoms::popover)); - - if (data->GetPopoverVisibilityState() != aExpectedState) { + if (GetPopoverData()->GetPopoverVisibilityState() != aExpectedState) { return false; } 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 index fdfe1f3da85f..69ab227520f4 100644 --- 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 @@ -1,94 +1,4 @@ [popover-attribute-basic.html] - [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 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 null (via idl), and then auto during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to undefined (via idl), and then auto during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to null (via idl), and then auto during 'beforetoggle' works] - expected: FAIL - - [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 - - [Changing a popover from auto to undefined (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to undefined (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to null (via idl), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to null (via idl), and then undefined during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to undefined (via idl), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to undefined (via idl), and then undefined during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to null (via idl), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to null (via idl), and then undefined during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to undefined (via idl), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to undefined (via idl), and then undefined during 'beforetoggle' works] - expected: FAIL - [Changing a popover from manual to auto (via attr), and then auto during 'beforetoggle' works] expected: if (os == "mac") and not debug: [PASS, FAIL]