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

Hang trailing whitespace preceding explicit newline #276

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wfdewith
Copy link
Contributor

Fixes #263

@wfdewith wfdewith added this to the 0.3 Release milestone Feb 16, 2025
Comment on lines +539 to +541
// An RTL layout that has a line that ends with a space followed by a
// forced line break will always have a separate run that looks like:
// [" ", "\n"]. This means that the first cluster of a run with trailing
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this is? Perhaps with an example? I believe you, but in terms of providing a meaningful review, I'm not currently fully following the logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I have to be honest here, and tell you that I don't exactly know why RTL works like this. I assume that the space can be that the explicit newline forces mixed direction runs in the BiDi algorithm, but from my first skimming of https://www.unicode.org/reports/tr9/, line separators should be treated the same as any white space.

In any case, the current shaper gives you three runs like this: [RTL("ااااا"), LTR(" \n"), RTL("بببب")], with the explicit newline. Without it you get just one run like this: [RTL("بببب ااااا"].

I'll make this PR a draft until I understand it better and can explain it.

@nicoburns
Copy link
Contributor

nicoburns commented Feb 17, 2025

We perhaps ought to land #275 before this PR. As I think the tests for this PR would be much more useful with that change.

EDIT: (#275 has now landed)

@wfdewith wfdewith marked this pull request as draft February 17, 2025 21:55
@wfdewith wfdewith force-pushed the trailing-whitespace-newline branch from 724e443 to 29e479e Compare February 17, 2025 22:09
@xStrom xStrom modified the milestones: 0.3 Release, 0.4 Release Feb 27, 2025
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.

Trailing whitespace with forced line breaks is not handled correctly
3 participants