Skip to content
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.

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

Update acceptable anchor element algorithm #195

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

ayoreis
Copy link
Contributor

@ayoreis ayoreis commented May 20, 2024

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

Copy link

netlify bot commented May 20, 2024

Deploy Preview for anchor-polyfill ready!

Name Link
🔨 Latest commit db82d13
🔍 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.

Copy link

netlify bot commented May 20, 2024

Deploy Preview for anchor-position-wpt canceled.

Name Link
🔨 Latest commit db82d13
🔍 Latest deploy log https://app.netlify.com/sites/anchor-position-wpt/deploys/6669d79573a96500081b59d6

@jamesnw
Copy link
Contributor

jamesnw commented May 20, 2024

@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
Copy link
Contributor Author

ayoreis commented May 21, 2024

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
Copy link
Contributor

jamesnw commented May 21, 2024

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
Copy link
Contributor Author

ayoreis commented May 29, 2024

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
Copy link
Contributor

jamesnw commented May 29, 2024

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.

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution! Aside from the bug you and @jamesnw have already discussed, I made a few minor suggestions to simplify the example CSS.

src/validate.ts Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
public/anchor-absolute.css Outdated Show resolved Hide resolved
@ayoreis
Copy link
Contributor Author

ayoreis commented Jun 12, 2024

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:

  • Applied @jgerigmeyer's reviews (thanks!)
  • Added some comments with links to the spec
  • Removed the special case for top layer elements

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?

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

This is great -- thanks again! 👏

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

Yes, if you're willing to adjust the 5 failing tests, then this is good to merge.

<a href="#anchor-absolute" aria-hidden="true">🔗</a>
Absolutely positioned anchor
</h2>
<div style="position: relative; height: 150px;" class="demo-elements">
Copy link
Member

Choose a reason for hiding this comment

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

I guess the linter complains about this extra semicolon 🤷

Suggested change
<div style="position: relative; height: 150px;" class="demo-elements">
<div style="position: relative; height: 150px" class="demo-elements">

@jgerigmeyer
Copy link
Member

@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!

@jgerigmeyer jgerigmeyer merged commit 8def64e into oddbird:main Jun 26, 2024
8 checks passed
@ayoreis
Copy link
Contributor Author

ayoreis commented Jun 26, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support absolutely positioned anchors (not just top layer)
3 participants