-
-
Notifications
You must be signed in to change notification settings - Fork 942
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
feat: Add basic support for RTL text direction. #1637
Conversation
That is a good PR. Can you add any unit tests and possible visual tests? The next major and complex edge case is when text has "letterSpacing". |
Update on letterSpacing support: It looks like all major browsers except Safari has native support for letterSpacing in canvas, and non-native support is very difficult due to mixed LTR/RTL text scenarios. Given this, are we okay with doing feature detection on letterSpacing:
How does that sound? I'll update this PR with the behavior mentioned, and then start working on unit testing. |
Ah, Safari... I love you so much. For now, I suggest assuming that |
What do you think of this revised proposal:
I think this approach is better than entirely ignoring I have A/B checked the current polyfill and Chrome's native implementation and the two look identical, so I don't think this would introduce inconsistent results other than the point noted above. |
The issue mentioned in pixi is already fixed in Konva. I just afraid that polyfill will be inconsistent with the native implementation. So I prefer to use polyfill everywhere for now. I think VERY VERY deep testing will be required to make sure all possible text variations look the same in polyfill vs native. But, as soon as Safari will have, we should drop polyfill at all. Are there any news about such support? We can use native |
I updated the PR to reflect our discussion:
Could you take a look before I start adding tests? |
Updated PR to address comments and add unit tests. It's ready for another look. |
Thanks for the Pull Request and your work with the feedback. |
@lavrton Given the situation in Israel, could we push out a new release with this PR so that Konva can officially support Hebrew? |
Released. |
Currently, supporting RTL languages requires workarounds as described in #1251, which has many caveats such as not being able to support
toDataUrl
or mixing LTR and RTL languages.This PR upstreams a basic feature addition that we have to address these cases. A new property
direction
is added on theText
element, which is passed through to the 2d canvas during rendering.This PR is basic to a minimum to ensure that it doesn't disrupt LTR languages, as such I'm sure there are still edge cases for RTL that it doesn't address. I wanted to keep the initial change small so that we can iterate, while this first step should still be useful for a lot of common cases that's useful to the Konva community.
Best,
xkxx