Reland "[anchor-position] Make anchor attribute work for non-popovers"
#39114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Mar 21, 2023
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}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-4355591
branch
from
1676345 to
9019b29
Compare
marcoscaceres
pushed a commit
that referenced
this pull request
Mar 28, 2023
…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]>
cookiecrook
pushed a commit
to cookiecrook/wpt
that referenced
this pull request
Mar 29, 2023
…rs" (web-platform-tests#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]>
cookiecrook
pushed a commit
to cookiecrook/wpt
that referenced
this pull request
Apr 8, 2023
…rs" (web-platform-tests#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]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
anchorattribute(IDL or content) is modified.
Original change's description:
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}