search

oddbird / css-anchor-positioning

Polyfill for CSS Anchor Positioning
https://anchor-polyfill.netlify.app/
BSD 3-Clause "New" or "Revised" License
250 stars 8 forks source link

Update acceptable anchor element algorithm #195

Closed ayoreis closed 3 months ago

ayoreis commented 4 months ago

Hi, I updated the acceptable anchor element algorithm because I needed it for a project but I think it's good to merge upstream.

All test pass (one needed updating) and I added a new example to the home page.

I left a TODO which I already started to fix but decided it would be better to separate into another PR (pseudo-element anchors).

Closes #103

netlify[bot] commented 4 months ago

Deploy Preview for anchor-polyfill ready!

Name Link
Latest commit db82d13bd717f1361ea97eb94d24f896d579c45a
Latest deploy log https://app.netlify.com/sites/anchor-polyfill/deploys/6669d7954eb2a9000858ec18
Deploy Preview https://deploy-preview-195--anchor-polyfill.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] commented 4 months ago

Deploy Preview for anchor-position-wpt canceled.

Name Link
Latest commit db82d13bd717f1361ea97eb94d24f896d579c45a
Latest deploy log https://app.netlify.com/sites/anchor-position-wpt/deploys/6669d79573a96500081b59d6
jamesnw commented 4 months ago

@ayoreis Thanks for this PR! This is a huge help.

I haven't done a full review yet, but I did run it through the Web Platform Tests locally. This PR makes many more tests pass 🥳 , but there is a regression in anchor-name-inline-001.html.

It looks like originally all the anchor-sizes were resolving to 20, and in this PR, they now are all resolving to 30.

Per @xiaochengh in #103, "An element can use absolutely positioned anchors that come earlier in tree order". This is laid out more explicitly in the spec, with-

An element el is a acceptable anchor element for an absolutely positioned element query el if all of the following are true: ...

...

ayoreis commented 4 months ago

Thanks for looking at this @jamesnw.

Turns out I only ran the unit tests, I'm already working on fixing the e2e tests.

When running the Web Platform Tests (npm run test:wpt) I'm getting this error: Error: invariant: WPT_MANIFEST environment variable must be set. What do I need to set it to? Is there anything else to know?

jamesnw commented 4 months ago

Apologies for that- we're aware that WPT is one area that needs attention as we resume active development. What I'd recommend for right now-

  1. Clone and setup WPT for running locally.
  2. In the polyfill repo, run npm run build:wpt
  3. In the WPT repo, run ./wpt serve --inject-script=[path-to-polyfill-repo]/dist/css-anchor-positioning-wpt.umd.cjs
  4. Open http://web-platform.test:8000/tools/runner/index.html, and run the tests in the /css/css-anchor-position directory. I usually limit to just the JavaScript Tests.

Note that there are roughly 6000 failures right now, so your goal isn't to get all WPT tests passing- that will be a larger effort.

ayoreis commented 4 months ago

Hi, sorry for taking so long. Quick update: I found the issue and started fixing it (Floating UI's platform.getOffsetParent does not work to get an element's containing block in all cases), I'll commit the fix tomorrow.

To check for regressions on a WPT run do you just use expectations metadata and then search for FAIL [expected PASS] on the output or is there a better way?

jamesnw commented 4 months ago

To check for regressions on a WPT run do you just use expectations metadata and then search for FAIL [expected PASS] on the output or is there a better way?

I ran on main, and then on my branch, and exported the results as JSON from both. Then I did a diff between the two, which was incredibly noisy and pretty annoying. I would love to make that easier.

ayoreis commented 3 months ago

Hi, sorry I took so long again.

The problem was using the platform.getOffsetParent function to get the containing block (Floating UI also has a containingBlock function but oddly it performed worse). I created a new function and wrote the code for when the element is absolutely positioned, but still used platform.getOffsetParent for the other positions (spec says to use the element's formatting context).

Also:

All previous Web Platform Tests pass + 46 new ones. I've been using this script I made to compare the results: https://github.com/ayoreis/wptr (It's been a great help, feel free to try it out).

Some end-to-end tests that are outdated compared with spec started failing, should I rewrite them?

jgerigmeyer commented 3 months ago

@ayoreis We're going to merge this and fix up the tests on our end so we can cut a new release. Thanks again for your contribution!

ayoreis commented 3 months ago

Thanks!