Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add resize support to box when changing font-face on display/edit mode #1275
Add resize support to box when changing font-face on display/edit mode #1275
Changes from 21 commits
731d2d3
4e4a74b
ddbfa6f
5ab0266
1d4c346
f15ec78
ef07e8f
a0c6cad
468738b
117100f
880b46e
05c31fd
e516e7f
f130434
70b6a77
3a91999
b65c513
94fec7d
b7a18ba
94c7720
e836308
a9cf8cf
5bab938
32121b6
f95e480
124a96d
a4b8199
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fontWeight
can beMULTIPLE_VALUE
here. How does that play out with the font loader?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise,
isItalic
beingfalse
doesn't mean that the entire text field is non-italic. It just means that at least one character is non-italic, the rest can still be.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it appears that all font variants are always loaded, so I'm not fully sure what this is even here for? But it seems to work okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be an issue then. We always load all
@font-face
s. However, the font loading itself is lazy, so we need to proactively force-load the variants that we need. E.g. if it's "Roboto" text which includes bold, non-bold, italic and non-italic sections, in all likelihood, we'll need to ask to load the following font combinations:normal Roboto
normal italic Roboto
bold normal Roboto
bold italic Roboto
If a
MULTIPLE_VALUE
is possible, we should actually iterate over allselectedElements
and just pull these combinations. I see theselectedElements.map()
there that roughly looks like what I'd expect. But I think you're right, at this is not quite correct. If so, we need to follow up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an alternative here, of course. We can actually proactively force load all variants always. That could be a bit of an overkill. It's not uncomment for a font to have 8+ variants. We'd be looking at 2-3M on average to load all variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My findings were, that we do load all the font combinations for the currently active font.
However the browser doesn't actually load the font files before it's used the first time. We just load the style sheet for all the font files, where some of those
@font-face
declarations are irrelevant currently.But it's not a lot of penalty bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@barklund it's not about
@font-face
themselves. This is about the loading of actual fonts. We do have all@font-face
declared, but we have to deal with the race conditions related to the lazy font loading itself. Currently (without these changes) the typical sequence on font prop change can look like this:@font-face
).The minimal goal for this pull request was to build the API that can be given a set of font combinations and it ensures that they are all loaded and ready to be used by the browser. And I think that part is solid here.
The maximal goal was to intercept (at least a few) places where font combinations change and use this API to ensure that race conditions do not happen. It's not 100% guaranteed that this is all done properly now and we may reconsider where/how we call this API ideally. But, at the very least, the API itself as developed is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm okay. But I did notice that #1197 calls for not ever displaying fallbacks. That's not factored in yet, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not displaying fallbacks is a low-hanging fruit and only solves point 3 above:
The critical problem however is point 4: