Add resize support to box when changing font-face on display/edit mode #1275

obetomuniz · 2020-04-20T14:44:06Z

* 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 ...

barklund

The font load functionality is a bit weirdly written - should be cleaned up, modernized, reactified and placed properly in the stack.

assets/src/edit-story/components/panels/textStyle/font.js

assets/src/edit-story/utils/textMeasurements.js

assets/src/edit-story/elements/text/edit.js

assets/src/edit-story/app/font/actions/useLoadFontFiles.js

github-actions · 2020-04-22T20:48:05Z

Size Change: +774 B (0%)

Total Size: 876 kB

Filename	Size	Change
`assets/js/edit-story.js`	409 kB	+443 B (0%)
`assets/js/stories-dashboard.js`	460 kB	+331 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`assets/css/edit-story.css`	3.04 kB	0 B
`assets/css/stories-dashboard.css`	3.04 kB	0 B

_{compressed-size-action}

assets/src/edit-story/components/library/text/fontPreview.js

assets/src/edit-story/app/font/actions/useLoadFontFiles.js

…r MULTIPLE. Improve useLoadFontFiles.js

codecov · 2020-04-27T15:03:40Z

Codecov Report

Merging #1275 into master will increase coverage by 0.02%.
The diff coverage is 75.47%.

@@             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

