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

text: Do not show text that's missing an embedded font #18856

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kjarosh
Copy link
Member

@kjarosh kjarosh commented Dec 4, 2024

Flash Player does not show any text that requires an embedded font that is missing.

As we now support DefineFont4, we're safe to remove the fallback device font.

@kjarosh kjarosh added text Issues relating to text rendering/input A-core Area: Core player, where no other category fits T-compat Type: Compatibility with Flash Player labels Dec 4, 2024
@kjarosh kjarosh requested a review from Dinnerbone December 4, 2024 13:33
@kjarosh kjarosh marked this pull request as draft December 4, 2024 13:33
@kjarosh kjarosh added the waiting-on-review Waiting on review from a Ruffle team member label Dec 5, 2024
@kjarosh kjarosh marked this pull request as ready for review December 8, 2024 17:36
@@ -862,9 +862,9 @@ impl<'a, 'gc> LayoutContext<'a, 'gc> {
fn left_alignment_offset(span: &TextSpan, is_first_line: bool) -> Twips {
if span.bullet {
if is_first_line {
Twips::from_pixels(35.0 + span.left_margin + span.block_indent + span.indent)
Twips::from_pixels(36.0 + span.left_margin + span.block_indent + span.indent)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this just to make the tests pass better? The change is kinda out of nowhere ;D

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I added a test that verifies how the bullet behaves when there's no font (to make sure I'm not breaking anything), and it turned out the bullet was misaligned by one pixel 😅

@@ -660,10 +660,10 @@ impl<'a, 'gc> LayoutContext<'a, 'gc> {
{
return font;
}
// TODO: If set to use embedded fonts and we couldn't find any matching font, show nothing
// However - at time of writing, we don't support DefineFont4. If we matched this behaviour,
// then a bunch of SWFs would just show no text suddenly.
Copy link
Collaborator

Choose a reason for hiding this comment

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

just to be sure, did you test some SWFs like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally yes, I made sure that Flash behaves this way and most SWFs from my text collection work.

However, I've just remembered one SWF that used a wrong font: #14740, and unfortunately checking it now, it turns out the text disappears in it. Which makes sense, because now Ruffle somehow fails to load the embedded font and falls back to a device font :/

So I guess there are (and probably will be) cases when Ruffle fails to load an embedded font. Some probably are hard to spot.

@kjarosh kjarosh marked this pull request as draft December 9, 2024 23:28
Apparently it should be 36px, not 35px.
Flash Player does not show any text that requires
an embedded font that is missing.
This test verifies how bullets are rendered and which font should be used.
This test verifies how FP behaves when we want to get some metrics
related to text that uses a font which is missing.
@kjarosh kjarosh marked this pull request as ready for review December 12, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core player, where no other category fits T-compat Type: Compatibility with Flash Player text Issues relating to text rendering/input waiting-on-review Waiting on review from a Ruffle team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants