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.

Sign up for GitHub

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

[Mobile] - Line-height and font-size regression fixes #47284

Merged
merged 4 commits into from
Jan 19, 2023
Merged

[Mobile] - Line-height and font-size regression fixes #47284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d29f488
Revert "[Mobile] - RichText - Parse CSS values and avoid setting unde…
Jan 19, 2023
8119bde
parseCssUnitToPx - Fix issue with decimals in math expressions
Jan 19, 2023
859a4f2
Mobile - Avoid passing NaN line height values to Aztec. It also disab…
Jan 19, 2023
ea858c9
Mobile - Preformatted - Update snapshot to remove the fontSize value …
Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions packages/block-library/src/audio/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ exports[`Audio block renders audio block error state without crashing 1`] = `
fontFamily="serif"
fontSize={14}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -307,7 +306,6 @@ exports[`Audio block renders audio file without crashing 1`] = `
fontFamily="serif"
fontSize={14}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/file/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ exports[`File block renders file error state without crashing 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -173,7 +172,6 @@ exports[`File block renders file error state without crashing 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
minWidth={40}
onBackspace={[Function]}
Expand Down Expand Up @@ -296,7 +294,6 @@ exports[`File block renders file without crashing 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -377,7 +374,6 @@ exports[`File block renders file without crashing 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
minWidth={40}
onBackspace={[Function]}
Expand Down
4 changes: 0 additions & 4 deletions packages/block-library/src/preformatted/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,7 @@ exports[`core/more/edit/native should match snapshot when content is empty 1`] =
}
disableEditingMenu={false}
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBlur={[Function]}
onChange={[Function]}
Expand Down Expand Up @@ -76,9 +74,7 @@ exports[`core/more/edit/native should match snapshot when content is not empty 1
}
disableEditingMenu={false}
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBlur={[Function]}
onChange={[Function]}
Expand Down
7 changes: 0 additions & 7 deletions packages/block-library/src/search/test/__snapshots__/edit.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ exports[`Search Block renders block with button inside option 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -148,7 +147,6 @@ exports[`Search Block renders block with button inside option 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
minWidth={75}
onBackspace={[Function]}
Expand Down Expand Up @@ -228,7 +226,6 @@ exports[`Search Block renders block with icon button option matches snapshot 1`]
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -404,7 +401,6 @@ exports[`Search Block renders block with label hidden matches snapshot 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
minWidth={75}
onBackspace={[Function]}
Expand Down Expand Up @@ -484,7 +480,6 @@ exports[`Search Block renders with default configuration matches snapshot 1`] =
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down Expand Up @@ -600,7 +595,6 @@ exports[`Search Block renders with default configuration matches snapshot 1`] =
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
minWidth={75}
onBackspace={[Function]}
Expand Down Expand Up @@ -680,7 +674,6 @@ exports[`Search Block renders with no-button option matches snapshot 1`] = `
fontFamily="serif"
fontSize={16}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBackspace={[Function]}
onBlur={[Function]}
Expand Down
9 changes: 9 additions & 0 deletions packages/react-native-aztec/src/AztecView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,15 @@ class AztecView extends Component {
// We now support this but passing line-height as a prop instead.
}

// Remove Font size rendering for pre elements until we fix an issue with AztecAndroid.
if (
Platform.OS === 'android' &&
this.props.text?.tag === 'pre' &&
style.hasOwnProperty( 'fontSize' )
) {
delete style.fontSize;
}

return (
<TouchableWithoutFeedback onPress={ this._onPress }>
<RCTAztecView
Expand Down
77 changes: 32 additions & 45 deletions packages/rich-text/src/component/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,13 +854,9 @@ export class RichText extends Component {
// we compare previous values to refresh the selected font size,
// this is also used when the tag name changes
// e.g Heading block and a level change like h1->h2.
const currentFontSizeStyle = this.getParsedCssValue(
style?.fontSize,
DEFAULT_FONT_SIZE
);
const prevFontSizeStyle = this.getParsedCssValue(
prevProps?.style?.fontSize,
DEFAULT_FONT_SIZE
const currentFontSizeStyle = this.getParsedFontSize( style?.fontSize );
const prevFontSizeStyle = this.getParsedFontSize(
prevProps?.style?.fontSize
);
const isDifferentTag = prevProps.tagName !== tagName;
if (
Expand Down Expand Up @@ -915,16 +911,16 @@ export class RichText extends Component {
};
}

getParsedCssValue( value, defaultValue, fontSize = DEFAULT_FONT_SIZE ) {
getParsedFontSize( fontSize ) {
const { height, width } = Dimensions.get( 'window' );
const cssUnitOptions = { height, width, fontSize };
const cssUnitOptions = { height, width, fontSize: DEFAULT_FONT_SIZE };

if ( ! value ) {
return value;
if ( ! fontSize ) {
return fontSize;
}

const selectedPxValue =
getPxFromCssUnit( value, cssUnitOptions ) ?? defaultValue;
getPxFromCssUnit( fontSize, cssUnitOptions ) ?? DEFAULT_FONT_SIZE;

return parseFloat( selectedPxValue );
}
Expand All @@ -936,6 +932,11 @@ export class RichText extends Component {

let newFontSize = DEFAULT_FONT_SIZE;

// Disables line-height rendering for pre elements until we fix some issues with AztecAndroid.
if ( tagName === 'pre' && ! this.isIOS ) {
return undefined;
}

// For block-based themes, get the default editor font size.
if ( baseGlobalStyles?.typography?.fontSize && tagName === 'p' ) {
newFontSize = baseGlobalStyles?.typography?.fontSize;
Expand All @@ -961,40 +962,43 @@ export class RichText extends Component {

// We need to always convert to px units because the selected value
// could be coming from the web where it could be stored as a different unit.
const selectedPxValue = this.getParsedCssValue(
newFontSize,
DEFAULT_FONT_SIZE
);
const selectedPxValue = this.getParsedFontSize( newFontSize );

return selectedPxValue;
}

getLineHeight() {
const { baseGlobalStyles, tagName, lineHeight, style } = this.props;
const { currentFontSize } = this.state;
const tagNameLineHeight =
baseGlobalStyles?.elements?.[ tagName ]?.typography?.lineHeight;
let newLineHeight;

// Disables line-height rendering for pre elements until we fix some issues with AztecAndroid.
if ( tagName === 'pre' && ! this.isIOS ) {
return undefined;
}

if ( ! this.getIsBlockBasedTheme() ) {
return MIN_LINE_HEIGHT;
return;
}

// For block-based themes, get the default editor line height.
if ( baseGlobalStyles?.typography?.lineHeight && tagName === 'p' ) {
newLineHeight = baseGlobalStyles?.typography?.lineHeight;
newLineHeight = parseFloat(
baseGlobalStyles?.typography?.lineHeight
);
}

// For block-based themes, get the default element line height
// e.g h1, h2.
if ( tagNameLineHeight ) {
newLineHeight = tagNameLineHeight;
newLineHeight = parseFloat( tagNameLineHeight );
}

// For line height values provided from the styles,
// usually from values set from the line height picker.
if ( style?.lineHeight ) {
newLineHeight = style.lineHeight;
newLineHeight = parseFloat( style.lineHeight );
}

// Fall-back to a line height provided from its props (if there's any)
Expand All @@ -1003,34 +1007,17 @@ export class RichText extends Component {
newLineHeight = lineHeight;
}

// If it can't be converted into a floating point number is probably a CSS value
if ( ! parseFloat( newLineHeight ) ) {
const parsedLineHeight = this.getParsedCssValue(
newLineHeight,
MIN_LINE_HEIGHT,
currentFontSize
);

// Line height is in pixels
if ( Number.isInteger( parsedLineHeight ) ) {
const pxValue = parsedLineHeight / currentFontSize;
newLineHeight = parseFloat(
parseFloat( pxValue ).toFixed( 2 )
);
} else {
newLineHeight = parsedLineHeight;
}
}

// Check the final value is not over the minimum supported value.
if (
! newLineHeight ||
( newLineHeight && newLineHeight < MIN_LINE_HEIGHT )
) {
if ( newLineHeight && newLineHeight < MIN_LINE_HEIGHT ) {
newLineHeight = MIN_LINE_HEIGHT;
}

return parseFloat( newLineHeight );
// Until we parse CSS values correctly, avoid passing NaN values to Aztec
if ( isNaN( newLineHeight ) ) {
return undefined;
}

return newLineHeight;
}

getIsBlockBasedTheme() {
Expand Down
2 changes: 0 additions & 2 deletions packages/rich-text/src/test/__snapshots__/index.native.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ exports[`<RichText/> Font Size should update the font size when style prop with
fontFamily="serif"
fontSize={12}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBlur={[Function]}
onChange={[Function]}
Expand Down Expand Up @@ -64,7 +63,6 @@ exports[`<RichText/> Font Size should update the font size with decimals when st
fontFamily="serif"
fontSize={13}
isMultiline={false}
lineHeight={1}
maxImagesWidth={200}
onBlur={[Function]}
onChange={[Function]}
Expand Down
33 changes: 4 additions & 29 deletions packages/rich-text/src/test/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,9 @@ import RichText from '../component/index.native';
const mockGlobalSettings = (
settings = { fontSize: 'var(--wp--preset--font-size--normal)' }
) => {
const { fontSize, lineHeight } = settings;
const { fontSize } = settings;
const DEFAULT_GLOBAL_STYLES = {
__experimentalGlobalStylesBaseStyles: {
typography: { fontSize, lineHeight },
},
__experimentalGlobalStylesBaseStyles: { typography: { fontSize } },
};
jest.spyOn( select( blockEditorStore ), 'getSettings' ).mockReturnValue(
DEFAULT_GLOBAL_STYLES
Expand Down Expand Up @@ -264,9 +262,7 @@ describe( '<RichText/>', () => {
// Assert.
expect( screen.toJSON() ).toMatchSnapshot();
} );
} );

describe( 'Line height', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we remove test cases for the line height? I know we won't parse it from a CSS value but we're still parsing the value and enforcing a minimum value, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it is not removing it actually, we already had one added but it was part of the Font size tests, it looks weird in this diff but here's the test still.

I didn't keep should set a line height from a CSS value from the revert because we are no currently parsing CSS values after these changes. We should add it when we add support again.

it( 'should set the default minimum line height value if the provided value from the styles is lower', () => {
// Arrange.
const expectedLineHeight = 1;
Expand All @@ -276,29 +272,8 @@ describe( '<RichText/>', () => {
<RichText accessibilityLabel={ 'editor' } style={ style } />
);
// Assert.
const actualLineHeight =
getByLabelText( 'editor' ).props.lineHeight;
expect( actualLineHeight ).toBe( expectedLineHeight );
} );

it( 'should set a line height from a CSS value', () => {
// Arrange.
const expectedFontSize = 16;
const expectedLineHeight = 1.88;
mockGlobalSettings( {
fontSize: 'min(1em, 2em)',
lineHeight: 'calc( 1em + 0.875rem )',
} );
// Act.
const { getByLabelText } = render(
<RichText accessibilityLabel={ 'editor' } tagName="p" />
);
// Assert.
const actualFontSize = getByLabelText( 'editor' ).props.fontSize;
expect( actualFontSize ).toBe( expectedFontSize );
const actualLineHeight =
getByLabelText( 'editor' ).props.lineHeight;
expect( actualLineHeight ).toBe( expectedLineHeight );
const actualFontSize = getByLabelText( 'editor' ).props.lineHeight;
expect( actualFontSize ).toBe( expectedLineHeight );
} );
} );
} );