Skip to content

Commit

Permalink
Reimplement Android lineHeight positioning/determination (facebook#47271
Browse files Browse the repository at this point in the history
)

Summary:
Pull Request resolved: facebook#47271

Let's keep the recent goals of centering (instead of arbitrary prioritizing ascent), ala spec, and make some changes to allow overlapping interior line-boxes, and make the implementation a lot simpler, instead of the cruft it has been accumulating.

The new simple versions is implemented as the only `CustomLineHeightSpan`. This replaces the code used when `enableAndroidLineHeightCentering` is enabled (which is now the default).

Legacy path is renamed to `LegacyLineHeightSpan`, slated to be deleted if rollout goes well.

We cannot yet cause text to overflow the bounds of the underlying TextView until potentially large later work related to ReactTextView reimplementation.

There's a somewhat arbitrary choice here, when rounding, to whether we ceil ascent vs descent when pixels don't evenly split. This does result in a visual difference, and for sake of avoiding breakage of screenshots, I left the same choice as before.

Changelog:
[Android][Fixed] - Reimplement Android lineHeight positioning/determination

Reviewed By: javache

Differential Revision: D64716557
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Oct 29, 2024
1 parent 9a8459d commit 505830e
Show file tree
Hide file tree
Showing 10 changed files with 132 additions and 226 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @generated SignedSource<<57ac7a4a01e8639c22244a237a6dba1d>>
* @generated SignedSource<<09dd6fb517b7b965c5cc554a075e5590>>
*/

