Skip to content

eric fix(#3540): add manual positioning to Popover #3655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrisolsen merged 1 commit into from
Mar 27, 2026
Merged

eric fix(#3540): add manual positioning to Popover #3655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions libs/react-components/specs/app-header-menu.browser.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ describe("AppHeaderMenu", () => {

// Close menu with Escape
await userEvent.keyboard("{Escape}");
const popoverContent = result.getByTestId("popover-content");
await vi.waitFor(() => {
const popoverContent = result.getByTestId("popover-content");
expect(popoverContent.element().checkVisibility()).toBeFalsy();
});

Expand All @@ -66,7 +66,6 @@ describe("AppHeaderMenu", () => {
// 3. Open menu again — should still work after banner is removed from DOM
await menuTrigger.click();
await vi.waitFor(() => {
const popoverContent = result.getByTestId("popover-content");
expect(popoverContent.element().checkVisibility()).toBeTruthy();
});
expect(result.getByTestId("menu-item-1").element().checkVisibility()).toBeTruthy();
Expand Down
102 changes: 91 additions & 11 deletions libs/web-components/src/components/popover/Popover.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@
// Private
let _rootEl: HTMLElement;
let _popoverEl: HTMLElement;
const _needsManualPositioning =
typeof document !== "undefined" &&
!("anchorName" in document.documentElement.style);
let _positionRafId: number | null = null;

// Reactive
let _targetEl: HTMLElement;
Expand Down Expand Up @@ -122,13 +126,67 @@
});

onDestroy(() => {
if (_needsManualPositioning || _positionRafId) {
stopManualPositioning();
}
window.removeEventListener("resize", updateAutoPosition);
// true was passed when the listener was added, so it's necesary to be passed here as well
window.removeEventListener("popstate", handleUrlChange, true);
});

// Functions

function updatePopoverPosition() {
Copy link
Copy Markdown
Collaborator

@ArakTaiRoth ArakTaiRoth Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Safari 18 will ignore Anchor Positioning, but it won't ignore all the translate calls I put in here. So when you're manually calculating the positioning here, the translate calls are still running. This means when someone is using below, the horizontal and vertical offset will be doubled. And when someone is using above, the position of the popover will be twice its height above the element.

if (!_isOpen || !_targetEl || !_popoverEl) return;

const targetRect = _targetEl.getBoundingClientRect();
const xOffset = hoffset ? parseFloat(hoffset) : 0;
const yOffset = voffset ? parseFloat(voffset) : 3;
Comment thread
willcodeforcoffee marked this conversation as resolved.

// Recalculate auto position based on current viewport space
if (position === "auto") {
const popoverRect = _popoverEl.getBoundingClientRect();
const spaceAbove = targetRect.top;
const spaceBelow = window.innerHeight - targetRect.bottom;

_autoPosition =
spaceBelow < popoverRect.height && spaceAbove > spaceBelow
? "above"
: "below";
}

const isAbove =
position === "above" ||
(position === "auto" && _autoPosition === "above");

if (isAbove) {
_popoverEl.style.top = `${targetRect.top - yOffset}px`;
_popoverEl.style.left = `${targetRect.left + xOffset}px`;
_popoverEl.style.transform = "translateY(-100%)";
} else {
_popoverEl.style.top = `${targetRect.bottom + yOffset}px`;
_popoverEl.style.left = `${targetRect.left + xOffset}px`;
_popoverEl.style.transform = "";
}
Comment on lines +158 to +170
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updatePopoverPosition only handles above/below/auto. When position === "right" and anchor positioning isn't supported, the popover will still be positioned below the target (and may retain position-right class styling that no longer makes sense). Manual positioning should include a right branch that matches the CSS-anchor behavior for position-right.

Copilot uses AI. Check for mistakes.
Comment on lines +162 to +170
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual positioning sets top/left and also applies transform, but .popover-content still has the CSS translate: rules (including the position-above -100% shift). In browsers without anchor positioning support this will compound transforms/offsets (e.g., position-above ends up translating by -200% and offsets are effectively applied twice), producing incorrect placement. When _needsManualPositioning is true, disable/override the CSS translate behavior (or rely on it exclusively) so the JS positioning math isn't compounded by existing transform rules.

Copilot uses AI. Check for mistakes.
}

function startManualPositioning() {
if (!_needsManualPositioning) return;

const loop = () => {
updatePopoverPosition();
_positionRafId = requestAnimationFrame(loop);
};
_positionRafId = requestAnimationFrame(loop);
}
Comment on lines +173 to +181
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startManualPositioning() can be called multiple times without checking/canceling an existing loop. If it gets invoked twice (e.g., duplicate toggle handling), the first RAF loop will keep running but its id is lost, causing a leak and extra per-frame work. Add a guard (return if _positionRafId is already set) or call stopManualPositioning() before starting a new loop.

Copilot uses AI. Check for mistakes.

function stopManualPositioning() {
if (_positionRafId !== null) {
cancelAnimationFrame(_positionRafId);
_positionRafId = null;
}
}

function isPopoverOpen(): boolean {
try {
return _popoverEl.matches(":popover-open");
Expand Down Expand Up @@ -188,8 +246,7 @@
}
}

function handleNativeToggle(e: Event) {
const toggleEvent = e as Event & { newState?: "open" | "closed" };
function handleNativeToggle(toggleEvent: ToggleEvent) {
if (toggleEvent.newState === "open") {
_isOpen = true;
} else if (toggleEvent.newState === "closed") {
Expand All @@ -205,15 +262,30 @@
if (_isOpen) {
dispatch(_rootEl, "_open", {}, { bubbles: true });
requestAnimationFrame(updateAutoPosition); // same vs await tick(), make sure popover element is fully rendered before we measure its dimension
if (_needsManualPositioning) {
startManualPositioning();
}
} else {
_targetEl.focus();
if (_needsManualPositioning || _positionRafId) {
stopManualPositioning();
}
_targetEl?.focus();
dispatch(_rootEl, "_close", {}, { bubbles: true });
}
}

function closePopover() {
if (_isOpen) {
_popoverEl?.hidePopover(); // browser will fire and trigger handleNativeToggle
// If the browser doesn't support the API we have to trigger the toggle event manually.
if (_needsManualPositioning) {
const event = new ToggleEvent("toggle", {
bubbles: true,
newState: "closed",
oldState: "open",
});
handleNativeToggle(event); // in case the browser doesn't fire toggle event, we need to manually update the state
Comment thread
willcodeforcoffee marked this conversation as resolved.
}
}
}

Expand All @@ -227,18 +299,19 @@
_popoverEl.hidePopover();
_isOpen = false;
} else {
// If the Popover API is not supported, we need to manually close other
// popovers before opening a new one.
if (_needsManualPositioning) {
dispatch(document.body, "goa:closePopover", { target: _targetEl });
}
Comment thread
chrisolsen marked this conversation as resolved.
_popoverEl.showPopover();
_isOpen = true;
requestAnimationFrame(updateAutoPosition);
}
}

function updateAutoPosition() {
if (!_isOpen || !_targetEl || !_popoverEl) {
return;
}

if (position !== "auto") {
if (position !== "auto" || !_isOpen || !_targetEl || !_popoverEl) {
return;
}

Expand Down Expand Up @@ -298,10 +371,14 @@
class:position-below={position === "below" ||
(position === "auto" && _autoPosition === "below")}
class:position-right={position === "right"}
class:use-anchor-based-positioning={!_needsManualPositioning}
style={styles(
style("width", position !== "right" ? width : undefined),
style("min-width", minwidth),
style("max-width", position !== "right" && width ? `max(${width}, ${maxwidth})` : maxwidth),
style(
"max-width",
position !== "right" && width ? `max(${width}, ${maxwidth})` : maxwidth,
),
style("padding", _padded ? "var(--goa-space-m)" : "0"),
)}
>
Expand Down Expand Up @@ -355,7 +432,6 @@
filter: var(--goa-popover-shadow, none);
border: var(--goa-popover-border, none);
margin: 0;

position-anchor: --goa-popover-target;
inset-block-start: anchor(bottom);
inset-inline-start: anchor(left);
Expand All @@ -364,7 +440,11 @@
translate: var(--popover-translate-x) var(--popover-translate-y);
}

.popover-content.position-above {
.popover-content.use-anchor-based-positioning {
inset-block-start: anchor(top);
--popover-translate-y: calc(-100% - var(--offset-bottom, 3px));
}
.popover-content.use-anchor-based-positioning.position-above {
inset-block-start: anchor(top);
--popover-translate-y: calc(-100% - var(--offset-bottom, 3px));
position-try-fallbacks: none;
Expand Down
Loading