Fix RenderParagraph to use correct ideographic baseline - #180292

#94680 has been fixed, I believe.

flutter branch	alphabetic	ideographic
main
this

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.

flutter branch	null - even	null - proportional	even - proportional	proportional - even
main
this

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 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

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.

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.

Going to rebase to re-trigger Google testing here and see if we can move this forward to land.

Discussed with @Renzo-Olivares offline:

I don't think skparagraph is using the baseline anywhere so that probably means setting this in TextStyle has no effect.

I suspect this by itself does not close #22625 because skparagraph isn't respecting TextStyle.textBaseline (and as a result if you mix ideographs and latin characters in the same paragraph and choose to use .ideographic baseline the text is still laid out using latin baseline).

I tried specifying textBaseline in dartpad (without this patch) and it seems to confirm textBaseline isn't being used by the paragraph:

@Rusino it looks like skparagraph isn't using TextStyle.textBaseline, do you know if there's planned work around adding textBaseline support?

Edited to mark #94680 as a partial fix, as we don't think we should close that with this PR until baseline configuration is supported in Skia too.

Looks like this is still failing as of last week. 😞

@koji-1009 Thanks for the patience with this one. There's an associated g3fix but I have not requested approval from the roller yet. Our current plan to move forward with this PR is to investigate adding ideographic baseline support to SkParagraph before we move forward with landing this. This way we don't break google testing twice and the fix is fully robust. As it stands getIdeographicBaseline() returned from SkParagraph is only valid for the first line of a paragraph, as well as other limitations noted in this comment #180292 (comment).

Greetings from stale triage, is this one still on hold for now? Should we mark it a draft in the meantime?

@Piinks Yeah I think for now I'll mark it as draft until I can take a deeper look at the changes necessary.