-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Mobile] - Line-height and font-size regression fixes #47284
Conversation
…les passing font size and line height values on Android for pre elements.
…as it will be set by Aztec for Android
Size Change: 0 B Total Size: 1.33 MB ℹ️ View Unchanged
|
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 changes seem sound to me. Thank you for putting this fix together so quickly. 🙇🏻
The provided test cases passed for the most part with the Masu, Quadrat, and Geologist themes. However, I noticed text size oddities in very specific cases. These may be small enough issues to address them in a separate PR/release. WDYT?
Pull Quote Text Size
- Enable Masu theme.
- Add a Pull Quote block.
- Add a line of text.
- Press the Return key.
Expected: The text size does not change.
Actual: Notice the text size increases.
masu-theme-pull-quote-text-size.mov
Quote Text Size
- Enable Geologist theme.
- Add a Quote block.
- Add a line of text.
- Press the Return key.
- Add a line of text.
- Press the Return key.
- Press the Delete key.
- Focus the first line of text.
- Focus the second line of text.
- Press the Return key.
- Press the Delete key.
Expected: All lines of text render the same text size (I think the larger text is actually the correct size).
Actual: The second line of text renders a different, larger text size.
geologist-quote-focus-return-key.mov
Thank you for testing @dcalhoun !
Yeah, there are still some cases where the font size is not consistent, I agree that these are small enough and this shouldn't affect users too much, then we can address them separately so it's can be fixed correctly without rushing an introducing more regressions 😅 |
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 provided test cases passed for the most part with the Masu, Quadrat, and Geologist themes. However, I noticed text size oddities in very specific cases. These may be small enough issues to address them in a separate PR/release. WDYT?
Yeah, there are still some cases where the font size is not consistent, I agree that these are small enough and this shouldn't affect users too much, then we can address them separately so it's can be fixed correctly without rushing an introducing more regressions 😅
👍🏻 Sounds good. These changes look good from my perspective.
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.
LGTM 🎊 !
I tested on both platforms the following cases:
- Having a block theme (e.g. Masu), setting alignment to a Paragraph block remains after splitting the block.
- Having a block theme/non-block theme, the Citation of both Quote and Pullquote blocks doesn't introduce
p
tags. - Having a block theme/non-block theme, the content of the Pullquote block is center aligned.
- Having a block theme/non-block theme, line height is fixed in blocks that use
pre
HTML tag like the Verse block and Preformatted block.
I noticed the same issues with the font size on iOS as the ones that David outlined in this comment. But I think they are not critical to solving them in this PR, especially due to the fact that we need to include this fix in ongoing release.
|
||
describe( 'Line height', () => { |
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.
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?
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.
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.
* 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
* Release script: Update react-native-editor version to 1.87.0 * Release script: Update with changes from 'npm run core preios' * Mobile - Update changelog * Release script: Update react-native-editor version to 1.87.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Remove unnecessary negative margin around empty gallery block (#47086) This PR removes some unwanted negative margin space from empty gallery blocks on native. * [Mobile] - RichText - Parse CSS values and avoid setting undefined ones (#47080) * parseCssUnitToPx - Fix issue with decimals in math expressions * Mobile - RichText - Parse CSS values from line height values and avoid undefined or NaN values being sent to the native TextInput * Mobile - Update Changelog * Release script: Update react-native-editor version to 1.87.2 * Release script: Update with changes from 'npm run core preios' * Add boolean contentStyle and clientId check to Column Edit InnerBlocks * Mobile - Column block - Move parentWidth to a const variable * docs: Update changelog * [Mobile] - Line-height and font-size regression fixes (#47284) * 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 * Update react-native-editor changelog Co-authored-by: Gerardo <gerardo.pacheco@automattic.com> Co-authored-by: Siobhan Bamber <siobhan@automattic.com> Co-authored-by: Derek Blank <derekpblank@gmail.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
* Release script: Update react-native-editor version to 1.87.0 * Release script: Update with changes from 'npm run core preios' * Mobile - Update changelog * Release script: Update react-native-editor version to 1.87.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Remove unnecessary negative margin around empty gallery block (#47086) This PR removes some unwanted negative margin space from empty gallery blocks on native. * [Mobile] - RichText - Parse CSS values and avoid setting undefined ones (#47080) * parseCssUnitToPx - Fix issue with decimals in math expressions * Mobile - RichText - Parse CSS values from line height values and avoid undefined or NaN values being sent to the native TextInput * Mobile - Update Changelog * Release script: Update react-native-editor version to 1.87.2 * Release script: Update with changes from 'npm run core preios' * Add boolean contentStyle and clientId check to Column Edit InnerBlocks * Mobile - Column block - Move parentWidth to a const variable * docs: Update changelog * [Mobile] - Line-height and font-size regression fixes (#47284) * 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 * Update react-native-editor changelog * Release script: Update react-native-editor version to 1.87.3 * Release script: Update with changes from 'npm run core preios' * [Mobile] Native Bridge: Fix insert blocks not handling raw string properly in unsupported block editor (#47472) * Update react-native-editor changelog --------- Co-authored-by: Gerardo <gerardo.pacheco@automattic.com> Co-authored-by: Siobhan Bamber <siobhan@automattic.com> Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com> Co-authored-by: Derek Blank <derekpblank@gmail.com> Co-authored-by: Renato Augusto Gama dos Santos <renato_0603@hotmail.com>
Related PRs:
Fixes #47263
Fixes #47264
What?
This PR addresses some regressions after #47080 was merged.
Why?
To prevent adding new regressions and to keep some of the fixes that were added before.
How?
First, this PR reverts partly #47080 due to other bugs showing up after it was merged.
We are keeping the changes related to updating the regex in
evalMathExpression
. Because without this some CSS values that included decimals were not being parsed.This PR also reverts passing a fallback line-height value due to causing new regressions on iOS for the Quote and Pullquote block and the Preformatted block on Android, now it will just prevent passing
NaN
values to the native side. TheseNaN
values happen because they're CSS values that are not being parsed, we will need to handle this in a different PR.Lastly it will not pass font-size and line-height values for blocks that use the
pre
HTML tag for Android (Preformatted
,Verse
), this causes issues with the Aztec formatter, until we found a workaround it is better to disable it. Note that if users change the values from the Font-size/Line-height pickers it won't change visually.Testing Instructions
Regression 1 (iOS) - Text alignment breaking after splitting or changing selection
Precondition: A site with the
Masu
theme enabled.Note: There was a font issue
in the first
21.5
betaRegression 2 (iOS) - Pullquote block render typography discrepancies
Precondition: A site with a block-based theme, I suggest
Quadrat
where the issue was spotted andMasu
.p
tags are not inserted for the citation field.Regression 3 (iOS) - Quote block render typography discrepancies
Precondition: A site with a block-based theme, I suggest
Quadrat
where the issue was spotted andMasu
.p
tags are not inserted for the citation field.Regression 4 (Android) - Preformatted and Verse blocks render incorrect line heights
Precondition: A site with a block-based theme, I suggest
Quadrat
where the issue was spotted andMasu
.Testing Instructions for Keyboard
N/A
Screenshots or screencast
See the testing instructions.