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

Use modern canvas2D api for text where is available #7748

Merged
merged 10 commits into from
Oct 23, 2021

Conversation

eXponenta
Copy link
Contributor

@eXponenta eXponenta commented Aug 24, 2021

Partial solution for #7729 on latest Chrome by newest Canvas2D API.

At the moment there are not way solving this case for all cases outside browser implementation without external library that will include glyphs database order, reconstruct ordering of glyphs and ligatures.

See:
https://github.com/fserb/canvas2D/blob/master/spec/text-modifiers.md
https://chromestatus.com/feature/6051647656558592

Description of change

Using canvas2D letterSpacing for avoid flipping text for RTL and broking a glyphs for ligatures-heavy language.

Example:

https://exponenta.games/games/fix-spacing/

NOTE: You should use Chrome Canary or enable chrome://flags/#new-canvas-2d-api in stable chrome.
Current version of chrome 92 origin trials still not workig (for me) how should.

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

Older Chrome use a `textLetterSpacing` instead of proposed API `letterSpacing`
@eXponenta eXponenta marked this pull request as ready for review August 24, 2021 16:26
@eXponenta eXponenta changed the title Use modern canvas2D api for text when avalilable Use modern canvas2D api for text where is avalilable Aug 24, 2021
@eXponenta eXponenta changed the title Use modern canvas2D api for text where is avalilable Use modern canvas2D api for text where is available Aug 24, 2021
Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Overall, I think this is the correct approach for RTL and letter-spacing.

My only tiny suggestion would be to avoid the any cast. Create a stricter type.

interface ModernContext2D extends ContextRenderingContext2D {
   textLetterSpacing?: number;
   letterSpacing?: number;
}

public canvas: ModernContext2D;

@bigtimebuddy
Copy link
Member

Thanks 👍

@themoonrat
Copy link
Member

My query here; does this change mean that text on Chrome can now look different to text on any other browser? Consistency of visuals is pretty essential imo. If there is a visual difference, maybe there's a flag that is off by default that devs can opt into if they understand the ramifications?

@bigtimebuddy
Copy link
Member

bigtimebuddy commented Aug 25, 2021

Fair point @themoonrat. I could see this being an issue if someone is using line wrapping or expecting a certain width. Makes it more fragmented a dev experience. Though the impact is probably on the smaller side.

Adding a flag is similar to what we did with the new line height behavior, would be good approach here too. This should probably be opt-in.

@eXponenta
Copy link
Contributor Author

eXponenta commented Aug 27, 2021

Problem that users should select different implementation when native letter spacing not supported.
Looks like we should implement getter for setting that now inside text, like:

if ( Settings.NATIVE_LETTER_SPACING_SUPPORTED ) {
      Settings.USE_NATIVE_LETTER_SPACING = true; // disable emulation
      .....
} else {
     // preventing use letterSpacing and force 0 to it on text
     Settings.USE_LETTER_SPACING_EMULATION = false;
     .....
}

Looks like dirty, but allow to change bechavior globally for all Texts.

Upd.

Or maybe enum?

Settings.TEXT_LETTER_SPACING = NATIVE || EMULATED || DISABLED

@bigtimebuddy
Copy link
Member

I think you're over complicating this from the API design. There are two cases we should consider:

  1. Emulated-only letter spacing. Ignores the availability of native letter spacing (default)
  2. Uses native letters spacing if available but fallback to emulated.

We do not need to allow disabling letter-spacing. I don't see a case where that is useful. I also don't think we need to allow this on a per-instance basis, so global flag is fine. The case for using native is RTL or situations where emulated spacing fails.

I'd suggest a simple flag on Text, not settings.

Text.nativeLetterSpacing = true

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

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

nice!

@bigtimebuddy bigtimebuddy added this to the v6.2.0 milestone Sep 8, 2021
@eXponenta
Copy link
Contributor Author

Chrome 94 was shipped and letter spacing arrived.😁

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants