From 4328b1ce9373d761a89f09f84c50115b7ec95ecb Mon Sep 17 00:00:00 2001 From: Mirko Brodesser Date: Mon, 8 May 2023 14:08:37 +0000 Subject: [PATCH] Bug 1829660: pass `true` for `aFireEvents` and defer hiding the popover and updating the popover states. r=emilio Pass `true` for `aFireEvents` to match the spec. Defer hiding the popover because it may run script, which is forbidden, because `SetAttrAndNotify` runs with a script blocker (`mozAutoDocUpdate`). Updated test expectations: The assertion count increased because is violated more often, which was violated before. No new `NS_ASSERTION`s are violated. "[Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works] expected: FAIL" still fails because is violated, which is unrelated to this change. Other instances of still fail too, but the general mechanism of firing events seems to work. Therefore it seems reasonable to investigate the remaining failures in bug 1816851. Differential Revision: https://phabricator.services.mozilla.com/D176660 --- dom/html/nsGenericHTMLElement.cpp | 63 ++++++++++++--------- dom/html/nsGenericHTMLElement.h | 2 + .../popovers/popover-attribute-basic.html.ini | 65 +--------------------- 3 files changed, 39 insertions(+), 91 deletions(-) diff --git a/dom/html/nsGenericHTMLElement.cpp b/dom/html/nsGenericHTMLElement.cpp index 0f7429ac7d4a..98f355792c6e 100644 --- a/dom/html/nsGenericHTMLElement.cpp +++ b/dom/html/nsGenericHTMLElement.cpp @@ -653,6 +653,39 @@ void nsGenericHTMLElement::BeforeSetAttr(int32_t aNamespaceID, nsAtom* aName, aNotify); } +void nsGenericHTMLElement::AfterSetPopoverAttr() { + const nsAttrValue* newValue = GetParsedAttr(nsGkAtoms::popover); + + // https://html.spec.whatwg.org/multipage/popover.html#attr-popover + PopoverState newState; + if (newValue) { + if (newValue->Type() == nsAttrValue::eEnum) { + newState = static_cast(newValue->GetEnumValue()); + } else { + // The invalid value default is the manual state + newState = PopoverState::Manual; + } + } else { + // The missing value default is the no popover state. + newState = PopoverState::None; + } + PopoverState oldState = GetPopoverState(); + if (newState != oldState) { + if (oldState != PopoverState::None) { + HidePopoverInternal(/* aFocusPreviousElement = */ true, + /* aFireEvents = */ true, IgnoreErrors()); + } + // Bug 1831081: `newState` might here differ from `GetPopoverState()`. + if (newState != PopoverState::None) { + EnsurePopoverData().SetPopoverState(newState); + PopoverPseudoStateUpdate(false, true); + } else { + ClearPopoverData(); + RemoveStates(ElementState::POPOVER_OPEN); + } + } +} + void nsGenericHTMLElement::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName, const nsAttrValue* aValue, const nsAttrValue* aOldValue, @@ -667,33 +700,9 @@ void nsGenericHTMLElement::AfterSetAttr(int32_t aNamespaceID, nsAtom* aName, SyncEditorsOnSubtree(this); } else if (aName == nsGkAtoms::popover && StaticPrefs::dom_element_popover_enabled()) { - // https://html.spec.whatwg.org/multipage/popover.html#attr-popover - PopoverState newState; - if (aValue) { - if (aValue->Type() == nsAttrValue::eEnum) { - newState = static_cast(aValue->GetEnumValue()); - } else { - // The invalid value default is the manual state - newState = PopoverState::Manual; - } - } else { - // The missing value default is the no popover state. - newState = PopoverState::None; - } - PopoverState oldState = GetPopoverState(); - if (newState != oldState) { - if (oldState != PopoverState::None) { - HidePopoverInternal(/* aFocusPreviousElement = */ true, - /* aFireEvents = */ false, IgnoreErrors()); - } - if (newState != PopoverState::None) { - EnsurePopoverData().SetPopoverState(newState); - PopoverPseudoStateUpdate(false, true); - } else { - ClearPopoverData(); - RemoveStates(ElementState::POPOVER_OPEN); - } - } + nsContentUtils::AddScriptRunner( + NewRunnableMethod("nsGenericHTMLElement::AfterSetPopoverAttr", this, + &nsGenericHTMLElement::AfterSetPopoverAttr)); } else if (aName == nsGkAtoms::dir) { Directionality dir = eDir_LTR; // A boolean tracking whether we need to recompute our directionality. diff --git a/dom/html/nsGenericHTMLElement.h b/dom/html/nsGenericHTMLElement.h index 221baf92f376..f2c8ffcc485c 100644 --- a/dom/html/nsGenericHTMLElement.h +++ b/dom/html/nsGenericHTMLElement.h @@ -759,6 +759,8 @@ class nsGenericHTMLElement : public nsGenericHTMLElementBase { const nsAttrValue* aOldValue, nsIPrincipal* aMaybeScriptedPrincipal, bool aNotify) override; + MOZ_CAN_RUN_SCRIPT void AfterSetPopoverAttr(); + mozilla::EventListenerManager* GetEventListenerManagerForAttr( nsAtom* aAttrName, bool* aDefer) override; 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 33a3e4c30d8f..ee7d0734da8d 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 @@ -4,7 +4,7 @@ if not debug and (os == "linux") and not fission: [ERROR, OK] if not debug and (os == "mac"): [ERROR, OK] if not debug and (os == "android"): [ERROR, OK] - max-asserts: 15 + max-asserts: 19 [The element
Pop up
should behave as a popover.] expected: FAIL @@ -113,39 +113,12 @@ [Changing a popover from auto to manual (via attr), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from auto to manual (via attr), and then invalid during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to manual (via attr), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to manual (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - [Changing a popover from auto to invalid (via attr), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from auto to invalid (via attr), and then manual during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to invalid (via attr), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to invalid (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - [Changing a popover from auto to null (via attr), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from auto to null (via attr), and then manual during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to null (via attr), and then invalid during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from auto to null (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - [Changing a popover from auto to undefined (via attr), and then auto during 'beforetoggle' works] expected: FAIL @@ -161,18 +134,6 @@ [Changing a popover from manual to auto (via attr), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from manual to auto (via attr), and then manual during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to auto (via attr), and then invalid during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to auto (via attr), and then null during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to auto (via attr), and then undefined during 'beforetoggle' works] - expected: FAIL - [Changing a popover from manual to undefined (via attr), and then auto during 'beforetoggle' works] expected: FAIL @@ -188,9 +149,6 @@ [Changing a popover from auto to manual (via idl), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from auto to manual (via idl), and then invalid during 'beforetoggle' works] - expected: FAIL - [Changing a popover from auto to manual (via idl), and then null during 'beforetoggle' works] expected: FAIL @@ -200,9 +158,6 @@ [Changing a popover from auto to invalid (via idl), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from auto to invalid (via idl), and then manual during 'beforetoggle' works] - expected: FAIL - [Changing a popover from auto to invalid (via idl), and then null during 'beforetoggle' works] expected: FAIL @@ -212,12 +167,6 @@ [Changing a popover from auto to null (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 null (via idl), and then null during 'beforetoggle' works] expected: FAIL @@ -227,12 +176,6 @@ [Changing a popover from auto to undefined (via idl), and then auto 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 auto to undefined (via idl), and then null during 'beforetoggle' works] expected: FAIL @@ -242,12 +185,6 @@ [Changing a popover from manual to auto (via idl), and then auto during 'beforetoggle' works] expected: FAIL - [Changing a popover from manual to auto (via idl), and then manual during 'beforetoggle' works] - expected: FAIL - - [Changing a popover from manual to auto (via idl), and then invalid during 'beforetoggle' works] - expected: FAIL - [Changing a popover from manual to auto (via idl), and then null during 'beforetoggle' works] expected: FAIL -- 2.11.4.GIT