Skip to content

Fix RenderParagraph to use correct ideographic baseline#180292

Open
koji-1009 wants to merge 9 commits intoflutter:masterfrom
koji-1009:fix/paragraph_baseline
Open

Fix RenderParagraph to use correct ideographic baseline#180292
koji-1009 wants to merge 9 commits intoflutter:masterfrom
koji-1009:fix/paragraph_baseline

Conversation

@koji-1009
Copy link
Contributor

@koji-1009 koji-1009 commented Dec 25, 2025

fix #22625
fix #94680

The latest code shows that the implementation has been updated to call Skia's GetIdeographicBaseline.

https://github.com/flutter/flutter/blob/3.38.5/engine/src/flutter/lib/ui/text/paragraph.cc#L54

Skia's implementation was updated several years ago to use the calculation formula fDescent - fAscent + fLeading;.

https://github.com/google/skia/blob/33c2d9840f4e61e121c95a19ec7a0eb3bd2356a3/modules/skparagraph/src/Run.h#L496

import 'package:flutter/material.dart';

void main() {
  runApp(const MaterialApp(home: BaselineTest()));
}

class BaselineTest extends StatelessWidget {
  const BaselineTest({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: SafeArea(
        child: Padding(
          padding: const .all(16),
          child: Column(
            spacing: 8,
            crossAxisAlignment: .start,
            children: [
              _List(title: 'TextBaseline.alphabetic', baseline: .alphabetic),
              _List(title: 'TextBaseline.ideographic', baseline: .ideographic),
            ],
          ),
        ),
      ),
    );
  }
}

class _List extends StatelessWidget {
  const _List({required this.title, required this.baseline});

  final String title;
  final TextBaseline baseline;

  @override
  Widget build(BuildContext context) {
    return Column(
      spacing: 8,
      crossAxisAlignment: .start,
      children: [
        Text(title, style: Theme.of(context).textTheme.titleLarge),
        Container(
          color: Colors.grey[200],
          child: Row(
            crossAxisAlignment: .baseline,
            textBaseline: baseline,
            children: [
              const Text(
                'Highg',
                style: TextStyle(
                  fontSize: 60,
                  backgroundColor: Colors.amberAccent,
                ),
              ),
              const Text(
                '国jp',
                style: TextStyle(
                  fontSize: 60,
                  backgroundColor: Colors.orangeAccent,
                ),
              ),
              const SizedBox(width: 16),
              const Text(
                'Highg',
                style: TextStyle(
                  fontSize: 20,
                  backgroundColor: Colors.greenAccent,
                ),
              ),
              const Text(
                '国jp',
                style: TextStyle(
                  fontSize: 20,
                  backgroundColor: Colors.blueAccent,
                ),
              ),
              const SizedBox(width: 16),
              const Text(
                'Mixed国jp',
                style: TextStyle(
                  fontSize: 30,
                  backgroundColor: Colors.purpleAccent,
                ),
              ),
            ],
          ),
        ),
      ],
    );
  }
}

Screenshot

main fix
Android android_main android_fix
iOS ios_main ios_fix
Web web_main web_fix

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Dec 25, 2025
@koji-1009 koji-1009 marked this pull request as ready for review December 26, 2025 04:35
Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

Hi @koji-1009, thank you for your patience on this one! Overall this LGTM with a small comment about computeDryBaseline.

I'm curious if this also fixes related issues like #96713 and #94680

Another note is that RenderEditable does not have its similar methods hard-coded to alphabetic.

// property when the ideographic baseline is properly implemented
// (https://github.com/flutter/flutter/issues/22625).
return _textPainter.computeDistanceToActualBaseline(TextBaseline.alphabetic);
return _textPainter.computeDistanceToActualBaseline(baseline);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was also hard-coded in computeDryBaseline we should change that as well and add a test to verify its behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review.

The points you noted in commit aa7efe9 have been fixed. However, the testing step was missed, so I made a new commit, 0bfce44.

@koji-1009
Copy link
Contributor Author

koji-1009 commented Feb 18, 2026

#94680 has been fixed, I believe.

flutter branch alphabetic ideographic
main main_alphabetic main_ideographic
this fix_alphabetic fix_ideographic
import 'package:flutter/material.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(title: 'Flutter Demo', home: const MyHomePage());
  }
}

class MyHomePage extends StatelessWidget {
  const MyHomePage({super.key});

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: Center(
        child: Row(
          crossAxisAlignment: CrossAxisAlignment.baseline,
          textBaseline: TextBaseline.alphabetic,
          children: const [
            Text('ABCDefgh 漢字漢字漢字'),
            Text('ABCDefg 漢字', style: TextStyle(fontSize: 20)),
          ],
        ),
      ),
    );
  }
}

