Add support for anchor-size() fn.
#70
Conversation
jgerigmeyer
commented
Jan 11, 2023
jgerigmeyer
commented
✅ Deploy Preview for anchor-position-wpt canceled.
|
✅ Deploy Preview for anchor-polyfill ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
WPT report for this branch: https://anchor-size-wpt-results--anchor-position-wpt.netlify.app |
src/polyfill.ts
Outdated
| break; | ||
| case 'right': | ||
| value = (offsetParent as HTMLElement).clientWidth - value; | ||
| value = (offsetParent as HTMLElement).offsetWidth - value; |
There was a problem hiding this comment.
@mirisuzanne I think these changes from clientWidth to offsetWidth might actually be incorrect -- and I'm also not sure if I need to worry about scaling here. This change seemed to fix one test (first test in https://github.com/web-platform-tests/wpt/blob/6cd6741bc20cd6b3ad9f95d19123b7850c696d79/css/css-anchor-position/anchor-position-inline-004.html), but it also caused other tests with borders to fail (first 2 in https://github.com/web-platform-tests/wpt/blob/7a0b0c285b91a0b1fa08da58c964b82e5cf40733/css/css-anchor-position/anchor-position-borders-001.html).
Do you know what the correct way to convert from a left to a right px value is? That's the goal here. E.g. given the distance x that would the left position to the right edge of a rectangle, how to consistently convert that to a right positioning value?
There was a problem hiding this comment.
When I play with this, it looks like right: 0px does not include borders for the containing block -- so I think this should remain clientWidth and clientHeight... 🤔
There was a problem hiding this comment.
Yeah, to get the right offset based on the x offset, you need: the width of the reference box, minus the x offset, minus the width of the target.
For the parent context width, the relevant box is the 'content box' (clientWidth?), but for the the target element you would need the full 'border box' including borders (offsetWidth?).
There was a problem hiding this comment.
For now I added a hack in 2b48e06, but I'm not confident in it.
* main: chore(deps): Automated dependency upgrades
src/polyfill.ts
Outdated
| vertical = getCSSPropertyValue(targetEl, 'writing-mode').startsWith( | ||
| 'vertical-', | ||
| ); |
There was a problem hiding this comment.
I think ideally we also handle sideways- - it's experimental, but people use it. I'm not familiar with getCSSPropertyValue(), but I assume it's getting the computed value of the property? I think that should work.
There was a problem hiding this comment.
Yes, getCSSPropertyValue() is our shorthand fn for getComputedStyle(el).getPropertyValue(prop).trim().
| case 'block': | ||
| case 'self-block': | ||
| resolved = vertical ? 'width' : 'height'; | ||
| break; | ||
| case 'inline': | ||
| case 'self-inline': |
There was a problem hiding this comment.
For the self- options, do we need to check the both the writing mode of the element and the parent? I'm not clear how that one works, but my guess would be we want to use whichever anchor-element axis is the target-element's block or inline?
There was a problem hiding this comment.
I think according to the spec it's reversed -- inline and block should use the writing mode of the element's containing block. I added a story: https://trello.com/c/KnqCnHx3/55-i-want-better-support-for-writing-modes
|
@sanajaved7 Ping on this review |
| anchorRect[axis] + anchorRect[dir] * ((percentage as number) / 100); | ||
| switch (targetProperty) { | ||
| case 'bottom': { | ||
| let offsetHeight = (offsetParent as HTMLElement).clientHeight; |
There was a problem hiding this comment.
Do you think it would be good to surface this workaround for inline elements in the README (or some other docs)? I'm not an expert on clientWidth or clientHeight at all so I don't know if this approach has any drawbacks but maybe it could be useful context for users just in case.
And either way, we might want to pull out any notes or things we want to call out about our implementation throughout the polyfill into a single place for users so this could be done later too as a part of that.
There was a problem hiding this comment.
@sanajaved7 What do you think about the new limitations section of the readme?
