facebook / stylex

StyleX is the styling system for ambitious user interfaces.
https://stylexjs.com
MIT License
8.21k stars 304 forks source link

Logical properties for border-block-x incorrectly map. #541

Closed hogansghost closed 2 months ago

hogansghost commented 2 months ago

Describe the issue

When applying logical property styling rules for borderBlockStart and borderBlockEnd, they are incorrectly mapped to border-top-x and border-bottom-x rules respectively.

Screenshot 2024-04-17 at 11 29 02

Screenshot 2024-04-17 at 11 28 43

Screenshot 2024-04-17 at 11 28 54

borderInlineStart and borderInlineEnd seem to be working as expected as well as borderBlock which maps to the expected logical properties.

Screenshot 2024-04-17 at 11 30 25

Screenshot 2024-04-17 at 11 30 34

Expected behavior

borderBlockStart and borderBlockEnd should map to the respective long hand rules, as borderBlock already does:

border-block-start-color: ...
border-block-start-style: ...
border-block-start-width: ...
border-block-end-color: ...
border-block-end-style: ...
border-block-end-width: ...

Steps to reproduce

Package versions:

"@stylexjs/stylex": "0.6.0",
"vite-plugin-stylex": "0.8.3",
"vite": "5.2.8",
"react": "18.2.0",
  1. Create a simple element in your markup.
  2. Apply a new stylex.create({}) rule with the following properties applied:
    width: '200px',
    height '200px',
    borderBlockStart: '1px solid purple',
    borderBlockEnd: '1px solid crimson',
    borderInlineStart: '1px solid orange',
    borderInlineEnd: '1px solid lightblue',
  3. Inspect the computed rules for the element.
  4. See that block rules are being applied as top and bottom.

Test case

No response

Additional comments

I've had a quick investigation through the package code and I can see a few places where it is mapping the borderBlock rules to borderTop, etc. but I wasn't so far able to fix the issue locally adjusting the package files.

nmn commented 2 months ago

This is an intentional design decision at the moment. Older browser don't support all the various logical properties. This can be fully poly-filled but only for horizontal layouts. So to be maximally compatible StyleX currently only supports horizontal writing modes and changes block rules to top or bottom.

However, we plan to make this configurable as an option to the Babel plugin so that you can use truly logical styles if you only support the newest browsers.

hogansghost commented 2 months ago

Oh really? Should that not just be a developer choice for compatibility which rule they write?

if I know I need to support a browser earlier than 2021, I use the top | right | bottom | left and otherwise use the block | inline rules. I feel it's more confusing (spent 2 hours looking into this yesterday) mapping a rule "incorrectly" than someone getting a bug report for layout issues from an older browser.

It currently feels inconsistent as well in that borderBlockStart maps to border-top but then borderBlock correctly maps to border-block-start-x | border-block-end-x rules. I think border-block-start even has older key browser compatibility than the shorthand border-block.

** Side node - is there a list anywhere for any other that have these compatibility changes?

nmn commented 2 months ago

Should that not just be a developer choice for compatibility which rule they write?

My comment ends with the admission that yes, it should.

is there a list anywhere for any other that have these compatibility changes?

All the property mapping logic is here:

https://github.com/facebook/stylex/blob/main/packages/shared/src/preprocess-rules/application-order.js

necolas commented 2 months ago

We currently expand shorthands, so it's not a compatibility reason why we don't use border-block. Logical CSS is relatively recent. During development I didn't find many apps with vertical writng modes, probably because it wasn't possible before and users don't expect it on the internet. In practice, I don't think this detail matters much. Longer term we can drop polyfills like this but Meta serves half the planet, so compatibility with older browsers and meeting current user expectations is also necessary in some form.

I think we could actually be providing more (configurable) polyfills, including vendor prefixing, to simplify the DX and encourage engineers to adopt future features today. This has been the main value add of css preproccessors since they were introduced, and something stylex is well positioned to do

nmn commented 2 months ago

There are some complicating concerns. Specifically when it comes to mixing logical and physical properties. Not supporting vertical writing modes simplifies the implementation and results in a more efficient runtime. There should be a solution here. We only expand shorthands in the legacy “style resolution” mode.

hogansghost commented 2 months ago

All the property mapping logic is here:

https://github.com/facebook/stylex/blob/main/packages/shared/src/preprocess-rules/application-order.js

Excellent, I'll give that a once over. Thanks!

We currently expand shorthands ... ... Longer term we can drop polyfills like this but Meta serves half the planet, so compatibility with older browsers and meeting current user expectations is also necessary in some form.

I noticed that they were all expanded to the long form separate rules. My confusion mostly came from borderBlock correct expanding to block-start and block-end series of rules but then borderBlockStart expanding to border-top rules. That inconsistency threw me off as one logical property works as expected but then another doesn't.

I get that with the borderBlockStart current implementation developers can use the newer rules and it will automatically convert to a more widely supported rule and then in future styleX can update and no developer work will be needed for it to use the newer rules, but could this instead be done with some kind of under the hood utility like the firstThatWorks - just for those newer rules with @supports or something similar?

nmn commented 2 months ago

@hogansghost I think you’re looking at computed styles and not the styles that are actually applied.

hogansghost commented 2 months ago

@nmn oh dang, I definitely am. I didn't realise they were outputting different things! 💀

hogansghost commented 2 months ago

I guess we can close this one for now as it's a workable issue and there is a longer term plan to allow configuration.