/**
Expand Down Expand Up @@ -35,7 +35,7 @@ public open class ReactNativeFeatureFlagsDefaults : ReactNativeFeatureFlagsProvi

override fun enableAlignItemsBaselineOnFabricIOS(): Boolean = true

override fun enableAndroidLineHeightCentering(): Boolean = false
override fun enableAndroidLineHeightCentering(): Boolean = true

override fun enableBridgelessArchitecture(): Boolean = false

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.facebook.react.bridge.ReadableArray;
import com.facebook.react.bridge.ReadableMap;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags;
import com.facebook.react.uimanager.IllegalViewOperationException;
import com.facebook.react.uimanager.LayoutShadowNode;
import com.facebook.react.uimanager.NativeViewHierarchyOptimizer;
Expand All @@ -34,6 +35,7 @@
import com.facebook.react.views.text.internal.span.CustomLetterSpacingSpan;
import com.facebook.react.views.text.internal.span.CustomLineHeightSpan;
import com.facebook.react.views.text.internal.span.CustomStyleSpan;
import com.facebook.react.views.text.internal.span.LegacyLineHeightSpan;
import com.facebook.react.views.text.internal.span.ReactAbsoluteSizeSpan;
import com.facebook.react.views.text.internal.span.ReactBackgroundColorSpan;
import com.facebook.react.views.text.internal.span.ReactClickableSpan;
Expand Down Expand Up @@ -229,7 +231,11 @@ private static void buildSpannedFromShadowNode(
if (!Float.isNaN(effectiveLineHeight)
&& (parentTextAttributes == null
|| parentTextAttributes.getEffectiveLineHeight() != effectiveLineHeight)) {
ops.add(new SetSpanOperation(start, end, new CustomLineHeightSpan(effectiveLineHeight)));
if (ReactNativeFeatureFlags.enableAndroidLineHeightCentering()) {
ops.add(new SetSpanOperation(start, end, new CustomLineHeightSpan(effectiveLineHeight)));
} else {
ops.add(new SetSpanOperation(start, end, new LegacyLineHeightSpan(effectiveLineHeight)));
}
}
ops.add(new SetSpanOperation(start, end, new ReactTagSpan(textShadowNode.getReactTag())));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,14 @@
import com.facebook.react.common.ReactConstants;
import com.facebook.react.common.build.ReactBuildConfig;
import com.facebook.react.common.mapbuffer.MapBuffer;
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags;
import com.facebook.react.uimanager.PixelUtil;
import com.facebook.react.uimanager.ReactAccessibilityDelegate.AccessibilityRole;
import com.facebook.react.uimanager.ReactAccessibilityDelegate.Role;
import com.facebook.react.views.text.internal.span.CustomLetterSpacingSpan;
import com.facebook.react.views.text.internal.span.CustomLineHeightSpan;
import com.facebook.react.views.text.internal.span.CustomStyleSpan;
import com.facebook.react.views.text.internal.span.LegacyLineHeightSpan;
import com.facebook.react.views.text.internal.span.ReactAbsoluteSizeSpan;
import com.facebook.react.views.text.internal.span.ReactBackgroundColorSpan;
import com.facebook.react.views.text.internal.span.ReactClickableSpan;
Expand Down Expand Up @@ -317,9 +319,15 @@ private static void buildSpannableFromFragments(
textAttributes.mTextShadowColor)));
}
if (!Float.isNaN(textAttributes.getEffectiveLineHeight())) {
ops.add(
new SetSpanOperation(
start, end, new CustomLineHeightSpan(textAttributes.getEffectiveLineHeight())));
if (ReactNativeFeatureFlags.enableAndroidLineHeightCentering()) {
ops.add(
new SetSpanOperation(
start, end, new CustomLineHeightSpan(textAttributes.getEffectiveLineHeight())));
} else {
ops.add(
new SetSpanOperation(
start, end, new LegacyLineHeightSpan(textAttributes.getEffectiveLineHeight())));
}
}

ops.add(new SetSpanOperation(start, end, new ReactTagSpan(reactTag)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,105 +9,48 @@ package com.facebook.react.views.text.internal.span

import android.graphics.Paint.FontMetricsInt
import android.text.style.LineHeightSpan
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags
import kotlin.math.ceil
import kotlin.math.floor
import kotlin.math.min

/**
* We use a custom [LineHeightSpan], because `lineSpacingExtra` is broken. Details here:
* https://github.com/facebook/react-native/issues/7546
* Implements a [LineHeightSpan] which follows web-like behavior for line height, unlike
* LineHeightSpan.Standard which only effects space between the baselines of adjacent line boxes
* (does not impact space before the first line or after the last).
*/
public class CustomLineHeightSpan(height: Float) : LineHeightSpan, ReactSpan {
public val lineHeight: Int = ceil(height.toDouble()).toInt()

private fun chooseCenteredHeight(fm: FontMetricsInt) {
if (fm.descent > lineHeight) {
// Show as much descent as possible
fm.descent = lineHeight
fm.bottom = 0
fm.top = 0
fm.ascent = 0
} else if (-fm.ascent + fm.descent > lineHeight) {
// Calculate the amount we are over, and split the adjustment between descent and ascent
val difference = -(lineHeight + fm.ascent - fm.descent) / 2
val remainder = difference % 2
fm.ascent = fm.ascent + difference
fm.descent = fm.descent - difference - remainder
fm.top = fm.ascent
fm.bottom = fm.descent
} else if (-fm.top + fm.bottom > lineHeight) {
// Calculate the amount we are over, and split the adjustment between top and bottom

val excess = ((-fm.top + fm.bottom) - lineHeight) / 2

fm.top += excess
fm.bottom -= excess
} else {
// Show proportionally additional ascent / top & descent / bottom
val additional = lineHeight - (-fm.top + fm.bottom)

// Round up for the negative values and down for the positive values (arbitrary choice)
// So that bottom - top equals additional even if it's an odd number.
val top = (fm.top - ceil(additional / 2.0f)).toInt()
val bottom = (fm.bottom + floor(additional / 2.0f)).toInt()

fm.top = top
fm.ascent = top
fm.descent = bottom
fm.bottom = bottom
}
}

private fun chooseOriginalHeight(fm: FontMetricsInt) {
// This is more complicated that I wanted it to be. You can find a good explanation of what the
// FontMetrics mean here: http://stackoverflow.com/questions/27631736.
// The general solution is that if there's not enough height to show the full line height, we
// will prioritize in this order: descent, ascent, bottom, top

if (fm.descent > lineHeight) {
// Show as much descent as possible
fm.descent = min(lineHeight.toDouble(), fm.descent.toDouble()).toInt()
fm.bottom = fm.descent
fm.ascent = 0
fm.top = fm.ascent
} else if (-fm.ascent + fm.descent > lineHeight) {
// Show all descent, and as much ascent as possible
fm.bottom = fm.descent
fm.ascent = -lineHeight + fm.descent
fm.top = fm.ascent
} else if (-fm.ascent + fm.bottom > lineHeight) {
// Show all ascent, descent, as much bottom as possible
fm.top = fm.ascent
fm.bottom = fm.ascent + lineHeight
} else if (-fm.top + fm.bottom > lineHeight) {
// Show all ascent, descent, bottom, as much top as possible
fm.top = fm.bottom - lineHeight
} else {
// Show proportionally additional ascent / top & descent / bottom
val additional = lineHeight - (-fm.top + fm.bottom)

// Round up for the negative values and down for the positive values (arbitrary choice)
// So that bottom - top equals additional even if it's an odd number.
val top = (fm.top - ceil(additional / 2.0f)).toInt()
val bottom = (fm.bottom + floor(additional / 2.0f)).toInt()

fm.top = top
fm.ascent = top
fm.descent = bottom
fm.bottom = bottom
}
}

public override fun chooseHeight(
text: CharSequence?,
text: CharSequence,
start: Int,
end: Int,
spanstartv: Int,
v: Int,
fm: FontMetricsInt,
) {
if (ReactNativeFeatureFlags.enableAndroidLineHeightCentering()) chooseCenteredHeight(fm)
else chooseOriginalHeight(fm)
// https://www.w3.org/TR/css-inline-3/#inline-height
// When its computed line-height is not normal, its layout bounds are derived solely from
// metrics of its first available font (ignoring glyphs from other fonts), and leading is used
// to adjust the effective A and D to add up to the used line-height. Calculate the leading L as
// L = line-height - (A + D). Half the leading (its half-leading) is added above A of the first
// available font, and the other half below D of the first available font, giving an effective
// ascent above the baseline of A′ = A + L/2, and an effective descent of D′ = D + L/2. However,
// if line-fit-edge is not leading and this is not the root inline box, if the half-leading is
// positive, treat it as zero. The layout bounds exactly encloses this effective A′ and D′.

val leading = lineHeight - ((-fm.ascent) + fm.descent)
fm.ascent -= ceil(leading / 2.0f).toInt()
fm.descent += floor(leading / 2.0f).toInt()

// The top of the first line, and the bottom of the last line, may influence bounds of the
// paragraph, so we match them to the text ascent/descent. It is otherwise desirable to allow
// line boxes to overlap (to allow too large glyphs to be drawn outside them), so we do not
// adjust the top/bottom of interior line-boxes.
if (start == 0) {
fm.top = fm.ascent
}
if (end == text.length) {
fm.bottom = fm.descent
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

package com.facebook.react.views.text.internal.span

import android.graphics.Paint.FontMetricsInt
import android.text.style.LineHeightSpan
import kotlin.math.ceil
import kotlin.math.floor
import kotlin.math.min

/**
* We use a custom [LineHeightSpan], because `lineSpacingExtra` is broken. Details here:
* https://github.com/facebook/react-native/issues/7546
*/
public class LegacyLineHeightSpan(height: Float) : LineHeightSpan, ReactSpan {
public val lineHeight: Int = ceil(height.toDouble()).toInt()

public override fun chooseHeight(
text: CharSequence?,
start: Int,
end: Int,
spanstartv: Int,
v: Int,
fm: FontMetricsInt,
) {
// This is more complicated that I wanted it to be. You can find a good explanation of what the
// FontMetrics mean here: http://stackoverflow.com/questions/27631736.
// The general solution is that if there's not enough height to show the full line height, we
// will prioritize in this order: descent, ascent, bottom, top

if (fm.descent > lineHeight) {
// Show as much descent as possible
fm.descent = min(lineHeight.toDouble(), fm.descent.toDouble()).toInt()
fm.bottom = fm.descent
fm.ascent = 0
fm.top = fm.ascent
} else if (-fm.ascent + fm.descent > lineHeight) {
// Show all descent, and as much ascent as possible
fm.bottom = fm.descent
fm.ascent = -lineHeight + fm.descent
fm.top = fm.ascent
} else if (-fm.ascent + fm.bottom > lineHeight) {
// Show all ascent, descent, as much bottom as possible
fm.top = fm.ascent
fm.bottom = fm.ascent + lineHeight
} else if (-fm.top + fm.bottom > lineHeight) {
// Show all ascent, descent, bottom, as much top as possible
fm.top = fm.bottom - lineHeight
} else {
// Show proportionally additional ascent / top & descent / bottom
val additional = lineHeight - (-fm.top + fm.bottom)

// Round up for the negative values and down for the positive values (arbitrary choice)
// So that bottom - top equals additional even if it's an odd number.
val top = (fm.top - ceil(additional / 2.0f)).toInt()
val bottom = (fm.bottom + floor(additional / 2.0f)).toInt()

fm.top = top
fm.ascent = top
fm.descent = bottom
fm.bottom = bottom
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
import com.facebook.react.bridge.ReactSoftExceptionLogger;
import com.facebook.react.common.ReactConstants;
import com.facebook.react.common.build.ReactBuildConfig;
import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags;
import com.facebook.react.uimanager.BackgroundStyleApplicator;
import com.facebook.react.uimanager.LengthPercentage;
import com.facebook.react.uimanager.LengthPercentageType;
Expand All @@ -71,6 +72,7 @@
import com.facebook.react.views.text.internal.span.CustomLetterSpacingSpan;
import com.facebook.react.views.text.internal.span.CustomLineHeightSpan;
import com.facebook.react.views.text.internal.span.CustomStyleSpan;
import com.facebook.react.views.text.internal.span.LegacyLineHeightSpan;
import com.facebook.react.views.text.internal.span.ReactAbsoluteSizeSpan;
import com.facebook.react.views.text.internal.span.ReactBackgroundColorSpan;
import com.facebook.react.views.text.internal.span.ReactForegroundColorSpan;
Expand Down Expand Up @@ -866,7 +868,13 @@ private void addSpansFromStyleAttributes(SpannableStringBuilder workingText) {

float lineHeight = mTextAttributes.getEffectiveLineHeight();
if (!Float.isNaN(lineHeight)) {
workingText.setSpan(new CustomLineHeightSpan(lineHeight), 0, workingText.length(), spanFlags);
if (ReactNativeFeatureFlags.enableAndroidLineHeightCentering()) {
workingText.setSpan(
new CustomLineHeightSpan(lineHeight), 0, workingText.length(), spanFlags);
} else {
workingText.setSpan(
new LegacyLineHeightSpan(lineHeight), 0, workingText.length(), spanFlags);
}
}
}

Expand Down
Loading

0 comments on commit 505830e

Please sign in to comment.