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

Render arabic rtl text properly #361

Merged
merged 7 commits into from
Apr 17, 2019

Conversation

solomancode
Copy link
Contributor

Refactor featureQuery.js implementation to favor functions composition over objects orientation

  • Gains:

    • easy to test
    • easy to debug
    • fails early
    • loosely coupled

Unify Substitution action + (class SubstitutionAction)

  • Gains:

    • common convention for glyph substitution
    • promotes code re-usability

  • added Latin word context check (required for applying Latin features)
  • normalize features application for different scripts
  • support Latin ligatures feature using featureQuery
  • auto apply essential features for scripts (arab & latn)
  • roll-back to default features if script was not found

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I did npm run test and all tests passed green (including code styling checks).
  • I have added tests to cover my changes.
  • My change requires a change to the documentation.
  • I have updated the README accordingly.
  • I have read the CONTRIBUTING document.

main components:
Tokenizer: convert a string into a list of tokens
FeatureQuery: query a feature and lookup a substitution
Bidi: query, apply arabic features and adjust arabic layout
Char: character class assertions

supported gsub tables
Single Substitution - lookup type 1, substFormat 2
Chaining Context Substitution - lookup type 6, substFormat 3
Ligature Substitution - lookup type 4, substFormat 1
* and shouldn't be turned off when rendering arabic text.
*/
{ script: 'arab', tags: ['init', 'medi', 'fina', 'rlig'] },
{ script: 'latn', tags: ['liga', 'rlig'] }
Copy link
Member

Choose a reason for hiding this comment

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

So if the font is not detected as Latin liga & rlig will be off?
Like if a font only supports emoji or a certain script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, every registered feature is checked before application. If the script is missing or the font doesn't support this feature, It'll silently fail to apply this feature.

* @param {any} options features options
*/
Font.prototype.updateFeatures = function (options) {
// TODO: update all features options not only 'latn'.
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to finish this TODO before merging this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe later. But it'll not affect the current functionality at all. I need find a common model for updating features on UI controls changes / passed options to stringToGlyphs for all scripts.

@Jolg42
Copy link
Member

Jolg42 commented Oct 22, 2018

👍 for all the gains!

Just curious about what happens if a font script is not arab or latn?
Kerning will be on by default and all other features off?
If so it looks like a breaking change from the latest defaults.

@solomancode
Copy link
Contributor Author

solomancode commented Oct 22, 2018

Just tested it now, Kerning is working fine for Latin script. as for other features. I'm not sure which features should be applied by default for other scripts. for now I added the required features for Arabic (init, medi, fina, rlig) and the most important features for Latin (liga, rlig). as for other scripts we might need some help from other contributors to understand which features to apply by default for which script.

The latest defaults only applied liga and rlig for every script. what I did was to narrow down the application of these features to latin script/text only and I added required Arabic features as well. and of course all the tests for liga and rlig are passing.

@solomancode
Copy link
Contributor Author

Take your time to make sure everything is fine before merging to master, If anything needs clarifying just leave me a comment.

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