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

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

Merged
merged 27 commits into from
May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
731d2d3
Add resize support to box when changing font-face on display/edit mode
obetomuniz Apr 20, 2020
4e4a74b
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz Apr 20, 2020
ddbfa6f
Cover unicode chars, font-weight, font-style
obetomuniz Apr 22, 2020
5ab0266
Add ((MULTIPLE)) support proposal
obetomuniz Apr 22, 2020
1d4c346
Improve docs.
obetomuniz Apr 22, 2020
f15ec78
Fix problematic rest approach.
obetomuniz Apr 23, 2020
ef07e8f
Add fontSize support. Remove content support. Add a better support fo…
obetomuniz Apr 23, 2020
a0c6cad
Update textStyle tests
obetomuniz Apr 27, 2020
468738b
Force display=auto on @font-face declaration while loading fonts from…
obetomuniz Apr 27, 2020
117100f
Fix typo
obetomuniz Apr 27, 2020
880b46e
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz Apr 28, 2020
05c31fd
Add codecov to useLoadFontFiles
obetomuniz Apr 28, 2020
e516e7f
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz Apr 29, 2020
f130434
Update tests to match new APIs proposed after merge.
obetomuniz Apr 29, 2020
70b6a77
Address PR reviews
obetomuniz Apr 30, 2020
3a91999
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz May 1, 2020
b65c513
Address some PR reviews.
obetomuniz May 5, 2020
94fec7d
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz May 6, 2020
b7a18ba
Adjustments after #1323 merge. Revert content as parameter to load fo…
obetomuniz May 7, 2020
94c7720
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz May 7, 2020
e836308
Address recent PR review
obetomuniz May 8, 2020
a9cf8cf
Improve code performance/reusability. Thanks @dvoytenko
obetomuniz May 13, 2020
5bab938
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz May 13, 2020
32121b6
Merge branch 'master' into fix/text-box-not-resizing
swissspidy May 16, 2020
f95e480
Merge branch 'master' into fix/text-box-not-resizing
obetomuniz May 19, 2020
124a96d
Clean up promise logic a bit
May 19, 2020
a4b8199
Merge branch 'master' into fix/text-box-not-resizing
May 19, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 68 additions & 20 deletions assets/src/edit-story/app/font/actions/useLoadFontFiles.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,38 +25,86 @@ import { useCallback } from 'react';
import cleanForSlug from '../../../utils/cleanForSlug';
import getGoogleFontURL from '../../../utils/getGoogleFontURL';

/**
* This is a utility ensure that Promise.all return ONLY when all promises are processed.
obetomuniz marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {Promise} promise Promise to be processed
* @return {Promise} Return a rejected or fulfilled Promise
*/
const reflect = (promise) => {
return promise.then(
(v) => ({ v, status: 'fulfilled' }),
(e) => ({ e, status: 'rejected' })
);
};

