Skip to content

[shaping] fix crash when wrapping part of string including forced break#266

Draft
benoitkugler wants to merge 2 commits into
mainfrom
wrapping-forced-break
Draft

[shaping] fix crash when wrapping part of string including forced break#266
benoitkugler wants to merge 2 commits into
mainfrom
wrapping-forced-break

Conversation

@benoitkugler

Copy link
Copy Markdown
Contributor

We recently changed our Bidi implementation, introducing a subtle difference when handling strings containing line breaks (\n).

The x/text implementation, despite claiming

If s contains a paragraph separator it will only process the first paragraph and report the number of bytes
consumed from s including this separator.

actually segments line1\nline2 as a single run containing the whole text.

The new Bidi implementation correctly recognizes two paragraphs, and according to the Unicode spec, properly truncates the text input.

This change uncovers a latent bug (see the included test). I'm not 100% sure this PR is the proper fix and would love to have confirmation from @whereswaldon

@andydotxyz

Copy link
Copy Markdown
Contributor

Can you paste the panic/crash you were seeing too - I'd love to verify if it overlaps with something I was seeing

Comment thread shaping/wrapping_test.go
func TestWrapForcedBreak(t *testing.T) {
f := loadOpentypeFont(t, "../font/testdata/Roboto-Regular.ttf")

s := []rune("line1\nline2")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's certainly better not to crash in this case, but isn't this API misuse? The whole shaping infrastructure expects you to perform segmentation before you shape. Segmentation should convert this input string into two runs.

Or is the problem that we previously would fail to convert this into two runs, and would shape/wrap it as a single run regardless?

I don't think I understand the full context of the problem. 😅 Sorry! I've reread the PR description a few times, but I think I'm still missing something.

@benoitkugler benoitkugler Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think I've mixed up line break and paragraph break. That's probably why the context is not very clear here.
We will probably need to tweak our bidi implementation, but I need to double check that.
Sorry for the noise...

@benoitkugler
benoitkugler marked this pull request as draft July 15, 2026 08:52
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.

3 participants