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

fix(Android) drawing additional empty line when 'textAlign' is set to 'justify' #47122

Closed
wants to merge 8 commits into from
Closed

fix(Android) drawing additional empty line when 'textAlign' is set to 'justify' #47122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7a2726c
fix justification issue on both arch
coado Oct 18, 2024
9b87f7c
fix alignment
coado Oct 18, 2024
4937a7f
remove unused LineBreaker import
coado Oct 18, 2024
9feb447
refactor getTextAlignment and getTextJustificationMode
coado Oct 22, 2024
2288f1c
fix spacing
coado Oct 22, 2024
68cf371
refactor getTextJustificationMode
coado Oct 24, 2024
ff5fa9a
fix comments
coado Oct 24, 2024
232ed3f
using current justification mode in onDraw in ReactTextView
coado Oct 24, 2024
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
4 changes: 4 additions & 0 deletions ...-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,10 @@ private Layout measureSpannedText(Spannable text, float width, YogaMeasureMode w
.setBreakStrategy(mTextBreakStrategy)
.setHyphenationFrequency(mHyphenationFrequency);

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
builder.setJustificationMode(mJustificationMode);
}

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
builder.setUseLineSpacingFromFallbacks(true);
}
Expand Down
3 changes: 2 additions & 1 deletion .../react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,8 +380,9 @@ protected void onDraw(Canvas canvas) {
getBreakStrategy(),
getHyphenationFrequency(),
// always passing ALIGN_NORMAL here should be fine, since this method doesn't depend on
// how exacly lines are aligned, just their width
// how exactly lines are aligned, just their width
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct, since justification can change line width.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could get current justification mode or pass -1 as well I think.

Layout.Alignment.ALIGN_NORMAL,
(Build.VERSION.SDK_INT < Build.VERSION_CODES.O) ? -1 : getJustificationMode(),
getPaint());
setText(getSpanned());
}
Expand Down
84 changes: 61 additions & 23 deletions ...ct-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,17 +146,48 @@ public static boolean isRTL(MapBuffer attributedString) {
== LayoutDirection.RTL;
}

private static Layout.Alignment getTextAlignment(MapBuffer attributedString, Spannable spanned) {
@Nullable
private static String getTextAlignmentAttr(MapBuffer attributedString) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is splitting the alignment methods related to core change, or just for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just for consistency and to not retrieve alignment twice from MapBuffer (first from getTextAlignment and second from getTextJustificationMode)

// TODO: Don't read AS_KEY_FRAGMENTS, which may be expensive, and is not present when using
// cached Spannable
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like we chopped off this part of the comment

if (!attributedString.contains(AS_KEY_FRAGMENTS)) {
return Layout.Alignment.ALIGN_NORMAL;
}

// Android will align text based on the script, so normal and opposite alignment needs to be
// swapped when the directions of paragraph and script don't match.
// I.e. paragraph is LTR but script is RTL, text needs to be aligned to the left, which means
// ALIGN_OPPOSITE needs to be used to align RTL script to the left
if (!attributedString.contains(AS_KEY_FRAGMENTS)) {
return null;
}

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)) {
return textAttributes.getString(TextAttributeProps.TA_KEY_ALIGNMENT);
}
}

return null;
}


private static int getTextJustificationMode(@Nullable String alignmentAttr) {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
return -1;
}

if (alignmentAttr != null && alignmentAttr.equals("justified")) {
return Layout.JUSTIFICATION_MODE_INTER_WORD;
}

return Layout.JUSTIFICATION_MODE_NONE;
}

