Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

editable_test.dart not font-hermetic on web #83129

Open
yjbanov opened this issue May 21, 2021 · 5 comments
Open

editable_test.dart not font-hermetic on web #83129

yjbanov opened this issue May 21, 2021 · 5 comments
Labels
a: tests "flutter test", flutter_test, or one of our tests c: flake Tests that sometimes, but not always, incorrectly pass c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team: skip-test An issue used to track tests that are skipped. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-web Owned by Web platform team triaged-web Triaged by Web platform team

Comments

@yjbanov
Copy link
Contributor

yjbanov commented May 21, 2021

This test is checking for specific size of the Chinese characters, but our ahem.ttf font doesn't have them (according to fontdrop.info). This causes the browser to fallback to a local font, which can be different from one computer to another.

This causes HHH to fail, for example:

https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8846622841805053712/+/u/flutter_test_web_tests/stdout

Somehow the C++ engine is immune to this problem. Perhaps it also falls back to some font, but that fallback font is stable across machines.

@yjbanov yjbanov added a: tests "flutter test", flutter_test, or one of our tests team Infra upgrades, team productivity, code health, technical debt. See also team: labels. engine flutter/engine repository. See also e: labels. team: flakes platform-web Web applications specifically P2 Important issues not at the top of the work list c: tech-debt Technical debt, code quality, testing, etc. labels May 21, 2021
@GaryQian
Copy link
Contributor

We should add ideographic sample glyphs to cough (flutter/engine#13649) and change tests to depend on that instead of Ahem for ideographic scripts.

@darrenaustin
Copy link
Contributor

Not sure what fixed this, but I was able to turn the test marked with this bug in editable_test.dart with #87700. I will close this, if you feel it is still not properly addressed, please reopen.

@darrenaustin darrenaustin added the team: skip-test An issue used to track tests that are skipped. label Aug 5, 2021
@sigmundch sigmundch reopened this Aug 17, 2021
@sigmundch
Copy link
Contributor

Unfortunately this is still happening - the Dart/flutter HHH bot went red again once the test was reenabled in 225a43d

See https://ci.chromium.org/ui/p/dart/builders/ci.sandbox/flutter-engine-linux-web_tests/3108/overview

I believe this is hard to repro because many of our local setups have fallback fonts that would work OK.

Maybe there is a way we can force these processes to not look for an alternative font? Could we use an approach, like using FONTCONFIG_PATH or FONTCONFIG_FILE, to ensure we can repro more easily locally?

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Feb 23, 2023

Somehow the C++ engine is immune to this problem.

Looks like font fallback is disabled in test mode so those characters are always tofu:
https://github.com/flutter/engine/blob/34794638c8b88ea9cabde44e587ece6d0dc56757/lib/ui/text/font_collection.cc#L143
flutter/engine#4661

Looking at the original issue that for this test, it doesn't seem to have to do with the script or the baseline positions? It's just in those fonts the Chinese characters are generally taller than the 0x20 whitespace char we used to use to measure the line height? If that's the case using Ahem defeats the purpose of the test.

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Feb 23, 2023

Ah nvm SkParagraph reports font metrics instead of glyph metrics for height. So it looks like the original issue is caused by different typefaces being used. I'll update the test and use a child text span with a larger font size instead.

whesse added a commit to whesse/flutter that referenced this issue Jun 20, 2023
Monorepo builds and try jobs use an engine.version that is different
than the commit hash of the engine build. This different engine.version
value allows artifacts to be uploaded to and downloaded from a unique
path for those builds. Control over skipping this test is moved
to an explicit environment variable named
NO_FLUTTER_ENGINE_VERSION_CHECK.

The previous way of skipping these checks was also used to skip
a single test in the flutter package test file rendering/editable_test.dart.
This is changed to a separate environment flag to skip that test.
Links in the source code comments are updated to the relevant issue,
flutter#83129
@flutter-triage-bot flutter-triage-bot bot added c: flake Tests that sometimes, but not always, incorrectly pass multiteam-retriage-candidate team-web Owned by Web platform team triaged-web Triaged by Web platform team labels Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: tests "flutter test", flutter_test, or one of our tests c: flake Tests that sometimes, but not always, incorrectly pass c: tech-debt Technical debt, code quality, testing, etc. engine flutter/engine repository. See also e: labels. P2 Important issues not at the top of the work list platform-web Web applications specifically team: skip-test An issue used to track tests that are skipped. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. team-web Owned by Web platform team triaged-web Triaged by Web platform team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants