Skip to content

Commit

Permalink
Collect popover invokers on root instead of document
Browse files Browse the repository at this point in the history
See this HTML spec PR:

  whatwg/html#8993

It points out that it would be better to look within `root`
rather than `document` for invokers. Otherwise this use case
will be broken:

<div>
  <template shadowrootmode=open>
    <button popovertarget=p1>Click</button>
    <div popover id=p1>Popover 1
      <button popovertarget=p2>Click</button>
    </div>
    <div popover id=p2>Popover 2 (I'm not detected as a being related to p1)</div>
  </template>
</div>

I.e. nested popovers entirely within a shadow tree. This CL
fixes that behavior and adds a test.

Bug: 1307772
Change-Id: I28521ec1008d43994ca738c5673da3b704d7ba9c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4335444
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1118671}
  • Loading branch information
Mason Freed authored and Chromium LUCI CQ committed Mar 17, 2023
1 parent be795e5 commit ad83a39
Show file tree
Hide file tree
Showing 7 changed files with 47 additions and 12 deletions.
6 changes: 6 additions & 0 deletions third_party/blink/renderer/core/dom/container_node.h
Expand Up @@ -27,6 +27,7 @@

#include "third_party/blink/public/mojom/input/focus_type.mojom-blink-forward.h"
#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/css/css_value.h"
#include "third_party/blink/renderer/core/css/style_recalc_change.h"
#include "third_party/blink/renderer/core/dom/node.h"
#include "third_party/blink/renderer/core/dom/static_node_list.h"
Expand Down Expand Up @@ -408,6 +409,11 @@ class CORE_EXPORT ContainerNode : public Node {

Element* GetAutofocusDelegate() const;

HTMLCollection* PopoverInvokers() {
DCHECK(IsTreeScope());
return EnsureCachedCollection<HTMLCollection>(kPopoverInvokers);
}

void Trace(Visitor*) const override;

protected:
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/renderer/core/dom/document.cc
Expand Up @@ -7253,10 +7253,6 @@ HTMLCollection* Document::DocumentAllNamedItems(const AtomicString& name) {
kDocumentAllNamedItems, name);
}

HTMLCollection* Document::PopoverInvokers() {
return EnsureCachedCollection<HTMLCollection>(kPopoverInvokers);
}

void Document::IncrementLazyAdsFrameCount() {
data_->lazy_ads_frame_count_++;
}
Expand Down
1 change: 0 additions & 1 deletion third_party/blink/renderer/core/dom/document.h
Expand Up @@ -548,7 +548,6 @@ class CORE_EXPORT Document : public ContainerNode,
HTMLCollection* WindowNamedItems(const AtomicString& name);
DocumentNameCollection* DocumentNamedItems(const AtomicString& name);
HTMLCollection* DocumentAllNamedItems(const AtomicString& name);
HTMLCollection* PopoverInvokers();

// The unassociated listed elements are listed elements that are not
// associated to a <form> element.
Expand Down
4 changes: 0 additions & 4 deletions third_party/blink/renderer/core/dom/live_node_list_base.h
Expand Up @@ -83,10 +83,6 @@ class CORE_EXPORT LiveNodeListBase : public GarbageCollectedMixin {
protected:
Document& GetDocument() const { return owner_node_->GetDocument(); }

ALWAYS_INLINE NodeListSearchRoot SearchRoot() const {
return static_cast<NodeListSearchRoot>(search_root_);
}

template <typename MatchFunc>
static Element* TraverseMatchingElementsForwardToOffset(
Element& current_element,
Expand Down
3 changes: 2 additions & 1 deletion third_party/blink/renderer/core/html/html_collection.cc
Expand Up @@ -111,13 +111,14 @@ static NodeListSearchRoot SearchRootFromCollectionType(
case kSelectedOptions:
case kDataListOptions:
case kMapAreas:
case kPopoverInvokers:
return NodeListSearchRoot::kOwnerNode;
case kFormControls:
if (IsA<HTMLFieldSetElement>(owner))
return NodeListSearchRoot::kOwnerNode;
DCHECK(IsA<HTMLFormElement>(owner));
return NodeListSearchRoot::kTreeScope;
case kPopoverInvokers:
return NodeListSearchRoot::kTreeScope;
case kNameNodeListType:
case kRadioNodeListType:
case kRadioImgNodeListType:
Expand Down
6 changes: 4 additions & 2 deletions third_party/blink/renderer/core/html/html_element.cc
Expand Up @@ -1391,7 +1391,8 @@ void MarkPopoverInvokersDirty(const HTMLElement& popover) {
if (!cache) {
return;
}
for (auto* invoker_candidate : *document.PopoverInvokers()) {
for (auto* invoker_candidate :
*popover.GetTreeScope().RootNode().PopoverInvokers()) {
auto* invoker = To<HTMLFormControlElement>(invoker_candidate);
if (popover == invoker->popoverTargetElement().popover) {
cache->MarkElementDirty(invoker);
Expand Down Expand Up @@ -1959,7 +1960,8 @@ const HTMLElement* HTMLElement::FindTopmostPopoverAncestor(
// 2. Anchor attribute.
check_ancestor(new_popover.anchorElement());
// 3. Invoker to popover (need to consider all of them).
for (auto* invoker : *document.PopoverInvokers()) {
for (auto* invoker :
*new_popover.GetTreeScope().RootNode().PopoverInvokers()) {
DCHECK(IsA<HTMLFormControlElement>(invoker));
auto* popover = To<HTMLFormControlElement>(invoker)
->popoverTargetElement()
Expand Down
Expand Up @@ -5,6 +5,7 @@
<link rel=help href="https://html.spec.whatwg.org/multipage/popover.html">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/resources/declarative-shadow-dom-polyfill.js"></script>
<script src="resources/popover-utils.js"></script>

<script>
Expand Down Expand Up @@ -167,3 +168,37 @@
assert_false(isElementVisible(popover2));
}, "The popover stack is preserved across shadow-inclusive ancestors");
</script>


<div id=test5>
<template shadowrootmode=open>
<button popovertarget=p1>Test 5 Popover 1</button>
<div popover id=p1>Popover 1
<p>This should not get hidden when popover2 opens.</p>
<button popovertarget=p2>Click</button>
</div>
<div popover id=p2>Popover 2
<p>This should not hide popover1.</p>
</div>
</template>
</div>
<script>
promise_test(async function() {
polyfill_declarative_shadow_dom(test5);
const [popover1,popover2] = getPopoverReferences('test5');
popover1.showPopover();
popover2.showPopover();
// Both 1 and 2 should be open at this point.
assert_true(popover1.matches(':open'), 'popover1 not open');
assert_true(isElementVisible(popover1));
assert_true(popover2.matches(':open'), 'popover2 not open');
assert_true(isElementVisible(popover2));
// This should hide both of them.
popover1.hidePopover();
await waitForRender();
assert_false(popover1.matches(':open'));
assert_false(isElementVisible(popover1));
assert_false(popover2.matches(':open'));
assert_false(isElementVisible(popover2));
}, "Popover ancestor relationships are within a root, not within the document");
</script>

0 comments on commit ad83a39

Please sign in to comment.