@koji-1009
Copy link
Contributor Author

It does not appear to affect #96713.

flutter branch null - even null - proportional even - proportional proportional - even
main main_null_e main_null_p main_e_p main_p_e
this fix_null_e fix_null_p fix_e_p fix_p_e
import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      home: Scaffold(
        appBar: AppBar(),
        body: Padding(
          padding: const .all(16),
          child: Column(
            children: [
              ..._buildOne(leadingDistribution: null),
              ..._buildOne(leadingDistribution: .proportional),
            ],
          ),
        ),
      ),
    );
  }

  List<Widget> _buildOne({
    required TextLeadingDistribution? leadingDistribution,
  }) {
    return [
      Text('leadingDistribution=${leadingDistribution?.name}'),
      const SizedBox(height: 32),
      for (final text in ['探索文本工王田一二三四口', 'ABCijk123', '探索文本工王田一二三四口ABCijk123'])
        Container(
          color: Colors.orange[200],
          margin: const .only(bottom: 32),
          child: Text(
            text,
            style: TextStyle(
              fontSize: 24,
              height: 1,
              leadingDistribution: leadingDistribution,
            ),
          ),
        ),
    ];
  }
}

@flutting
Copy link

Regarding text centering, this is a problem that can be seen by the naked eye, and it will appear everywhere, hoping to be solved. https://github.com/flutter/flutter/issues/96713

@koji-1009
Copy link
Contributor Author

@Renzo-Olivares
The issue with #96713 appears to be caused by Skia's ParagraphCache.

The ParagraphCache caches shaped Run objects that include fUseHalfLeading (const bool). When two paragraphs have the same text/font/size but different halfLeading, the second one gets a cache hit from the first, and the wrong fUseHalfLeading is used. This makes the result order-dependent.

https://github.com/google/skia/blob/b4f752493ea3a82f0cefd0b82904a6ec2b32c87c/modules/skparagraph/src/Run.cpp#L17-L61

In ParagraphCacheKey::computeHash(), the value of halfLeading is not reflected in the hash for TextStyle. While this does not appear to directly impact the bug, fHeightOverride also seems to be missing from TextStyle.

https://github.com/google/skia/blob/b4f752493ea3a82f0cefd0b82904a6ec2b32c87c/modules/skparagraph/src/ParagraphCache.cpp#L148-L205

Additionally, halfLeading is missing from TextStyle::equalsByFonts(), which is used by ParagraphCacheKey::operator==.

https://github.com/google/skia/blob/b4f752493ea3a82f0cefd0b82904a6ec2b32c87c/modules/skparagraph/src/TextStyle.cpp#L102-L115

https://github.com/google/skia/blob/b4f752493ea3a82f0cefd0b82904a6ec2b32c87c/modules/skparagraph/src/ParagraphCache.cpp#L247-L249

@justinmc justinmc added the a: text input Entering text in a text field or keyboard related problems label Mar 10, 2026
@github-actions github-actions bot removed the a: text input Entering text in a text field or keyboard related problems label Mar 10, 2026
@koji-1009
Copy link
Contributor Author

Hi @Renzo-Olivares, friendly ping on this one. I've addressed your review comment — added the fix for computeDryBaseline and a test to verify it (0bfce44). Please let me know if there's anything else needed.

Copy link
Contributor

@Renzo-Olivares Renzo-Olivares left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the patience on this one @koji-1009 and for the amazing contribution! I'm happy we are cleaning this up! Also for more context the original issue that was worked around by always using alphabetic baseline is still fixed after this change #25782.

We still need a secondary review and i'm also curious how google testing will do.

Can you please rebase to the latest master commit when you get a chance.

@Renzo-Olivares Renzo-Olivares added a: text input Entering text in a text field or keyboard related problems CICD Run CI/CD labels Mar 17, 2026
@github-actions github-actions bot removed a: text input Entering text in a text field or keyboard related problems CICD Run CI/CD labels Mar 17, 2026
@Renzo-Olivares Renzo-Olivares added the CICD Run CI/CD label Mar 17, 2026
@Renzo-Olivares
Copy link
Contributor

Google testing is failing because a user was explicitly setting baseline for their row to .ideographic, before this change we were hardcoding the baseline for text to alphabetic so it was a no-op, but after this change we can actually use ideographic so there are some visual changes. Still need to verify whether the ideographic is intentional or not considering it was a no-op before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CICD Run CI/CD framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

With TextBaseline.ideographic text gets cut off below the baseline. Properly implement ideographic baseline

4 participants