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

1/n Nested Text textAlignVertical (Android) - Adding CustomStyleSpan API to align nested text vertically #35949

Closed
wants to merge 107 commits into from

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Jan 24, 2023

Summary

1/n of a series of pull requests (Nested Text textAlignVertical).

The CSS property vertically aligns the text relative to the lineHeight (comment).
https://developer.mozilla.org/en-US/docs/Web/CSS/vertical-align

The vertical-align CSS property sets the vertical alignment of an inline, inline-block, or table-cell box.

This PR adds the option textAlignVertical in the CustomStyleSpan constructor to change the text baseline (vertical alignment). The updateDrawState method is responsible for aligning the span (text) to the top or bottom.

  • If the text has multiple spans with different line heights, it applies the highest line height (the default behavior without inline images).
  • The text is aligned based on the font metrics top, bottom, ascent, and descent and the line height
  • Text FontMetrics are updated when fontSize changes and the text remains aligned to the top/bottom or center
  • Multiple spans with different font sizes correctly align to the top/bottom

Update 2nd of February

  • Moved the vertical text-align logic from ReactAbsoluteSizeSpan to CustomStyleSpan.
  • Investigate the cpp implementation of Paint.cpp
  • Use TextPaint API to calculate FontMetrics with different font-sizes
  • Mock the TextPaint constructor with PowerMock to fix JUnit tests.

Next PR: 2/n Nested Text textAlignVertical (Android) - adding textAlignVertical prop to NestedText #35704

Related:

fixes #30375 #30375 (comment)

Changelog

[ANDROID] [ADDED] - Adding CustomStyleSpan API to align nested text vertically

Test Plan

Video tests of the functionality: #35949 (comment) and #35949 (comment)
Previous Video tests of the functionality in PR #35704
#35949 (comment)
#35704 (comment)
#35704 (comment)
#35704 (comment)

Previous Tests

#35704 (comment)
#35704 (comment) #35704 (comment)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 24, 2023
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Jan 24, 2023

94c5da5

P type task
1 feature Run CustomLineHeight test on circleci
1.1 task Run test on circleci for CustomLineHeight without failure
1.2 task Trigger CustomLineHeight failure in CircleCi (log)

P type task
1 feature Run CustomLineHeight test on circleci
1.1 task Run test on circleci for CustomLineHeight without failure
1.2 task Trigger CustomLineHeight failure in CircleCi (log)

@fabOnReact fabOnReact added this to In Progress in Fabrizio Collaboration Jan 24, 2023
@analysis-bot
Copy link

analysis-bot commented Jan 24, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,453,797 -4,837
android hermes armeabi-v7a 7,777,311 -4,584
android hermes x86 8,930,030 -4,505
android hermes x86_64 8,787,255 -4,369
android jsc arm64-v8a 6,668,475 -2,424,297
android jsc armeabi-v7a 7,462,453 -828,416
android jsc x86 9,192,588 +48,998
android jsc x86_64 6,893,602 -2,508,853

Base commit: 26b2bb5
Branch: main

fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 9, 2023
@fabOnReact
Copy link
Contributor Author

P type task
1 review Test functionality with different types of font styles
2 bug Text does not align correctly to the top when moving logic to CustomStyleSpan
2.1 task Debug the value of fontSize in CustomStyleSpan with fontSize 32. Record video (video).
2.2 task Debug the value of getSize in ReactAbsSizeSpan with fontSize 32. Record video (video).

P type task
1 review Test functionality with different types of font styles
2 bug Text does not align correctly to the top when moving logic to CustomStyleSpan
2.1 task Debug the value of fontSize in CustomStyleSpan with fontSize 32. Record video (video).
2.2 task Debug the value of getSize in ReactAbsSizeSpan with fontSize 32. Record video (video).

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Feb 9, 2023

fabOnReact@112eeee fabOnReact@d8556a7

P type task
3 bug The parent Text does not align correctly with CustomStyleSpan with Gravity.TOP (correct-behaviour, not-correct-behaviour, commit)
3.1 task Change parent text alignment (Gravity.TOP to BOTTOM)
4 bug The logic for the highestFontSize correctly aligns the text. The issue is the remaining logic from CustomStyleSpan applied to other Spans.
4.1 task Comment the remaining logic and understand the root cause of the problem.

P type task
3 bug The parent Text does not align correctly with CustomStyleSpan with Gravity.TOP (correct-behaviour, not-correct-behaviour, commit)
3.1 task Change parent text alignment (Gravity.TOP to BOTTOM)
4 bug The logic for the highestFontSize correctly aligns the text. The issue is the remaining logic from CustomStyleSpan applied to other Spans.
4.1 task Comment the remaining logic and understand the root cause of the problem.

fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 9, 2023
…avity.TOP (correct-behaviour, not-correct-behaviour, commit)

facebook#35949 (comment)
@fabOnReact
Copy link
Contributor Author

fabOnReact commented Feb 10, 2023

Correctly aligning top/bottom with a different font family/style

https://www.icloud.com/iclouddrive/08ekjoNTUTIhPIY3nCF1C83mQ#using_different_font_style

@fabOnReact fabOnReact changed the title 1/n Nested Text textAlignVertical (Android) - Adding ReactAbsoluteSizeSpan API to align nested text vertically 1/n Nested Text textAlignVertical (Android) - Adding CustomStyleSpan API to align nested text vertically Feb 10, 2023
@fabOnReact fabOnReact marked this pull request as ready for review February 10, 2023 11:40
@fabOnReact fabOnReact moved this from In Progress to Waiting For Others in Fabrizio Collaboration Feb 10, 2023
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 20, 2023
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 17, 2024
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Jan 24, 2024
@fabOnReact
Copy link
Contributor Author

Should not have been closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
No open projects
Fabrizio Collaboration
Waiting For Others
Development

Successfully merging this pull request may close these issues.

[Android] textAlignVertical doesn't work on nested text
4 participants