private static Layout.Alignment getTextAlignment(
MapBuffer attributedString,
Spannable spanned,
@Nullable String alignmentAttr) {
boolean isParagraphRTL = isRTL(attributedString);
boolean isScriptRTL =
TextDirectionHeuristics.FIRSTSTRONG_LTR.isRtl(spanned, 0, spanned.length());
Expand All @@ -165,23 +196,17 @@ private static Layout.Alignment getTextAlignment(MapBuffer attributedString, Spa
Layout.Alignment alignment =
swapNormalAndOpposite ? Layout.Alignment.ALIGN_OPPOSITE : Layout.Alignment.ALIGN_NORMAL;

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 (alignmentAttr == null) {
return alignment;
}

if (textAttributes.contains(TextAttributeProps.TA_KEY_ALIGNMENT)) {
String alignmentAttr = textAttributes.getString(TextAttributeProps.TA_KEY_ALIGNMENT);

if (alignmentAttr.equals("center")) {
alignment = Layout.Alignment.ALIGN_CENTER;
} else if (alignmentAttr.equals("right")) {
alignment =
swapNormalAndOpposite
? Layout.Alignment.ALIGN_NORMAL
: Layout.Alignment.ALIGN_OPPOSITE;
}
}
if (alignmentAttr.equals("center")) {
alignment = Layout.Alignment.ALIGN_CENTER;
} else if (alignmentAttr.equals("right")) {
alignment =
swapNormalAndOpposite
? Layout.Alignment.ALIGN_NORMAL
: Layout.Alignment.ALIGN_OPPOSITE;
}

return alignment;
Expand All @@ -190,7 +215,8 @@ private static Layout.Alignment getTextAlignment(MapBuffer attributedString, Spa
public static int getTextGravity(
MapBuffer attributedString, Spannable spanned, int defaultValue) {
int gravity = defaultValue;
Layout.Alignment alignment = getTextAlignment(attributedString, spanned);
@Nullable String alignmentAttr = getTextAlignmentAttr(attributedString);
Layout.Alignment alignment = getTextAlignment(attributedString, spanned, alignmentAttr);

// depending on whether the script is LTR or RTL, ALIGN_NORMAL and ALIGN_OPPOSITE may mean
// different things
Expand Down Expand Up @@ -363,6 +389,7 @@ private static Layout createLayout(
int textBreakStrategy,
int hyphenationFrequency,
Layout.Alignment alignment,
int justificationMode,
TextPaint paint) {
Layout layout;

Expand Down Expand Up @@ -420,6 +447,10 @@ private static Layout createLayout(
.setTextDirection(
isScriptRTL ? TextDirectionHeuristics.RTL : TextDirectionHeuristics.LTR);

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
builder.setJustificationMode(justificationMode);
}

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
builder.setUseLineSpacingFromFallbacks(true);
}
Expand Down Expand Up @@ -503,7 +534,9 @@ private static Layout createLayout(
? paragraphAttributes.getInt(PA_KEY_MAX_NUMBER_OF_LINES)
: ReactConstants.UNSET;

Layout.Alignment alignment = getTextAlignment(attributedString, text);
@Nullable String alignmentAttr = getTextAlignmentAttr(attributedString);
Layout.Alignment alignment = getTextAlignment(attributedString, text, alignmentAttr);
int justificationMode = getTextJustificationMode(alignmentAttr);

if (adjustFontSizeToFit) {
double minimumFontSize =
Expand All @@ -523,6 +556,7 @@ private static Layout createLayout(
textBreakStrategy,
hyphenationFrequency,
alignment,
justificationMode,
paint);
}

Expand All @@ -535,6 +569,7 @@ private static Layout createLayout(
textBreakStrategy,
hyphenationFrequency,
alignment,
justificationMode,
paint);
}

Expand All @@ -550,6 +585,7 @@ private static Layout createLayout(
int textBreakStrategy,
int hyphenationFrequency,
Layout.Alignment alignment,
int justificationMode,
TextPaint paint) {
BoringLayout.Metrics boring = BoringLayout.isBoring(text, paint);
Layout layout =
Expand All @@ -562,6 +598,7 @@ private static Layout createLayout(
textBreakStrategy,
hyphenationFrequency,
alignment,
justificationMode,
paint);

// Minimum font size is 4pts to match the iOS implementation.
Expand Down Expand Up @@ -610,6 +647,7 @@ private static Layout createLayout(
textBreakStrategy,
hyphenationFrequency,
alignment,
justificationMode,
paint);
}
}
Expand Down
Loading