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

feat: Add basic support for RTL text direction. #1637

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Conversation

xkxx
Copy link
Contributor

@xkxx xkxx commented Sep 9, 2023

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 the Text 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

@lavrton
Copy link
Member

lavrton commented Sep 9, 2023

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".

@xkxx
Copy link
Contributor Author

xkxx commented Sep 11, 2023

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:

  • if it's supported in the browser, we use it natively for LTR and RTL languages, skipping the current polyfill behavior.
  • if it's not, we continue to use the current polyfill, and RTL with letterSpacing simply won't work.

How does that sound?

I'll update this PR with the behavior mentioned, and then start working on unit testing.

@lavrton
Copy link
Member

lavrton commented Sep 12, 2023

Ah, Safari... I love you so much.

For now, I suggest assuming that letterSpacing is not supported at all and render RTL text without it. From my point of view, consistent result across different browsers is critical.

@xkxx
Copy link
Contributor Author

xkxx commented Sep 12, 2023

What do you think of this revised proposal:

  • if it's supported in the browser, we use it natively for LTR and RTL languages, skipping the current polyfill behavior.
  • if it's not, we continue to use the current polyfill and adapt it to support LTR and RTL languages. This will be imperfect since we are not the browser and cannot detect Unicode codepoints 100%.

I think this approach is better than entirely ignoring letterSpacing for the scenario of mixing LTR and RTL text. If we use letterSpacing where we can, these text will render 100% correctly and with better performance, except in Safari where we use a polyfill. If we ignore letterSpacing and only use a polyfill, we may keep discover edge cases where it doesn't render correctly like pixijs/pixijs#5571, and these would be broken for all browsers.

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.

@lavrton
Copy link
Member

lavrton commented Sep 14, 2023

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 letterSpacing for RTL, because polyfill doesn't work for it at all, right?

@xkxx
Copy link
Contributor Author

xkxx commented Sep 18, 2023

I updated the PR to reflect our discussion:

  • Use the polyfill for all other cases except RTL.
  • Use native letterSpacing for RTL.

Could you take a look before I start adding tests?

src/shapes/Text.ts Outdated Show resolved Hide resolved
src/Context.ts Outdated Show resolved Hide resolved
src/shapes/Text.ts Outdated Show resolved Hide resolved
src/shapes/Text.ts Outdated Show resolved Hide resolved
src/shapes/Text.ts Outdated Show resolved Hide resolved
@xkxx
Copy link
Contributor Author

xkxx commented Sep 19, 2023

Updated PR to address comments and add unit tests. It's ready for another look.

src/Context.ts Outdated Show resolved Hide resolved
src/shapes/Text.ts Outdated Show resolved Hide resolved
@lavrton lavrton merged commit 23cbea2 into konvajs:master Sep 21, 2023
1 of 2 checks passed
@lavrton
Copy link
Member

lavrton commented Sep 21, 2023

Thanks for the Pull Request and your work with the feedback.

@xkxx
Copy link
Contributor Author

xkxx commented Oct 9, 2023

@lavrton Given the situation in Israel, could we push out a new release with this PR so that Konva can officially support Hebrew?

@lavrton
Copy link
Member

lavrton commented Oct 10, 2023

Released.

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.

2 participants