Support soft hyphen (U+00AD) rendering with a Hyphens API - #185152

@LongCatIsLooong — this is the draft PR per your suggestion in #18443. Would appreciate it if you could kick off Google testing to surface any regressions in internal apps.

The change is intentionally minimal (one-line engine opt-in + gtest). A design doc covering this approach vs. exposing a Dart-level hyphens API (per @chunhtai's suggestion) is in progress and will be linked here once drafted.

Design doc is now posted: https://flutter.dev/go/soft-hyphens (redirect PR: flutter/website#13282; direct: https://docs.google.com/document/d/1w_d53wb-RMuwXmXIPq67JrZYG97XF1mDaKEoKBDt8-E)

Tracking issue: #185154

This PR is Stage 1 of the two-stage rollout described in the doc. Stage 2 (Dart Hyphens API) will be a separate PR once the design doc settles.

I put on the CICD label. The tests should finish in an hour or two, and then I'll start google testing.

../../../flutter/txt/src/skia/paragraph_builder_skia.cc:142:8: error: no member named 'setRenderSoftHyphens' in 'skia::textlayout::ParagraphStyle'
142 | skia.setRenderSoftHyphens(true);
| ~~~~ ^

The skia version with your reland commit should have already been rolled into flutter: #185131

@LongCatIsLooong good catch — thanks. My branch base (commit 6154f75) didn't include the Skia roll in #185131, so DEPS was still pinning the pre-fix Skia SHA. I've rebased onto current upstream/master (now pinning 391cdbe3ffe9, which is ahead of the reland commit 98daf58d4c), and force-pushed. The build should go through on the next CI run.

I'm really curious if this will pass Google and customer tests or not.

If so and we consider merging this, we should be certain that the phase 2 Dart solution does not need to touch Material/Cupertino, so it's not blocked by the decoupling code freeze, just in case someone shows up desperately needing to opt out.

it looks like the test is failing to compile:

../../../flutter/txt/tests/paragraph_builder_skia_tests.cc:39:23: error: 'TxtToSkia' is a private member of 'txt::ParagraphBuilderSkia'
39 | ASSERT_TRUE(builder.TxtToSkia(style).getRenderSoftHyphens());
| ~~~~~~~~^~~~~~~~~
../../../flutter/third_party/googletest/googletest/include/gtest/gtest.h:1838:50: note: expanded from macro 'ASSERT_TRUE'
1838 | #define ASSERT_TRUE(condition) GTEST_ASSERT_TRUE(condition)
| ^~~~~~~~~
../../../flutter/third_party/googletest/googletest/include/gtest/gtest.h:1821:23: note: expanded from macro 'GTEST_ASSERT_TRUE'
1821 | GTEST_TEST_BOOLEAN_(condition, #condition, false, true, GTEST_FATAL_FAILURE_)
| ^~~~~~~~~
../../../flutter/third_party/googletest/googletest/include/gtest/internal/gtest-internal.h:1451:38: note: expanded from macro 'GTEST_TEST_BOOLEAN_'
1451 | ::testing::AssertionResult(expression))
| ^~~~~~~~~~
../../../flutter/txt/src/skia/paragraph_builder_skia.h:41:36: note: declared private here
41 | skia::textlayout::ParagraphStyle TxtToSkia(const ParagraphStyle& txt);

Pushed a fix (2571b41e646). TxtToSkia is private, so the existing ParagraphStrutStyle test friends the generated gtest class in the header; I added the matching friend declaration for the new RenderSoftHyphensEnabled test:

friend class SkiaParagraphBuilderTests_ParagraphStrutStyle_Test;
friend class SkiaParagraphBuilderTests_RenderSoftHyphensEnabled_Test;

It looks like everything (include Google testing) passed. 🎉

Hi @dbebawy are you still working on this? In case you want to include this change in the next stable release, 3.47 will be cut soon (in about 2 weeks).

Hi @LongCatIsLooong — yes, still on it, and I'd like to land this in 3.47 (Q3 cutoff is July 7, ~2 weeks out).

Quick status so we're aligned. This PR currently only carries the engine-side opt-in (setRenderSoftHyphens(true) in ParagraphBuilderSkia). Per the 4/23 design update, the plan is to expand it to the combined scope so soft hyphens ship with an opt-out from day one:

• Hyphens { manual, none } enum (no auto)
• exposed as a direct widget parameter on Text/RichText plus ui.ParagraphStyle (not TextStyle)
• hyphenCharacter deferred (Skia shapeEllipsis hardcodes LTR bidi)

The only thing blocking me from writing that code is design sign-off — the doc has had no reviewer feedback since that 4/23 reconciliation, and I'd rather not implement the full API and then find the shape needs to change in review on a tight clock.

Could I get a gut-check from you (and/or @chunhtai @justinmc) on the reconciled design this week? If the enum + widget-param shape looks right, I can have the combined PR up within a few days, leaving runway for review before the cutoff. Happy to hop on a short sync if that's faster — or to just proceed against the doc as written and iterate in code review if you'd prefer that.

Thanks!

Applied @mdebbar's suggestions. The push reset the approvals and the CICD label, so could you all take another look, and could someone re-add CICD and add autosubmit once it's green?

@dbebawy Congrats on finishing this and thank your for your perseverance! I think this feature is really worth it.

@jtmcdole heads-up on a merge queue problem with Tree_analyze. When a PR adds dart:ui API and uses it in the framework, Tree_analyze passes on the PR but fails in the merge queue, because the merge queue run analyzes against the parent commit's engine. It isn't a required check, so nothing was blocked, but it marks the merge group red, along with the groups queued behind it since they include the same change.

Examples

• This PR (adds Hyphens to dart:ui): the merge queue run 37083712098 failed with Undefined name 'Hyphens'. The groups queued behind it failed the same way: Deprecate design libraries #192957 (37084701245), Starts flipping content on windows to avoid full screen renders #193133 (37086700371), Rebuild a semantics fragment when its sibling conflict changes #190431 (37088921157).
• [web] Add SemanticsRole.listBox and SemanticsRole.option #192971 (adds SemanticsRole.listBox / .option): the PR run 38316428020 passed, and the merge queue run 38334834175 failed with 12 issues like There's no constant named 'listBox' in 'SemanticsRole'.

What happens in the merge queue run (from 38334834175)

1. has-engine-changes reports a change, and wait_for_tests waits for Linux linux_host_engine and Linux linux_android_debug_engine on the merge group commit b535b37. Both succeed after about 10 minutes.
2. Both Redirect engine.version steps are skipped, because they only run when github.event_name == 'pull_request' (action.yml#L40-L50).
3. So the engine version comes from content_aware_hash.sh. The job checks out the merge group commit as a detached HEAD with full history (tree-analyze.yml#L18-L24), and there's no LUCI_CONTEXT on GitHub Actions. That means the gh-readonly-queue/master/pr-* exception doesn't match, and the script hashes the merge base with master instead (content_aware_hash.sh#L41-L66).
4. It downloads engine 2230fe9b7d48…, which is the content hash of the parent commit 2b089ff, not of the merge group commit:

$ git ls-tree 2b089ff2b42 -- DEPS engine bin/internal/release-candidate-branch.version | git hash-object --stdin
2230fe9b7d48a27900b2916a56f26d5d4209de64
$ git ls-tree b535b37972a -- DEPS engine bin/internal/release-candidate-branch.version | git hash-object --stdin
6c5179ef87f50bf0c9cdbaabc3ac4d663a8a4a75

For comparison, the PR run sets FLUTTER_PREBUILT_ENGINE_VERSION to the PR head (7cfe883) and finds no issues.

Two possible fixes: also redirect for merge_group (the wait step already waits on github.sha at L36), if merge queue engine builds publish to the same place as presubmit ones, or make the content hash use HEAD in this job so the merge queue exception applies. Artifacts for 6c5179ef87f5… are in flutter_infra_release now, though I can't tell whether they were there yet when that queue ran.