Skip to content

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

Closed
helianthuswhite wants to merge 1 commit intomozilla:masterfrom
helianthuswhite:fix-minimum-font-size-highlight
Closed

fix text layer transform when font-size less than the browser's min font-size#14427
helianthuswhite wants to merge 1 commit intomozilla:masterfrom
helianthuswhite:fix-minimum-font-size-highlight

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