Impacted Files	Coverage Δ	Complexity Δ
assets/src/edit-story/elements/text/display.js	`26.66% <0.00%> (-2.97%)`	`0.00 <0.00> (ø)`
assets/src/edit-story/elements/text/edit.js	`5.45% <0.00%> (-0.32%)`	`0.00 <0.00> (ø)`
...dit-story/components/panels/textStyle/textStyle.js	`61.11% <25.00%> (-10.32%)`	`0.00 <0.00> (ø)`
...rc/edit-story/app/font/actions/useLoadFontFiles.js	`94.11% <93.93%> (-0.33%)`	`0.00 <0.00> (ø)`
.../edit-story/components/library/text/fontPreview.js	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...src/edit-story/components/panels/textStyle/font.js	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`

… Google Fonts.

assets/src/edit-story/components/panels/textStyle/font.js

assets/src/edit-story/elements/text/display.js

assets/src/edit-story/elements/text/edit.js

obetomuniz · 2020-05-12T14:31:25Z

@dvoytenko What about this PR? Is there some plan to get it merged or even re-reviewed?

dvoytenko

Approved with one nit.

assets/src/edit-story/elements/text/display.js

assets/src/edit-story/elements/text/edit.js

* 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 ...

swissspidy · 2020-05-16T10:43:53Z

@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) ...

barklund · 2020-05-19T19:53:25Z

assets/src/edit-story/components/panels/textStyle/font.js

@@ -54,18 +55,19 @@ function FontControls({ selectedElements, pushUpdate }) {
  const fontSize = getCommonValue(selectedElements, 'fontSize');

  const {
-    textInfo: { fontWeight },
+    textInfo: { fontWeight, isItalic },


fontWeight can be MULTIPLE_VALUE here. How does that play out with the font loader?

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.

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.

That could be an issue then. We always load all @font-faces. 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'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.

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.

@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.

Hm okay. But I did notice that #1197 calls for not ever displaying fallbacks. That's not factored in yet, right?

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.

googlebot · 2020-05-19T20:16:53Z

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

barklund · 2020-05-19T20:18:58Z

@googlebot I consent.

googlebot · 2020-05-19T20:19:23Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

Add resize support to box when changing font-face on display/edit mode

731d2d3

obetomuniz requested review from barklund, dvoytenko and swissspidy as code owners April 20, 2020 14:44

googlebot added the cla: yes label Apr 20, 2020

obetomuniz requested a review from miina April 20, 2020 14:44

barklund suggested changes Apr 20, 2020

View reviewed changes

assets/src/edit-story/components/panels/textStyle/font.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/panels/textStyle/font.js Outdated Show resolved Hide resolved

assets/src/edit-story/components/panels/textStyle/font.js Outdated Show resolved Hide resolved

dvoytenko suggested changes Apr 20, 2020

View reviewed changes

obetomuniz added the Type: Enhancement New feature or improvement of an existing feature label Apr 21, 2020

obetomuniz changed the title ~~Add resize support to box when changing font-face on display/edit mode~~ [WIP] Add resize support to box when changing font-face on display/edit mode Apr 21, 2020

Cover unicode chars, font-weight, font-style

ddbfa6f

obetomuniz commented Apr 22, 2020

View reviewed changes

assets/src/edit-story/app/font/actions/useLoadFontFiles.js Outdated Show resolved Hide resolved

Add ((MULTIPLE)) support proposal

5ab0266

obetomuniz force-pushed the fix/text-box-not-resizing branch from bbd6da0 to 5ab0266 Compare April 22, 2020 20:49

Improve docs.

1d4c346

miina reviewed Apr 23, 2020

View reviewed changes

assets/src/edit-story/components/library/text/fontPreview.js Outdated Show resolved Hide resolved

Fix problematic rest approach.

f15ec78

dvoytenko suggested changes Apr 23, 2020

View reviewed changes

obetomuniz force-pushed the fix/text-box-not-resizing branch from 206ec12 to 2ff6659 Compare April 24, 2020 18:18

Add fontSize support. Remove content support. Add a better support fo…

ef07e8f

…r MULTIPLE. Improve useLoadFontFiles.js

obetomuniz force-pushed the fix/text-box-not-resizing branch from 2ff6659 to ef07e8f Compare April 24, 2020 19:08

swissspidy mentioned this pull request Apr 27, 2020

Client-side font declarations #1273

Merged

3 tasks

Update textStyle tests

a0c6cad

Force display=auto on @font-face declaration while loading fonts from…

468738b

… Google Fonts.

obetomuniz changed the title ~~[WIP] Add resize support to box when changing font-face on display/edit mode~~ Add resize support to box when changing font-face on display/edit mode Apr 27, 2020

obetomuniz requested review from miina and barklund April 27, 2020 15:21

dvoytenko suggested changes May 8, 2020

View reviewed changes

miina mentioned this pull request May 8, 2020

Use the whole contents to load the correct font #923

Closed

obetomuniz changed the title ~~[WIP] Add resize support to box when changing font-face on display/edit mode~~ Add resize support to box when changing font-face on display/edit mode May 8, 2020

obetomuniz requested a review from dvoytenko May 8, 2020 15:59

Address recent PR review

e836308

obetomuniz force-pushed the fix/text-box-not-resizing branch from 6fd98b0 to e836308 Compare May 8, 2020 16:01

dvoytenko approved these changes May 13, 2020

View reviewed changes

assets/src/edit-story/elements/text/display.js Outdated Show resolved Hide resolved

assets/src/edit-story/elements/text/edit.js Outdated Show resolved Hide resolved

obetomuniz added 2 commits May 13, 2020 00:48

Improve code performance/reusability. Thanks @dvoytenko

a9cf8cf

dvoytenko approved these changes May 13, 2020

View reviewed changes

Merge branch 'master' into fix/text-box-not-resizing

32121b6

swissspidy approved these changes May 16, 2020

View reviewed changes

swissspidy assigned barklund May 16, 2020

obetomuniz added the Pod:Workspace label May 18, 2020

barklund reviewed May 19, 2020

View reviewed changes

Clean up promise logic a bit

124a96d

googlebot added cla: no and removed cla: yes labels May 19, 2020

barklund approved these changes May 19, 2020

View reviewed changes

googlebot added cla: yes and removed cla: no labels May 19, 2020

Merge branch 'master' into fix/text-box-not-resizing

a4b8199

barklund merged commit bc47950 into master May 19, 2020

barklund deleted the fix/text-box-not-resizing branch May 19, 2020 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

obetomuniz commented Apr 20, 2020 •

edited

barklund left a comment

github-actions bot commented Apr 22, 2020 •

edited

codecov bot commented Apr 27, 2020 •

edited

obetomuniz commented May 12, 2020

dvoytenko left a comment

swissspidy commented May 16, 2020

barklund May 19, 2020

barklund May 19, 2020

barklund May 19, 2020

dvoytenko May 20, 2020

dvoytenko May 20, 2020

barklund May 20, 2020

dvoytenko May 21, 2020

barklund May 21, 2020

dvoytenko May 22, 2020

googlebot commented May 19, 2020

barklund commented May 19, 2020

googlebot commented May 19, 2020

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

Conversation

obetomuniz commented Apr 20, 2020 • edited

barklund left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 22, 2020 • edited

codecov bot commented Apr 27, 2020 • edited

Codecov Report

obetomuniz commented May 12, 2020

dvoytenko left a comment

Choose a reason for hiding this comment

swissspidy commented May 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented May 19, 2020

barklund commented May 19, 2020

googlebot commented May 19, 2020

obetomuniz commented Apr 20, 2020 •

edited

github-actions bot commented Apr 22, 2020 •

edited

codecov bot commented Apr 27, 2020 •

edited