fix(Android) drawing additional empty line when 'textAlign' is set to 'justify' #47122
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
Error ReferenceErrorDangerfile |
|
These three test cases enter all specified layout builders in createLayout (new arch) and measureSpannedText (old arch) and it seems to work codeimport {
SafeAreaView,
StyleSheet,
Text,
} from 'react-native';
function App(): React.JSX.Element {
return (
<SafeAreaView style={{ backgroundColor: 'white', flex: 1, justifyContent: 'center'}}>
{/* boring layout */}
<Text style={[styles.textJustify, styles.containerWidth]}>
asdasjand akjsnd snd jsdnjkdn js s
</Text>
{/* multiline, boring layout */}
<Text style={[styles.textJustify, styles.containerWidth]}>
asdasd dajdoij sdnk aosjn dojna s dnjasjdkn dnsjdnjnkdaajs sjdnjn sdjn
jdns ssjkndjkansd s s s s s s s njnd sjkdnajdn sndjand sdjnaojdnsjdnsnd
jsdn sdn a s kk sdnjsjdn jsnd sjdn dddd ajksnd jad nd a d sakjnd skjnd
sjdnaj ksjand akjsnd snd jsdnjkdn js s
</Text>
{/* not boring, single line */}
<Text style={[styles.textJustify]}>
تخطيط الاختبارتخطيط الاختبارتخطيط الاختبرا تخطيط تخطيط تخطيط تخطيط ا ا
</Text>
</SafeAreaView>
);
}
const styles = StyleSheet.create({
containerWidth: {
width: 400,
},
textJustify: {
backgroundColor: 'red',
textAlign: 'justify',
margin: 16,
},
});
export default App; |
coado
commented
Oct 22, 2024
Comment on lines
149
to
171
| private static int getTextJustificationMode(MapBuffer attributedString) { | ||
| int justificationMode = (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? 0 : Layout.JUSTIFICATION_MODE_NONE; | ||
| if (!attributedString.contains(AS_KEY_FRAGMENTS)) { | ||
| return justificationMode; | ||
| } | ||
|
|
||
| MapBuffer fragments = attributedString.getMapBuffer(AS_KEY_FRAGMENTS); | ||
| if (fragments.getCount() != 0) { | ||
| MapBuffer fragment = fragments.getMapBuffer(0); | ||
| MapBuffer textAttributes = fragment.getMapBuffer(FR_KEY_TEXT_ATTRIBUTES); | ||
|
|
||
| if (textAttributes.contains(TextAttributeProps.TA_KEY_ALIGNMENT)) { | ||
| String alignmentAttr = textAttributes.getString(TextAttributeProps.TA_KEY_ALIGNMENT); | ||
|
|
||
| if (alignmentAttr.equals("justified")) { | ||
| justificationMode = (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? 1 : Layout.JUSTIFICATION_MODE_INTER_WORD; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return justificationMode; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we could merge retrieving alignmentAttr into one function as similar code is used in getTextAlignment
facebook-github-bot
added
the
Shared with Meta
| // TODO: Don't read AS_KEY_FRAGMENTS, which may be expensive, and is not present when using | ||
| // cached Spannable |
There was a problem hiding this comment.
nit: looks like we chopped off this part of the comment
| int justificationMode = (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? 0 : Layout.JUSTIFICATION_MODE_NONE; | ||
|
|
||
| if (alignmentAttr != null && alignmentAttr.equals("justified")) { | ||
| justificationMode = (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? 1 : Layout.JUSTIFICATION_MODE_INTER_WORD; |
There was a problem hiding this comment.
What's the significance of 1 here? We will never use this value on the old versions, right? Maybe we should just early return something invalid like -1;
There was a problem hiding this comment.
Yeah I somehow missed that it won't be even used 😅
|
|
||
|
|
||
| private static int getTextJustificationMode(@Nullable String alignmentAttr) { | ||
| int justificationMode = (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? 0 : Layout.JUSTIFICATION_MODE_NONE; |
There was a problem hiding this comment.
@cortinico more code that will go away once we bump to minSdk 26
| MapBuffer attributedString, | ||
| Spannable spanned, | ||
| @Nullable String alignmentAttr) { | ||
| // cached Spannable |
There was a problem hiding this comment.
Here's where that other part of the comment ended up 😅
| @@ -146,13 +146,42 @@ public static boolean isRTL(MapBuffer attributedString) { | |||
| == LayoutDirection.RTL; | |||
| } | |||
|
|
|||
| private static Layout.Alignment getTextAlignment(MapBuffer attributedString, Spannable spanned) { | |||
| @Nullable | |||
| private static String getTextAlignmentAttr(MapBuffer attributedString) { | |||
There was a problem hiding this comment.
Is splitting the alignment methods related to core change, or just for consistency?
There was a problem hiding this comment.
It's just for consistency and to not retrieve alignment twice from MapBuffer (first from getTextAlignment and second from getTextJustificationMode)
| // always passing ALIGN_NORMAL here should be fine, since this method doesn't depend on | ||
| // how exacly lines are aligned, just their width | ||
| // always passing ALIGN_NORMAL and JUSTIFICATION_MODE_NONE here should be fine, since this method doesn't depend on | ||
| // how exactly lines are aligned, just their width |
There was a problem hiding this comment.
I don't think this is correct, since justification can change line width.
There was a problem hiding this comment.
Yeah we could get current justification mode or pass -1 as well I think.
|
@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@NickGerleman merged this pull request in 08e8f6a. |
|
This pull request was successfully merged by @coado in 08e8f6a When will my fix make it into a release? | How to file a pick request? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Fixes #46908
The
justificationModeis not set for multiline text without unicode characters with known width on both architectures. This caused the issue of drawing additional empty line at the end ofTextViewbecause Yoga thought that text takes 5 lines and falsely calculated it's height.Currently, on the old architecture, the
justificationModeis set only on text that is not boring (contains unicode characters) with unknown width. I am not sure why is that, so I am opening this as a draft for now as I am still checking if it doesn't break anything.Changelog:
[ANDROID] [FIXED] - fix generating empty line at the end of multiline text view when
textAlignis set tojustifyTest Plan:
I've tested on both architectures on repro provided in the issue.