Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@eyebrowsoffire
Copy link
Contributor

This brings the behavior in line with the other renderers. The framework sets this bit to make sure we render only using the Ahem font.

@github-actions github-actions bot added the platform-web Code specifically for the web engine label Mar 22, 2024
@eyebrowsoffire eyebrowsoffire requested a review from ditman March 22, 2024 20:37
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM, but is the test lying in a comment?

final ui.LineMetrics? metrics = paragraph.getLineMetricsAt(0);
expect(metrics, isNotNull);

// Ahem's 'X' character is a square, so it's the font size (10.0) * 4 characters.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the fallback font now FlutterTest? (_testFonts.first), or am I misunderstanding the testing fallback?

(If it's not Ahem, then I guess this comment is a little lie.)

PS: is there any way of adding an expectation of what font was eventually used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think FlutterTest is actually Ahem. It's the same font with a different name. Why do we have a FlutterTest font that is actually Ahem? I don't really know the historical reason. I'm just trying to match the behavior of CanvasKit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked up some more context, which can be found in this PR: #39809

The FlutterTest is almost Ahem, with a few modifications. I'm changing the comment to be more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, and in answer to your second question, no. The font is substituted under the hood in a way that isn't directly visible to a client via dart:ui. So we're doing measurements instead.

@eyebrowsoffire eyebrowsoffire added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 22, 2024
@auto-submit auto-submit bot merged commit 1c80bed into flutter:main Mar 22, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 23, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 23, 2024
flutter/engine@bc4fa43...8a51e12

2024-03-22 skia-flutter-autoroll@skia.org Roll Skia from 10a460af3308 to 2e99e3a5445c (1 revision) (flutter/engine#51632)
2024-03-22 skia-flutter-autoroll@skia.org Roll Dart SDK from a959c7def805 to ad26f4885bf4 (1 revision) (flutter/engine#51631)
2024-03-22 1961493+harryterkelsen@users.noreply.github.com [web] Add ability to customize font fallback download URL (flutter/engine#51569)
2024-03-22 jacksongardner@google.com [skwasm] Use test fonts while in debugEmulateFlutterTesterEnvironment mode (flutter/engine#51630)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

autosubmit Merge PR when tree becomes green via auto submit App platform-web Code specifically for the web engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants