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

fix text layer transform when font-size less than the browser's min font-size #14427

Conversation

helianthuswhite
Copy link

@helianthuswhite helianthuswhite changed the title fix text layer transform when font-size less than the browser's minim… fix text layer transform when font-size less than the browser's min font-size Jan 7, 2022
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

As mentioned in #14426 (comment), this feels a little "heavy" as far as implementations go.
Also, when manually testing this code it doesn't seem to work as expected in Firefox (which is the primary development target) since 1 is always returned regardless of the browser setting.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 8, 2022

Looking at this again, the following issues are present:

  • This will add too much runtime overhead, by continually re-computing this for every textLayer element. While that could possibly be solved with caching, there's a couple of other issues as well.
  • Regardless of the minimum font-size set in Mozilla Firefox, this function always seem to return 1. Given that Firefox is the primary development target, we unfortunately cannot accept a patch that doesn't work in Firefox while also adding (general) runtime overhead.
  • The helper function doesn't really support an unset minimum font-size (which is the default in browsers), since it'll always fallback to return 1 even when that's not correct. At very low zoom levels the textLayer may contain elements with a font-size < 1px, which this patch thus won't handle correctly (and essentially break).

All-in-all, this patch unfortunately seems a bit too far from correct/working; hence let's close this PR and keep the issue open to track the problem.

@helianthuswhite
Copy link
Author

OK, thank you. I will have a think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants