Skip to content

Commit

Permalink
Check popover stack when buttons are modified
Browse files Browse the repository at this point in the history
The popover stack is constructed by attributes set on buttons.  When
those buttons are modified, it can break connections in the stack. This
patch adds checks to spots where buttons can be modified in order to fix
up the list by closing all popovers when a connection has been broken.

This patch also moves the disabled check for popover*target attributes
which Anne asked for here:
whatwg/html#8221 (comment)

Bug: 1307772, 1408546
Change-Id: I129cf2768abc717292b86ea26f7522019ce36d33
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4115790
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1096920}
  • Loading branch information
josepharhar authored and Chromium LUCI CQ committed Jan 25, 2023
1 parent 859584c commit f886804
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 2 deletions.
Expand Up @@ -158,6 +158,8 @@ void HTMLFormControlElement::DisabledAttributeChanged() {
// Replace |CheckedStateChanged| with a generic tree changed event.
if (AXObjectCache* cache = GetDocument().ExistingAXObjectCache())
cache->CheckedStateChanged(this);

CheckAndPossiblyClosePopoverStack();
}

void HTMLFormControlElement::RequiredAttributeChanged() {
Expand Down Expand Up @@ -244,6 +246,7 @@ Node::InsertionNotificationRequest HTMLFormControlElement::InsertedInto(
void HTMLFormControlElement::RemovedFrom(ContainerNode& insertion_point) {
HTMLElement::RemovedFrom(insertion_point);
ListedElement::RemovedFrom(insertion_point);
CheckAndPossiblyClosePopoverStack();
}

void HTMLFormControlElement::WillChangeForm() {
Expand All @@ -256,6 +259,7 @@ void HTMLFormControlElement::DidChangeForm() {
ListedElement::DidChangeForm();
if (formOwner() && isConnected() && CanBeSuccessfulSubmitButton())
formOwner()->InvalidateDefaultButtonStyle();
CheckAndPossiblyClosePopoverStack();
}

HTMLFormElement* HTMLFormControlElement::formOwner() const {
Expand Down Expand Up @@ -338,7 +342,8 @@ HTMLFormControlElement::popoverTargetElement() {
if (!RuntimeEnabledFeatures::HTMLPopoverAttributeEnabled(
GetDocument().GetExecutionContext()) ||
!IsInTreeScope() ||
SupportsPopoverTriggering() == PopoverTriggerSupport::kNone) {
SupportsPopoverTriggering() == PopoverTriggerSupport::kNone ||
IsDisabledFormControl() || (Form() && IsSuccessfulSubmitButton())) {
return no_element;
}

Expand Down
Expand Up @@ -603,6 +603,8 @@ void HTMLInputElement::UpdateType() {
formOwner() && isConnected())
formOwner()->InvalidateDefaultButtonStyle();
NotifyFormStateChanged();

CheckAndPossiblyClosePopoverStack();
}

void HTMLInputElement::SubtreeHasChanged() {
Expand Down
32 changes: 31 additions & 1 deletion third_party/blink/renderer/core/html/html_element.cc
Expand Up @@ -1775,7 +1775,7 @@ const HTMLElement* NearestInclusiveTargetPopoverForInvoker(const Node* node) {
// popover=auto popovers are considered.
const HTMLElement* HTMLElement::FindTopmostPopoverAncestor(
const HTMLElement& new_popover) {
DCHECK(new_popover.HasPopoverAttribute() && !new_popover.popoverOpen());
DCHECK(new_popover.HasPopoverAttribute());
auto& document = new_popover.GetDocument();
DCHECK(RuntimeEnabledFeatures::HTMLPopoverAttributeEnabled(
document.GetExecutionContext()));
Expand Down Expand Up @@ -1948,6 +1948,36 @@ void HTMLElement::PopoverAnchorElementChanged() {
}
}

void HTMLElement::CheckAndPossiblyClosePopoverStack() {
if (LIKELY(!GetDocument().PopoverAutoShowing())) {
return;
}
// TODO(crbug.com/1307772): We could add more early returns by checking to see
// if the modified element is really a form control that contributed to the
// linking of the popover stack. For example, we could keep track of the set
// of elements which contributed to the current popover stack.
auto& stack = GetDocument().PopoverStack();
for (int i = stack.size() - 1; i > 0; i--) {
if (FindTopmostPopoverAncestor(*stack[i]) != stack[i - 1]) {
auto* console_message = MakeGarbageCollected<ConsoleMessage>(
mojom::blink::ConsoleMessageSource::kOther,
mojom::blink::ConsoleMessageLevel::kWarning,
"The ancestral popover relationship was changed due to a "
"modification to a button with a popover target attribute such as "
"adding the disabled attribute, adding the form attribute, or "
"disconnecting it from the document. All open popovers will be "
"closed.");
console_message->SetNodes(GetDocument().GetFrame(),
{DOMNodeIds::IdForNode(this)});
GetDocument().AddConsoleMessage(console_message);
HTMLElement::HideAllPopoversUntil(
nullptr, GetDocument(), HidePopoverFocusBehavior::kNone,
HidePopoverForcingLevel::kHideImmediately);
return;
}
}
}

void HTMLElement::SetOwnerSelectMenuElement(HTMLSelectMenuElement* element) {
DCHECK(RuntimeEnabledFeatures::HTMLSelectMenuElementEnabled());
DCHECK(RuntimeEnabledFeatures::HTMLPopoverAttributeEnabled(
Expand Down
6 changes: 6 additions & 0 deletions third_party/blink/renderer/core/html/html_element.h
Expand Up @@ -244,6 +244,12 @@ class CORE_EXPORT HTMLElement : public Element {
Document&,
HidePopoverFocusBehavior,
HidePopoverForcingLevel);
// This function checks that the ancestor relationships are still valid for
// the entire popover stack. These can change in various ways, such as a
// triggering element changing its `disabled` attribute. If any relationships
// are invalid, the entire popover stack is closed, and a console warning is
// emitted.
void CheckAndPossiblyClosePopoverStack();

void SetOwnerSelectMenuElement(HTMLSelectMenuElement* element);
HTMLSelectMenuElement* ownerSelectMenuElement() const;
Expand Down
@@ -0,0 +1,126 @@
<!DOCTYPE html>
<link rel=author href="mailto:jarhar@chromium.org">
<link rel=help href="https://github.com/whatwg/html/pull/8221#discussion_r1049379113">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<div id=outerpopover popover=auto>
<button popovertoggletarget=innerpopover disabled>toggle popover</button>
</div>
<div id=innerpopover popover=auto>popover</div>
<script>
test(() => {
outerpopover.showPopover();
innerpopover.showPopover();
assert_true(innerpopover.matches(':open'),
'The inner popover should be able to open successfully.');
assert_false(outerpopover.matches(':open'),
'The outer popover should be closed by opening the inner one.');
}, 'Disabled popover*target buttons should not affect the popover heirarchy.');
</script>

<div id=outerpopover2 popover=auto>
<button id=togglebutton2 popovertoggletarget=innerpopover2>toggle popover</button>
</div>
<div id=innerpopover2 popover=auto>popover</div>
<script>
test(() => {
outerpopover2.showPopover();
innerpopover2.showPopover();
assert_true(innerpopover2.matches(':open'),
'The inner popover should be able to open successfully.');
assert_true(outerpopover2.matches(':open'),
'The outer popover should stay open when opening the inner one.');

togglebutton2.disabled = true;
assert_false(innerpopover2.matches(':open'),
'The inner popover should be closed when the hierarchy is broken.');
assert_false(outerpopover2.matches(':open'),
'The outer popover should be closed when the hierarchy is broken.');
}, 'Disabling popover*target buttons when popovers are open should still cause all popovers to be closed when the formerly outer popover is closed.');
</script>

<div id=outerpopover3 popover=auto>
<button id=togglebutton3 popovertoggletarget=innerpopover3>toggle popover</button>
</div>
<div id=innerpopover3 popover=auto>popover</div>
<script>
test(() => {
outerpopover3.showPopover();
innerpopover3.showPopover();
assert_true(innerpopover3.matches(':open'),
'The inner popover should be able to open successfully.');
assert_true(outerpopover3.matches(':open'),
'The outer popover should stay open when opening the inner one.');

togglebutton3.disabled = true;
assert_false(innerpopover3.matches(':open'),
'The inner popover be should be closed when the hierarchy is broken.');
assert_false(outerpopover3.matches(':open'),
'The outer popover be should be closed when the hierarchy is broken.');
}, 'Disabling popover*target buttons when popovers are open should still cause all popovers to be closed when the formerly inner popover is closed.');
</script>

<div id=outerpopover4 popover=auto>
<button id=togglebutton4 popovertoggletarget=innerpopover4>toggle popover</button>
</div>
<div id=innerpopover4 popover=auto>popover</div>
<form id=submitform>form</form>
<script>
test(() => {
outerpopover4.showPopover();
innerpopover4.showPopover();
assert_true(innerpopover4.matches(':open'),
'The inner popover should be able to open successfully.');
assert_true(outerpopover4.matches(':open'),
'The outer popover should stay open when opening the inner one.');

togglebutton4.setAttribute('form', 'submitform');
assert_false(innerpopover4.matches(':open'),
'The inner popover be should be closed when the hierarchy is broken.');
assert_false(outerpopover4.matches(':open'),
'The outer popover be should be closed when the hierarchy is broken.');
}, 'Setting the form attribute on popover*target buttons when popovers are open should close all popovers.');
</script>

<div id=outerpopover5 popover=auto>
<input type=button id=togglebutton5 popovertoggletarget=innerpopover5>toggle popover</button>
</div>
<div id=innerpopover5 popover=auto>popover</div>
<script>
test(() => {
outerpopover5.showPopover();
innerpopover5.showPopover();
assert_true(innerpopover5.matches(':open'),
'The inner popover should be able to open successfully.');
assert_true(outerpopover5.matches(':open'),
'The outer popover should stay open when opening the inner one.');

togglebutton5.setAttribute('type', 'text');
assert_false(innerpopover5.matches(':open'),
'The inner popover be should be closed when the hierarchy is broken.');
assert_false(outerpopover5.matches(':open'),
'The outer popover be should be closed when the hierarchy is broken.');
}, 'Changing the input type on a popover*target button when popovers are open should close all popovers.');
</script>

<div id=outerpopover6 popover=auto>
<button id=togglebutton6 popovertoggletarget=innerpopover6>toggle popover</button>
</div>
<div id=innerpopover6 popover=auto>popover</div>
<script>
test(() => {
outerpopover6.showPopover();
innerpopover6.showPopover();
assert_true(innerpopover6.matches(':open'),
'The inner popover should be able to open successfully.');
assert_true(outerpopover6.matches(':open'),
'The outer popover should stay open when opening the inner one.');

togglebutton6.remove();
assert_false(innerpopover6.matches(':open'),
'The inner popover be should be closed when the hierarchy is broken.');
assert_false(outerpopover6.matches(':open'),
'The outer popover be should be closed when the hierarchy is broken.');
}, 'Disconnecting popover*target buttons when popovers are open should close all popovers.');
</script>

0 comments on commit f886804

Please sign in to comment.