Skip to content

Commit

Permalink
[Mobile] - Line-height and font-size regression fixes (#47284)
Browse files Browse the repository at this point in the history
* Revert "[Mobile] - RichText - Parse CSS values and avoid setting undefined ones (#47080)"

This reverts commit 2d44f06.

* parseCssUnitToPx - Fix issue with decimals in math expressions

* Mobile - Avoid passing NaN line height values to Aztec. It also disables passing font size and line height values on Android for pre elements.

* Mobile - Preformatted - Update snapshot to remove the fontSize value as it will be set by Aztec for Android
  • Loading branch information
Gerardo Pacheco authored Jan 19, 2023
1 parent c00e0f0 commit 16f74d3
Show file tree
Hide file tree
Showing 8 changed files with 45 additions and 93 deletions.
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
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
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
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
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
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
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
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', () => {
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 );
} );
} );
} );

0 comments on commit 16f74d3

Please sign in to comment.