function useLoadFontFiles() {
/**
* Adds a <link> element to the <head> for a given font in case there is none yet.
*
* Allows dynamically enqueuing font styles when needed.
*
* @param {string} name Font name.
* @param {Array} fonts An array of fonts properties to create a valid FontFaceSet to inject and preload a font-face
* @return {Promise} Returns fonts loaded promise
*/
const maybeEnqueueFontStyle = useCallback(({ family, service, variants }) => {
if (!family || service !== 'fonts.google.com') {
return;
}
const maybeEnqueueFontStyle = useCallback((fonts) => {
return Promise.all(
fonts
.map(
async ({
font: { family, service, variants },
fontWeight,
fontStyle,
content,
}) => {
if (!family || service !== 'fonts.google.com') {
return null;
}

const handle = cleanForSlug(family);
const elementId = `${handle}-css`;
const fontFaceSet = `
${fontStyle || ''} ${fontWeight || ''} 0 '${family}'
`.trim();

const handle = cleanForSlug(family);
const id = `${handle}-css`;
const element = document.getElementById(id);
const hasFontLink = () => {
return document.getElementById(elementId);
};

if (element) {
return;
}
const appendFontLink = () => {
return new Promise((resolve, reject) => {
const src = getGoogleFontURL([{ family, variants }], 'auto');
const fontStylesheet = document.createElement('link');
fontStylesheet.id = elementId;
fontStylesheet.href = src;
fontStylesheet.rel = 'stylesheet';
fontStylesheet.type = 'text/css';
fontStylesheet.media = 'all';
fontStylesheet.crossOrigin = 'anonymous';
fontStylesheet.addEventListener('load', () => resolve());
fontStylesheet.addEventListener('error', (e) => reject(e));
document.head.appendChild(fontStylesheet);
});
};

const src = getGoogleFontURL([{ family, variants }]);
const ensureFontLoaded = async () => {
if (document?.fonts) {
await document.fonts.load(fontFaceSet, content || '');
return document.fonts.check(fontFaceSet, content || '');
} else {
return null;
}
};

const fontStylesheet = document.createElement('link');
fontStylesheet.id = id;
fontStylesheet.href = src;
fontStylesheet.rel = 'stylesheet';
fontStylesheet.type = 'text/css';
fontStylesheet.media = 'all';
fontStylesheet.crossOrigin = 'anonymous';
if (!hasFontLink()) {
await appendFontLink();
}

document.head.appendChild(fontStylesheet);
return ensureFontLoaded();
}
)
.map(reflect)
);
}, []);

return maybeEnqueueFontStyle;
Expand Down
83 changes: 83 additions & 0 deletions assets/src/edit-story/app/font/test/actions/useLoadFontFiles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
* Copyright 2020 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* External dependencies
*/
import { renderHook } from '@testing-library/react-hooks';

/**
* Internal dependencies
*/
import useLoadFontFiles from '../../actions/useLoadFontFiles';

const DEFAULT_FONT = {
font: {
family: 'Font',
service: 'fonts.google.com',
},
fontWeight: 400,
fontStyle: 'normal',
content: 'Fill in some text',
};

describe('useLoadFontFiles', () => {
beforeEach(() => {
const el = document.getElementById('font-css');
if (el) {
el.remove();
}
});

it('maybeEnqueueFontStyle', () => {
expect(document.getElementById('font-css')).toBeNull();

renderHook(async () => {
const maybeEnqueueFontStyle = useLoadFontFiles();

await maybeEnqueueFontStyle([DEFAULT_FONT]);
});

expect(document.getElementById('font-css')).toBeDefined();
});

it('maybeEnqueueFontStyle skip', () => {
expect(document.getElementById('font-css')).toBeNull();

renderHook(async () => {
const maybeEnqueueFontStyle = useLoadFontFiles();

await maybeEnqueueFontStyle([
{ ...DEFAULT_FONT, font: { ...DEFAULT_FONT.font, service: 'abcd' } },
]);
});

expect(document.getElementById('font-css')).toBeNull();
});

it('maybeEnqueueFontStyle reflect', () => {
expect(document.getElementById('font-css')).toBeNull();

renderHook(async () => {
const maybeEnqueueFontStyle = useLoadFontFiles();

await maybeEnqueueFontStyle([{}, DEFAULT_FONT]);
});

expect(document.querySelectorAll('link')).toHaveLength(1);
expect(document.getElementById('font-css')).toBeDefined();
});
});
14 changes: 11 additions & 3 deletions assets/src/edit-story/components/library/text/fontPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { useEffect } from 'react';
import { useFont } from '../../../app';
import { ALLOWED_EDITOR_PAGE_WIDTHS, PAGE_WIDTH } from '../../../constants';
import { FontPropType } from '../../../types';
import stripHTML from '../../../utils/stripHTML';

const PREVIEW_EM_SCALE = ALLOWED_EDITOR_PAGE_WIDTHS[0] / PAGE_WIDTH;

Expand All @@ -53,14 +54,20 @@ const Text = styled.span`
color: ${({ theme }) => theme.colors.fg.v1};
`;

function FontPreview({ title, font, fontSize, fontWeight, onClick }) {
function FontPreview({ title, font, fontSize, fontWeight, content, onClick }) {
const {
actions: { maybeEnqueueFontStyle },
} = useFont();

useEffect(() => {
maybeEnqueueFontStyle(font);
}, [font, maybeEnqueueFontStyle]);
maybeEnqueueFontStyle([
{
font,
fontWeight,
content: stripHTML(content),
},
]);
}, [font, fontWeight, content, maybeEnqueueFontStyle]);

return (
<Preview onClick={onClick}>
Expand All @@ -80,6 +87,7 @@ FontPreview.propTypes = {
font: FontPropType,
fontSize: PropTypes.number,
fontWeight: PropTypes.number,
content: PropTypes.string,
onClick: PropTypes.func,
};

Expand Down
17 changes: 9 additions & 8 deletions assets/src/edit-story/components/panels/test/textStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ function Wrapper({ children }) {
],
},
actions: {
maybeEnqueueFontStyle: () => Promise.resolve(),
getFontByName: () => ({
name: 'Neu Font',
value: 'Neu Font',
Expand Down Expand Up @@ -418,9 +419,9 @@ describe('Panels/TextStyle', () => {
});

describe('FontControls', () => {
it('should select font', () => {
it('should select font', async () => {
const { pushUpdate } = renderTextStyle([textElement]);
act(() => controls.font.onChange('Neu Font'));
await act(() => controls.font.onChange('Neu Font'));
expect(pushUpdate).toHaveBeenCalledWith(
{
font: {
Expand All @@ -439,9 +440,9 @@ describe('Panels/TextStyle', () => {
);
});

it('should select font weight', () => {
it('should select font weight', async () => {
const { pushUpdate } = renderTextStyle([textElement]);
act(() => controls['font.weight'].onChange('300'));
await act(() => controls['font.weight'].onChange('300'));
const updatingFunction = pushUpdate.mock.calls[0][0];
const resultOfUpdating = updatingFunction({ content: 'Hello world' });
expect(resultOfUpdating).toStrictEqual(
Expand All @@ -452,17 +453,17 @@ describe('Panels/TextStyle', () => {
);
});

it('should select font size', () => {
it('should select font size', async () => {
const { getByTestId, pushUpdate } = renderTextStyle([textElement]);
const input = getByTestId('font.size');
fireEvent.change(input, { target: { value: '32' } });
await fireEvent.change(input, { target: { value: '32' } });
expect(pushUpdate).toHaveBeenCalledWith({ fontSize: 32 });
});

it('should select font size to empty value', () => {
it('should select font size to empty value', async () => {
const { getByTestId, pushUpdate } = renderTextStyle([textElement]);
const input = getByTestId('font.size');
fireEvent.change(input, { target: { value: '' } });
await fireEvent.change(input, { target: { value: '' } });
expect(pushUpdate).toHaveBeenCalledWith({ fontSize: '' });
});
});
Expand Down
54 changes: 40 additions & 14 deletions assets/src/edit-story/components/panels/textStyle/font.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { PAGE_HEIGHT } from '../../../constants';
import { useFont } from '../../../app/font';
import { getCommonValue } from '../utils';
import objectPick from '../../../utils/objectPick';
import stripHTML from '../../../utils/stripHTML';
import useRichTextFormatting from './useRichTextFormatting';
import getFontWeights from './getFontWeights';

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

const {
textInfo: { fontWeight },
textInfo: { fontWeight, isItalic },
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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

  1. normal Roboto
  2. normal italic Roboto
  3. bold normal Roboto
  4. bold italic Roboto
  5. 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.

Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

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:

  1. Change font family/weight/style combination in the editor that hasn't been loaded before (the actual font file, not just @font-face).
  2. Browser kicks off font file loading. This is as expected.
  3. The user immediately sees fallback font in the canvas. This is a little bit bad.
  4. We run text measurements to get the new height of the element based on the fallback font. This is pretty bad.
  5. The font loading has finished by the browser. Used styles are re-applied.
  6. The user now sees the actual font rendered on the canvas. The end-result is just a flicker.
  7. 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.

Copy link
Contributor

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?

Copy link
Contributor

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.

handlers: { handleSelectFontWeight },
} = useRichTextFormatting(selectedElements, pushUpdate);

const {
state: { fonts },
actions: { getFontByName },
actions: { maybeEnqueueFontStyle, getFontByName },
} = useFont();
const fontWeights = useMemo(() => getFontWeights(getFontByName(fontFamily)), [
getFontByName,
fontFamily,
]);
const fontStyle = isItalic ? 'italic' : 'normal';

return (
<>
Expand All @@ -76,21 +78,33 @@ function FontControls({ selectedElements, pushUpdate }) {
ariaLabel={__('Font family', 'web-stories')}
options={fonts}
value={fontFamily}
onChange={(value) => {
onChange={async (value) => {
const fontObj = fonts.find((item) => item.value === value);
const newFont = {
family: value,
...objectPick(fontObj, [
'service',
'fallbacks',
'weights',
'styles',
'variants',
]),
};

await maybeEnqueueFontStyle(
obetomuniz marked this conversation as resolved.
Show resolved Hide resolved
selectedElements.map(({ content }) => {
return {
font: newFont,
fontStyle,
fontWeight,
content: stripHTML(content),
};
})
);

pushUpdate(
{
font: {
family: value,
...objectPick(fontObj, [
'service',
'fallbacks',
'weights',
'styles',
'variants',
]),
},
font: newFont,
},
true
);
Expand All @@ -107,7 +121,19 @@ function FontControls({ selectedElements, pushUpdate }) {
placeholder={__('(multiple)', 'web-stories')}
options={fontWeights}
value={fontWeight}
onChange={handleSelectFontWeight}
onChange={async (value) => {
await maybeEnqueueFontStyle(
selectedElements.map(({ font, content }) => {
return {
font,
fontStyle,
fontWeight: parseInt(value),
content: stripHTML(content),
};
})
);
handleSelectFontWeight(value);
}}
/>
<Space />
</>
Expand Down