[#279] Fix position-try demos and polyfill logic#424
Conversation
✅ Deploy Preview for anchor-polyfill ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for anchor-position-wpt canceled.
|
| const anchorPosition: AnchorPosition = {}; | ||
| if (order) { | ||
| anchorPosition.order = order; | ||
| } |
There was a problem hiding this comment.
Claude's explanation of this change:
In the polyfill, a comma-separated selector list that uses position-try-fallbacks (e.g. #a, #b { position-try-fallbacks: flip-block, … }) caused the second target to be positioned using the first target's fallbacks. Visibly, in Firefox the second target stayed static (un-anchored) and targets could revert to a non-anchored position when a fallback couldn't be placed.
Cause
In parsePositionFallbacks, the anchorPosition object was declared outside the selectors.forEach loop, so it was shared across every selector in the list. Because try-tactic fallbacks are keyed per selector (${selector}-${tactics}), each selector's fallbacks accumulated into that one shared object — and since validPositions[selectorA] and validPositions[selectorB] both referenced it, the second target's fallback list began with the first target's fallbacks (which resolve against the wrong anchor). @position-try (at-rule) fallbacks were unaffected because their names are shared across selectors.
Fix
Give each selector its own anchorPosition, and separate the two concerns that were conflated under a single dedup guard:
- Add a fallback to this selector's position — now tracked per selector.
- Inject the generated
@position-tryrule into the stylesheet — still deduped once per stylesheet.
Tests
Added a regression unit test asserting that each selector in a shared rule gets only its own try-tactic fallback. Existing unit (186) and e2e (37) suites pass.
| CSS.registerProperty({ | ||
| name: customProperty, | ||
| syntax: '*', | ||
| inherits: false, | ||
| }); |
There was a problem hiding this comment.
TIL about CSS.registerProperty 🎉
There was a problem hiding this comment.
I was avoiding relying on CSS.registerProperty - it moves Safari support from Safari 10 to Safari 16.4 (Release date: 2023-03-27), and Firefox Support from Firefox 54 to Firefox 128 (Release date: 2024-07-09).
We would need to add this as a caveat. I'm not sure there's another way...
jamesnw
left a comment
jamesnw
left a comment
There was a problem hiding this comment.
Some testing notes-
In Firefox 124- Demo doesn't work. Adding padding (removing .tight from .demo-elements) makes it work, but the target goes outside of the scroll container before falling back. When it falls back, it still has the inheritance bug (CSS.registerProperty came to Firefox in 128). Fallback only happens on the y-axis.
In Firefox 128- Demo works. Adding padding makes the the targets go outside of the scroll container before falling back.
I'm not sure if this is because the custom properties are required to make it fallback correctly, or if the incorrect values are the making the fallback happen. We should figure out how crucial non-inheritance is to this change, and see if there's an alternate way of addressing that.
| * this only needs to run once. | ||
| */ | ||
| let propertiesRegistered = false; | ||
| export function registerShiftedProperties() { |
There was a problem hiding this comment.
Instead of eagerly registering all properties, would it make sense to check if it's registered in shiftUnsupportedProperties first, and only registering those?
| CSS.registerProperty({ | ||
| name: customProperty, | ||
| syntax: '*', | ||
| inherits: false, | ||
| }); |
There was a problem hiding this comment.
I was avoiding relying on CSS.registerProperty - it moves Safari support from Safari 10 to Safari 16.4 (Release date: 2023-03-27), and Firefox Support from Firefox 54 to Firefox 128 (Release date: 2024-07-09).
We would need to add this as a caveat. I'm not sure there's another way...
| content. The wrapper is the target's containing block, so this does not | ||
| affect where fallbacks are evaluated. */ | ||
| #position-try-tactics-combined .demo-elements { | ||
| overflow: clip; |
There was a problem hiding this comment.
It seems like this shouldn't be required to have fallback work within a scrollport, but it seems to be the case. This is repeated a lot, I'm wondering if we should make it a class?
Also, the lack of padding on the elements (tight) is essential to making this pattern work- I think moving it in here would make sense.
| @@ -1,7 +1,5 @@ | |||
| import { | |||
| autoUpdate, | |||
| detectOverflow, | |||
There was a problem hiding this comment.
Nice, this was simple enough to replace. I wonder if this reduces the polyfill size, and if there are other simple-to-replace parts that would help with that?
2 participants
Resolves #279
Background
The
position-trydemos didn't update on scroll, and were broken in native browsers too, not just under the polyfill. Investigating in real native Chrome (150) confirmed two distinct problems:Demo authoring bug (affects native). Per spec,
position-tryoptions are evaluated by checking whether the target overflows its inset-modified containing block — not the scrollport of an ancestor scroll container. Every demo target's containing block was its.scroll-container(position: relative), which also contains the anchor. So scrolling moved the anchor and target together and the target never overflowed its containing block → fallbacks never triggered, natively or otherwise. The polyfill only appeared to work because it measured overflow against the scrollport, diverging from native.Polyfill inheritance leak. Properties shifted into custom properties (
height, insets, margins,anchor-name, …) are all non-inherited in CSS, but custom properties inherit by default. Soheight: 400pxon a.scroll-containerleaked through--height-<uuid>into every descendant target and got stamped into every generated fallback.Changes
Demos (
public/position-try*.css): make the position-try scroll containersposition: staticso each target's containing block is promoted to the surrounding.demo-elementswrapper (an ancestor outside the scroller). Now scrolling pushes the target to overflow and fallbacks trigger — natively and under the polyfill. Addedoverflow: clipon the wrapper so a target whose anchor has scrolled out of view doesn't float over other content, and gave the combined demo's anchor an offset so its base position fits at rest. Removed the "this example is broken" warning inindex.html.Non-inherited custom properties (
src/cascade.ts,src/dom.ts): register the shifted custom properties withCSS.registerProperty({ syntax: '*', inherits: false })so a value set on an ancestor is no longer read back as if it were set directly on a descendant. Registration is global and runs once; it no-ops whereCSS.registerPropertyis unavailable. Updated the now-inaccurate inheritance comments.Overflow basis (
src/polyfill.ts): rewrotecheckOverflowto measure the target's margin-box against its containing block (offsetParent) usinggetBoundingClientRect, matching the spec/native behavior instead of floating-ui's scrollport-baseddetectOverflow. Both rects are read in viewport coordinates, so the page's scroll offset cancels out (the previous standalonedetectOverflowcall produced wildly wrong values once the containing block was no longer a near-viewport scroll container). Dropped the now-unuseddetectOverflow/MiddlewareStateimports andplatformWithCache.Test (
tests/e2e/polyfill.test.ts): the target'soffsetParentis now the (non-scrollable) wrapper, so the@position-fallbacktest scrolls.scroll-containerdirectly and captures the pre-polyfill position rather than hardcoding it.Behavior change to flag for review
checkOverflownow matches native semantics (overflow vs. containing block). This preserves existing behavior for the common cases — an absolutely-positioned target whose containing block is a scroll container still flips against that scroller's visible box, and a fixed target flips against the viewport — so nothing in the suite regressed. But it is a change to core polyfill logic and worth a careful look.Verification
height: 400pxleaks into generated fallbacks; no console errors.