Skip to content

Commit

Permalink
Reland "[anchor-position] Make anchor attribute work for non-popove…
Browse files Browse the repository at this point in the history
…rs" (#39114)

This reverts commit 80faa9503f457b4a09fa31912eafc7eefd585568.

Reason for reland: Performance regression fixed. See diff with PS#1.

The original patch used an expensive condition to decide whether to
install an AnchorElementObserver, which caused performance issues. This
patch changed to installing observer only when the `anchor` attribute
(IDL or content) is modified.

Original change's description:
> Revert "[anchor-position] Make `anchor` attribute work for non-popovers"
>
> This reverts commit 4abd0d04120561b80ad2e6dd398651d7e909463f.
>
> Reason for revert: Caused a lot of performance regressions.
>
> Original change's description:
> > [anchor-position] Make `anchor` attribute work for non-popovers
> >
> > This patch reimplements how we mark elements that are implicit anchors.
> > Previously it was only marked for popover element's implicit anchors,
> > and now it works for all implicit anchors. This is done by a new class
> > AnchorElementObserver that observes all possible changes in implicit
> > anchor and replaces the old PopoverAnchorObserver.
> >
> > This patch also reveals two existing issues:
> > 1. Element::GetElementAttribute() may return non-null result even if
> >    the element is out of tree scope. As fixing the bug is out of the
> >    scope here, this patch just works around it. crbug.com/1425215 was
> >    filed.
> > 2. popover-anchor-idl-property.html has some bugs that made it pass
> >    previously. This patch fixes those bugs.
> >
> >
> > Fixed: 1417346
> > Change-Id: I798977ab1ed7df0c528b7eb98c64fc04476e6106
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4338487
> > Commit-Queue: Xiaocheng Hu <[email protected]>
> > Reviewed-by: Mason Freed <[email protected]>
> > Cr-Commit-Position: refs/heads/main@{#1118933}
>
> Bug: 1425610, 1425635, 1425650
> Change-Id: I50a650f011e097c48c0efd110cf765d0df5835e4
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4353985
> Bot-Commit: Rubber Stamper <[email protected]>
> Reviewed-by: Mason Freed <[email protected]>
> Commit-Queue: Xiaocheng Hu <[email protected]>
> Cr-Commit-Position: refs/heads/main@{#1119483}

Bug: 1425610, 1425635, 1425650
Change-Id: I222e0cc302e16f208314641b153ce1746ca814a2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4355591
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Xiaocheng Hu <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1121203}

Co-authored-by: Xiaocheng Hu <[email protected]>
  • Loading branch information
chromium-wpt-export-bot and xiaochengh authored Mar 23, 2023
1 parent 87e344f commit fd7ffa7
Showing 1 changed file with 11 additions and 7 deletions.
18 changes: 11 additions & 7 deletions html/semantics/popovers/popover-anchor-idl-property.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>

<button id=b1>This is an anchor button</button>
<div popover id=p1 anchor=b1>This is a popover</div>
<button id=b2 popovertarget=p1>This button invokes the popover but isn't an anchor</button>
<div>
<button id=b1>This is an anchor button</button>
<div popover id=p1 anchor=b1>This is a popover</div>
<button id=b2 popovertarget=p1>This button invokes the popover but isn't an anchor</button>
</div>

<script>
test(function() {
Expand All @@ -24,11 +26,13 @@
}, "popover anchorElement is settable");
</script>

<button id=b1>button</button>
<div id=p2>Anchored div</div>
<div>
<button id=b3>button</button>
<div id=p2>Anchored div</div>
</div>
<style>
* {margin:0;padding:0;}
#b1 {width: 200px;}
#b3 {width: 200px;}
#p2 {
position: absolute;
left: anchor(right);
Expand All @@ -38,7 +42,7 @@
<script>
test(function() {
assert_equals(p2.anchorElement,null);
const button = document.getElementById('b1');
const button = document.getElementById('b3');
assert_true(!!button);
p2.anchorElement = button;
assert_equals(p2.getAttribute('anchor'),'','Idref should be empty after setting element');
Expand Down

0 comments on commit fd7ffa7

Please sign in to comment.