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
Conversation
* master: (30 commits) Update list of Google Fonts (#1272) Exclude template assets from plugin bundle for now (#1267) Alignment panel fix for single element (#1193) Bump polished from 3.5.1 to 3.5.2 (#1262) Bump eslint-plugin-testing-library from 3.0.3 to 3.0.4 (#1264) Bump lint-staged from 10.1.4 to 10.1.6 (#1263) Bump babel-jest from 25.3.0 to 25.4.0 (#1265) Bump jest from 25.3.0 to 25.4.0 (#1266) Removed unneeded init code Update MultiPartPill to use css power Updated SVGs to use currentColor Fixed typos Template Detail: Updated header and added left/right controls use thumbnail size from theme Dashboard: Remove circular imports from dashboard app (#1256) Bump lint-staged from 10.1.3 to 10.1.4 (#1257) fix popover layout fix dropdown alignment removes new line for buttons no boolean in proptypes ...
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.
The font load functionality is a bit weirdly written - should be cleaned up, modernized, reactified and placed properly in the stack.
Size Change: +774 B (0%) Total Size: 876 kB
ℹ️ View Unchanged
|
bbd6da0
to
5ab0266
Compare
206ec12
to
2ff6659
Compare
…r MULTIPLE. Improve useLoadFontFiles.js
2ff6659
to
ef07e8f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1275 +/- ##
============================================
+ Coverage 63.79% 63.81% +0.02%
Complexity 235 235
============================================
Files 590 590
Lines 9698 9731 +33
============================================
+ Hits 6187 6210 +23
- Misses 3511 3521 +10
|
… Google Fonts.
6fd98b0
to
e836308
Compare
@dvoytenko What about this PR? Is there some plan to get it merged or even re-reviewed? |
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.
Approved with one nit.
* master: (99 commits) Bump eslint-plugin-react from 7.19.0 to 7.20.0 (#1749) Checked if date is moment before wrapping Refactored formatted date tests Add spy to standardize timing on tests Remove tests that used to work in CI but no longer do Remove unused import Fix snapshot test for useTemplateAPI Added support for modal-routing on dashboard. Add additional sorting value to ensure the same order. (#1737) update tests around reshaping story object [ImgBot] Optimize images (#1723) Bump @testing-library/dom from 7.5.1 to 7.5.2 (#1732) Fixes element measuring relative to fullbleed (better) (#1558) swapping to use the decoded html version of the title we have access to to turn html symbols from & to characters & adding check for user name to display author moving required for users prop type to component level so that we can reuse the shape without it being required. updating displayDate propType to object not string some organization of imports moving date formatting to the card title level and memoizing clean up translation for modified date string in card item no need to call edit function inline - this has no functional difference but still ...
@barklund could you approve or dismiss your review when your requested changes have been addressed? Would be great to finally get this merged. |
* master: (43 commits) Fix issue templates headers Bump karma from 5.0.5 to 5.0.8 (#1851) add allow-empty-input in linter so that changes in storybook js don't fail in pre-commit (#1850) add FlagsProvider to storybook simplify jasmine eslint rules Bump @testing-library/dom from 7.5.6 to 7.5.7 (#1829) Bump react-moveable from 0.20.6 to 0.20.7 (#1827) Bump puppeteer from 3.0.4 to 3.1.0 (#1826) review fixes Karma: puppeeter support and native events ignore karma tests from coverage review fixes Update karma.config.js Update .eslintrc Incrase timeout for visual regression test (#1785) Dashboard: Adds Tooltips. Borders around Preview Cards. (#1800) Update issue & PR templates (#1824) Add docs about Stories Labs env (#1815) Bump @wordpress/e2e-test-utils from 4.6.0 to 4.7.0 (#1790) Add a basic database upgrader. (#1751) ...
@@ -54,18 +55,19 @@ function FontControls({ selectedElements, pushUpdate }) { | |||
const fontSize = getCommonValue(selectedElements, 'fontSize'); | |||
|
|||
const { | |||
textInfo: { fontWeight }, | |||
textInfo: { fontWeight, isItalic }, |
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 be MULTIPLE_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
being false
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
- etc.
If a MULTIPLE_VALUE
is possible, we should actually iterate over all selectedElements
and just pull these combinations. I see the selectedElements.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:
- Change font family/weight/style combination in the editor that hasn't been loaded before (the actual font file, not just
@font-face
). - Browser kicks off font file loading. This is as expected.
- The user immediately sees fallback font in the canvas. This is a little bit bad.
- We run text measurements to get the new height of the element based on the fallback font. This is pretty bad.
- The font loading has finished by the browser. Used styles are re-applied.
- The user now sees the actual font rendered on the canvas. The end-result is just a flicker.
- We do not re-trigger measurements, however. The box remains with the wrong height.
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 user immediately sees fallback font in the canvas. This is a little bit bad.
The critical problem however is point 4:
We run text measurements to get the new height of the element based on the fallback font. This is pretty bad.
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Close #1008
Close #923