Fix RenderParagraph to use correct ideographic baseline#180292
Fix RenderParagraph to use correct ideographic baseline#180292koji-1009 wants to merge 9 commits intoflutter:masterfrom
Conversation
Renzo-Olivares
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Looks like this was also hard-coded in computeDryBaseline we should change that as well and add a test to verify its behavior.
|
#94680 has been fixed, I believe.
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)),
],
),
),
);
}
} |
|
It does not appear to affect #96713.
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,
),
),
),
];
}
} |
|
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 |
|
@Renzo-Olivares The In Additionally, |
|
Hi @Renzo-Olivares, friendly ping on this one. I've addressed your review comment — added the fix for |
There was a problem hiding this comment.
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.
|
Google testing is failing because a user was explicitly setting baseline for their row to |












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
Screenshot
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-assistbot 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.