Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Commit 08eca17

Browse files
ziransunziransun
committed
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
1 parent a9138ad commit 08eca17

File tree

2 files changed

+30
-123
lines changed

2 files changed

+30
-123
lines changed

‎dom/html/nsGenericHTMLElement.cpp

Lines changed: 30 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -652,45 +652,47 @@ constexpr PopoverAttributeState ToPopoverAttributeState(
652652
} // namespace
653653

654654
void nsGenericHTMLElement::AfterSetPopoverAttr() {
655-
const nsAttrValue* newValue = GetParsedAttr(nsGkAtoms::popover);
656-
657-
const PopoverAttributeState newState = [&newValue]() {
658-
if (newValue) {
659-
MOZ_ASSERT(newValue->Type() == nsAttrValue::eEnum);
655+
auto mapPopoverState = [](const nsAttrValue* value) -> PopoverAttributeState {
656+
if (value) {
657+
MOZ_ASSERT(value->Type() == nsAttrValue::eEnum);
660658
const auto popoverAttributeKeyword =
661-
static_cast<PopoverAttributeKeyword>(newValue->GetEnumValue());
659+
static_cast<PopoverAttributeKeyword>(value->GetEnumValue());
662660
return ToPopoverAttributeState(popoverAttributeKeyword);
663661
}
664662

665663
// The missing value default is the no popover state, see
666664
// <https://html.spec.whatwg.org/multipage/popover.html#attr-popover>.
667665
return PopoverAttributeState::None;
668-
}();
666+
};
667+
668+
PopoverAttributeState newState =
669+
mapPopoverState(GetParsedAttr(nsGkAtoms::popover));
669670

670671
const PopoverAttributeState oldState = GetPopoverAttributeState();
671672

672673
if (newState != oldState) {
673-
EnsurePopoverData().SetPopoverAttributeState(newState);
674-
675-
HidePopoverInternal(/* aFocusPreviousElement = */ true,
676-
/* aFireEvents = */ true, IgnoreErrors());
677-
678-
// In case `HidePopoverInternal` changed the state, keep the corresponding
679-
// changes and don't overwrite anything here.
680-
if (newState == GetPopoverAttributeState()) {
681-
if (newState == PopoverAttributeState::None) {
682-
// `HidePopoverInternal` above didn't remove the element from the top
683-
// layer, because in that call, the element's popover attribute state
684-
// was already `None`. Revisit this, when the spec is corrected
685-
// (bug 1835811).
686-
OwnerDoc()->RemovePopoverFromTopLayer(*this);
674+
PopoverPseudoStateUpdate(false, true);
687675

688-
ClearPopoverData();
689-
RemoveStates(ElementState::POPOVER_OPEN);
690-
} else {
691-
// TODO: what if `HidePopoverInternal` called `ShowPopup()`?
692-
PopoverPseudoStateUpdate(false, true);
676+
if (IsPopoverOpen()) {
677+
HidePopoverInternal(/* aFocusPreviousElement = */ true,
678+
/* aFireEvents = */ true, IgnoreErrors());
679+
// Event handlers could have removed the popover attribute, or changed
680+
// its value.
681+
// https://github.com/whatwg/html/issues/9034
682+
newState = mapPopoverState(GetParsedAttr(nsGkAtoms::popover));
683+
}
684+
685+
if (newState == PopoverAttributeState::None) {
686+
// HidePopoverInternal above could have removed the popover from the top
687+
// layer.
688+
if (GetPopoverData()) {
689+
OwnerDoc()->RemovePopoverFromTopLayer(*this);
693690
}
691+
ClearPopoverData();
692+
RemoveStates(ElementState::POPOVER_OPEN);
693+
} else {
694+
// TODO: what if `HidePopoverInternal` called `ShowPopup()`?
695+
EnsurePopoverData().SetPopoverAttributeState(newState);
694696
}
695697
}
696698
}
@@ -3165,17 +3167,12 @@ bool nsGenericHTMLElement::PopoverOpen() const {
31653167
bool nsGenericHTMLElement::CheckPopoverValidity(
31663168
PopoverVisibilityState aExpectedState, Document* aExpectedDocument,
31673169
ErrorResult& aRv) {
3168-
const PopoverData* data = GetPopoverData();
3169-
if (!data ||
3170-
data->GetPopoverAttributeState() == PopoverAttributeState::None) {
3171-
MOZ_ASSERT(!HasAttr(nsGkAtoms::popover));
3170+
if (GetPopoverAttributeState() == PopoverAttributeState::None) {
31723171
aRv.ThrowNotSupportedError("Element is in the no popover state");
31733172
return false;
31743173
}
31753174

3176-
MOZ_ASSERT(HasAttr(nsGkAtoms::popover));
3177-
3178-
if (data->GetPopoverVisibilityState() != aExpectedState) {
3175+
if (GetPopoverData()->GetPopoverVisibilityState() != aExpectedState) {
31793176
return false;
31803177
}
31813178

‎testing/web-platform/meta/html/semantics/popovers/popover-attribute-basic.html.ini

Lines changed: 0 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,94 +1,4 @@
11
[popover-attribute-basic.html]
2-
[Changing a popover from auto to undefined (via attr), and then auto during 'beforetoggle' works]
3-
expected: FAIL
4-
5-
[Changing a popover from auto to undefined (via attr), and then manual during 'beforetoggle' works]
6-
expected: FAIL
7-
8-
[Changing a popover from auto to undefined (via attr), and then invalid during 'beforetoggle' works]
9-
expected: FAIL
10-
11-
[Changing a popover from auto to undefined (via attr), and then null during 'beforetoggle' works]
12-
expected: FAIL
13-
14-
[Changing a popover from manual to undefined (via attr), and then auto during 'beforetoggle' works]
15-
expected: FAIL
16-
17-
[Changing a popover from manual to undefined (via attr), and then manual during 'beforetoggle' works]
18-
expected: FAIL
19-
20-
[Changing a popover from manual to undefined (via attr), and then invalid during 'beforetoggle' works]
21-
expected: FAIL
22-
23-
[Changing a popover from manual to undefined (via attr), and then null during 'beforetoggle' works]
24-
expected: FAIL
25-
26-
[Changing a popover from auto to null (via idl), and then auto during 'beforetoggle' works]
27-
expected: FAIL
28-
29-
[Changing a popover from auto to undefined (via idl), and then auto during 'beforetoggle' works]
30-
expected: FAIL
31-
32-
[Changing a popover from manual to null (via idl), and then auto during 'beforetoggle' works]
33-
expected: FAIL
34-
35-
[Changing a popover from manual to undefined (via idl), and then auto during 'beforetoggle' works]
36-
expected: FAIL
37-
38-
[Changing a popover from auto to null (via idl), and then manual during 'beforetoggle' works]
39-
expected: FAIL
40-
41-
[Changing a popover from auto to null (via idl), and then invalid during 'beforetoggle' works]
42-
expected: FAIL
43-
44-
[Changing a popover from auto to undefined (via idl), and then manual during 'beforetoggle' works]
45-
expected: FAIL
46-
47-
[Changing a popover from auto to undefined (via idl), and then invalid during 'beforetoggle' works]
48-
expected: FAIL
49-
50-
[Changing a popover from manual to null (via idl), and then manual during 'beforetoggle' works]
51-
expected: FAIL
52-
53-
[Changing a popover from manual to null (via idl), and then invalid during 'beforetoggle' works]
54-
expected: FAIL
55-
56-
[Changing a popover from manual to undefined (via idl), and then manual during 'beforetoggle' works]
57-
expected: FAIL
58-
59-
[Changing a popover from manual to undefined (via idl), and then invalid during 'beforetoggle' works]
60-
expected: FAIL
61-
62-
[Changing a popover from auto to undefined (via attr), and then undefined during 'beforetoggle' works]
63-
expected: FAIL
64-
65-
[Changing a popover from manual to undefined (via attr), and then undefined during 'beforetoggle' works]
66-
expected: FAIL
67-
68-
[Changing a popover from auto to null (via idl), and then null during 'beforetoggle' works]
69-
expected: FAIL
70-
71-
[Changing a popover from auto to null (via idl), and then undefined during 'beforetoggle' works]
72-
expected: FAIL
73-
74-
[Changing a popover from auto to undefined (via idl), and then null during 'beforetoggle' works]
75-
expected: FAIL
76-
77-
[Changing a popover from auto to undefined (via idl), and then undefined during 'beforetoggle' works]
78-
expected: FAIL
79-
80-
[Changing a popover from manual to null (via idl), and then null during 'beforetoggle' works]
81-
expected: FAIL
82-
83-
[Changing a popover from manual to null (via idl), and then undefined during 'beforetoggle' works]
84-
expected: FAIL
85-
86-
[Changing a popover from manual to undefined (via idl), and then null during 'beforetoggle' works]
87-
expected: FAIL
88-
89-
[Changing a popover from manual to undefined (via idl), and then undefined during 'beforetoggle' works]
90-
expected: FAIL
91-
922
[Changing a popover from manual to auto (via attr), and then auto during 'beforetoggle' works]
933
expected:
944
if (os == "mac") and not debug: [PASS, FAIL]

0 commit comments

Comments
 (0)