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

Fix hyphenation for words with leading/trailing punctuation #26986

Merged
merged 1 commit into from Jan 12, 2021

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Dec 22, 2020

This patch fixes Minikin hyphenation to hyphenate words with
leading and/or trailing punctuation characters, using the same
logic as Minikin WordBreaker[1].

On Mac, the underlying Core Foundation API handles such cases.

This issue is part of the reasons for the last words not being
hyphenated <crbug.com/1022415>.

[1] https://android.googlesource.com/platform/frameworks/minikin/+/master/libs/minikin/WordBreaker.cpp#270:~:text=WordBreaker%3A%3AwordStart

Bug: 815061
Change-Id: I64908bf2000e9586b1cb7f0d28a0d730c6311da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599789
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Auto-Submit: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842337}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a comment

Choose a reason for hiding this comment

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

The review process for this patch is being conducted in the Chromium project.

@jfkthame
Copy link
Contributor

I'm a bit uneasy about the testcase here -- it assumes that hyphens:auto will yield the breaks "hy-phen-ation", which happens to match what current implementations produce (AFAIK all derived from the original Liang patterns for TeX) but is not the only possible correct result. A fuller set of breakpoints would be "hy-phen-a-tion" (see for example the title of https://www.tug.org/docs/liang/liang-thesis.pdf), but if an implementation found all three of these possible breakpoints, it would fail this test as written.

An implementation should not be penalized if it does a more thorough job of finding breakpoints than the existing patterns.

What should really be tested here is that the breakpoints found with surrounding punctuation are the same as those found without it -- whatever those may be -- not that one specific (and incomplete!) set of breakpoints is found. That'd be trickier to test, though, as it would no longer just involve a fixed reference.

Meanwhile, maybe it'd be good to revise the testcase to use a word where existing implementations more clearly find an exhaustive set of valid hyphenation points? E.g. I believe "ex-am-ple" is fully hyphenated by existing implementations.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2599789 branch 6 times, most recently from 3db120d to 6cd9dda Compare December 23, 2020 06:42
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2599789 branch 2 times, most recently from 876ce1b to 77a1780 Compare January 1, 2021 16:34
This patch fixes Minikin hyphenation to hyphenate words with
leading and/or trailing punctuation characters, using the same
logic as Minikin `WordBreaker`[1].

On Mac, the underlying Core Foundation API handles such cases.

This issue is part of the reasons for the last words not being
hyphenated <crbug.com/1022415>.

[1] https://android.googlesource.com/platform/frameworks/minikin/+/master/libs/minikin/WordBreaker.cpp#270:~:text=WordBreaker%3A%3AwordStart

Bug: 815061
Change-Id: I64908bf2000e9586b1cb7f0d28a0d730c6311da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2599789
Commit-Queue: Kent Tamura <tkent@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Auto-Submit: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842337}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants