Skip to content

Fix incorrect hit testing on text when layout reused #52692

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,18 @@ public PreparedLayout prepareTextLayout(
: null);
}

@AnyThread
@ThreadConfined(ANY)
@UnstableReactNativeAPI
public PreparedLayout reusePreparedLayoutWithNewReactTags(
PreparedLayout preparedLayout, int[] reactTags) {
return new PreparedLayout(
preparedLayout.getLayout(),
preparedLayout.getMaximumNumberOfLines(),
preparedLayout.getVerticalOffset(),
reactTags);
}

@AnyThread
@ThreadConfined(ANY)
@UnstableReactNativeAPI
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,6 @@ import com.facebook.proguard.annotations.DoNotStrip
internal class PreparedLayout(
val layout: Layout,
val maximumNumberOfLines: Int,
val verticalOffset: Float
val verticalOffset: Float,
val reactTags: IntArray,
)
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import com.facebook.proguard.annotations.DoNotStrip
import com.facebook.react.uimanager.BackgroundStyleApplicator
import com.facebook.react.uimanager.ReactCompoundView
import com.facebook.react.uimanager.style.Overflow
import com.facebook.react.views.text.internal.span.ReactTagSpan
import com.facebook.react.views.text.internal.span.ReactFragmentIndexSpan
import kotlin.collections.ArrayList
import kotlin.math.roundToInt

Expand Down Expand Up @@ -310,8 +310,9 @@ internal class PreparedLayoutTextView(context: Context) : ViewGroup(context), Re
override fun hasOverlappingRendering(): Boolean = false

override fun reactTagForTouch(touchX: Float, touchY: Float): Int =
getSpanInCoords(touchX.roundToInt(), touchY.roundToInt(), ReactTagSpan::class.java)?.reactTag
?: id
getSpanInCoords(touchX.roundToInt(), touchY.roundToInt(), ReactFragmentIndexSpan::class.java)
?.fragmentIndex
?.let { preparedLayout?.reactTags[it] } ?: id

@RequiresApi(api = Build.VERSION_CODES.UPSIDE_DOWN_CAKE)
private object Api34Utils {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ 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
import com.facebook.react.views.text.internal.span.ReactForegroundColorSpan
import com.facebook.react.views.text.internal.span.ReactFragmentIndexSpan
import com.facebook.react.views.text.internal.span.ReactLinkSpan
import com.facebook.react.views.text.internal.span.ReactOpacitySpan
import com.facebook.react.views.text.internal.span.ReactStrikethroughSpan
import com.facebook.react.views.text.internal.span.ReactTagSpan
Expand Down Expand Up @@ -218,7 +220,8 @@ internal object TextLayoutManager {
context: Context,
fragments: MapBuffer,
sb: SpannableStringBuilder,
ops: MutableList<SetSpanOperation>
ops: MutableList<SetSpanOperation>,
outputReactTags: IntArray?
) {
for (i in 0 until fragments.count) {
val fragment = fragments.getMapBuffer(i)
Expand Down Expand Up @@ -249,7 +252,11 @@ internal object TextLayoutManager {
(textAttributes.mAccessibilityRole ==
ReactAccessibilityDelegate.AccessibilityRole.LINK)
if (roleIsLink) {
ops.add(SetSpanOperation(start, end, ReactClickableSpan(reactTag)))
if (ReactNativeFeatureFlags.enablePreparedTextLayout()) {
ops.add(SetSpanOperation(start, end, ReactLinkSpan(i)))
} else {
ops.add(SetSpanOperation(start, end, ReactClickableSpan(reactTag)))
}
}
if (textAttributes.mIsColorSet) {
ops.add(SetSpanOperation(start, end, ReactForegroundColorSpan(textAttributes.mColor)))
Expand Down Expand Up @@ -307,7 +314,14 @@ internal object TextLayoutManager {
start, end, CustomLineHeightSpan(textAttributes.effectiveLineHeight)))
}

ops.add(SetSpanOperation(start, end, ReactTagSpan(reactTag)))
if (ReactNativeFeatureFlags.enablePreparedTextLayout()) {
ops.add(SetSpanOperation(start, end, ReactFragmentIndexSpan(i)))
if (outputReactTags != null) {
outputReactTags[i] = reactTag
}
} else {
ops.add(SetSpanOperation(start, end, ReactTagSpan(reactTag)))
}
}
}
}
Expand All @@ -323,7 +337,8 @@ internal object TextLayoutManager {

private fun buildSpannableFromFragmentsOptimized(
context: Context,
fragments: MapBuffer
fragments: MapBuffer,
outputReactTags: IntArray?
): Spannable {
val text = StringBuilder()
val parsedFragments = ArrayList<FragmentAttributes>(fragments.count)
Expand Down Expand Up @@ -363,7 +378,7 @@ internal object TextLayoutManager {
val spannable = SpannableString(text)

var start = 0
for (fragment in parsedFragments) {
for ((i, fragment) in parsedFragments.withIndex()) {
val end = start + fragment.length
val spanFlags =
if (start == 0) Spannable.SPAN_INCLUSIVE_INCLUSIVE else Spannable.SPAN_EXCLUSIVE_INCLUSIVE
Expand All @@ -386,7 +401,11 @@ internal object TextLayoutManager {
ReactAccessibilityDelegate.AccessibilityRole.LINK)

if (roleIsLink) {
spannable.setSpan(ReactClickableSpan(fragment.reactTag), start, end, spanFlags)
if (ReactNativeFeatureFlags.enablePreparedTextLayout()) {
spannable.setSpan(ReactLinkSpan(i), start, end, spanFlags)
} else {
spannable.setSpan(ReactClickableSpan(fragment.reactTag), start, end, spanFlags)
}
}

if (fragment.props.isColorSet) {
Expand Down Expand Up @@ -453,7 +472,14 @@ internal object TextLayoutManager {
CustomLineHeightSpan(fragment.props.effectiveLineHeight), start, end, spanFlags)
}

spannable.setSpan(ReactTagSpan(fragment.reactTag), start, end, spanFlags)
if (ReactNativeFeatureFlags.enablePreparedTextLayout()) {
spannable.setSpan(ReactFragmentIndexSpan(i), start, end, spanFlags)
if (outputReactTags != null) {
outputReactTags[i] = fragment.reactTag
}
} else {
spannable.setSpan(ReactTagSpan(fragment.reactTag), start, end, spanFlags)
}
}

start = end
Expand All @@ -474,21 +500,23 @@ internal object TextLayoutManager {
} else {
text =
createSpannableFromAttributedString(
context, attributedString, reactTextViewManagerCallback)
context,
attributedString.getMapBuffer(AS_KEY_FRAGMENTS),
reactTextViewManagerCallback,
null)
}

return text
}

private fun createSpannableFromAttributedString(
context: Context,
attributedString: MapBuffer,
reactTextViewManagerCallback: ReactTextViewManagerCallback?
fragments: MapBuffer,
reactTextViewManagerCallback: ReactTextViewManagerCallback?,
outputReactTags: IntArray?
): Spannable {
if (ReactNativeFeatureFlags.enableAndroidTextMeasurementOptimizations()) {
val spannable =
buildSpannableFromFragmentsOptimized(
context, attributedString.getMapBuffer(AS_KEY_FRAGMENTS))
val spannable = buildSpannableFromFragmentsOptimized(context, fragments, outputReactTags)

reactTextViewManagerCallback?.onPostProcessSpannable(spannable)
return spannable
Expand All @@ -500,7 +528,7 @@ internal object TextLayoutManager {
// a new spannable will be wiped out
val ops: MutableList<SetSpanOperation> = ArrayList()

buildSpannableFromFragments(context, attributedString.getMapBuffer(AS_KEY_FRAGMENTS), sb, ops)
buildSpannableFromFragments(context, fragments, sb, ops, outputReactTags)

// TODO T31905686: add support for inline Images
// While setting the Spans on the final text, we also check whether any of them are images.
Expand Down Expand Up @@ -756,7 +784,11 @@ internal object TextLayoutManager {
heightYogaMeasureMode: YogaMeasureMode,
reactTextViewManagerCallback: ReactTextViewManagerCallback?
): PreparedLayout {
val text = getOrCreateSpannableForText(context, attributedString, reactTextViewManagerCallback)
val fragments = attributedString.getMapBuffer(AS_KEY_FRAGMENTS)
val reactTags = IntArray(fragments.count)
val text =
createSpannableFromAttributedString(
context, fragments, reactTextViewManagerCallback, reactTags)
val baseTextAttributes =
TextAttributeProps.fromMapBuffer(attributedString.getMapBuffer(AS_KEY_BASE_ATTRIBUTES))
val layout =
Expand All @@ -779,7 +811,7 @@ internal object TextLayoutManager {
getVerticalOffset(
layout, paragraphAttributes, height, heightYogaMeasureMode, maximumNumberOfLines)

return PreparedLayout(layout, maximumNumberOfLines, verticalOffset)
return PreparedLayout(layout, maximumNumberOfLines, verticalOffset, reactTags)
}

@JvmStatic
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* 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

/**
* Maps a section of the text to the index of the AttributedString fragment originally used to
* create it.
*/
internal class ReactFragmentIndexSpan(val fragmentIndex: Int) : ReactSpan
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* 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.text.TextPaint
import android.text.style.ClickableSpan
import android.view.View
import com.facebook.react.bridge.ReactContext
import com.facebook.react.uimanager.UIManagerHelper
import com.facebook.react.views.text.PreparedLayoutTextView
import com.facebook.react.views.text.TextLayoutManager
import com.facebook.react.views.view.ViewGroupClickEvent

/**
* This class is used in [TextLayoutManager] to linkify and style a span of text with
* accessibilityRole="link". This is needed to make nested Text components accessible.
*
* For example, if your React component looks like this:
* ```js
* <Text>
* Some text with
* <Text onPress={onPress} accessible={true} accessibilityRole="link">a link</Text>
* in the middle.
* </Text>
* ```
*/
internal class ReactLinkSpan(val fragmentIndex: Int) : ClickableSpan(), ReactSpan {
override fun onClick(view: View) {
val context = view.context as ReactContext
val textView = view as? PreparedLayoutTextView ?: return
val preparedLayout = textView.preparedLayout ?: return
val reactTag = preparedLayout.reactTags[fragmentIndex]
val eventDispatcher = UIManagerHelper.getEventDispatcherForReactTag(context, reactTag)
eventDispatcher?.dispatchEvent(
ViewGroupClickEvent(UIManagerHelper.getSurfaceId(context), reactTag))
}

override fun updateDrawState(ds: TextPaint) {
// no super call so we don't change the link color or add an underline by default, as the
// superclass does.
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -310,14 +310,19 @@ TextLayoutManager::PreparedLayout TextLayoutManager::prepareLayout(
jfloat,
jfloat)>("prepareTextLayout");

return preparedTextCache_.get(
static auto reusePreparedLayoutWithNewReactTags =
jni::findClassStatic("com/facebook/react/fabric/FabricUIManager")
->getMethod<JPreparedLayout::javaobject(
JPreparedLayout::javaobject, jintArray)>(
"reusePreparedLayoutWithNewReactTags");

const auto [key, preparedText] = preparedTextCache_.getWithKey(
{.attributedString = attributedString,
.paragraphAttributes = paragraphAttributes,
.layoutConstraints = layoutConstraints},
[&] {
[&]() {
const auto& fabricUIManager =
contextContainer_->at<jni::global_ref<jobject>>("FabricUIManager");

auto attributedStringMB = JReadableMapBuffer::createWithContents(
toMapBuffer(attributedString));
auto paragraphAttributesMB = JReadableMapBuffer::createWithContents(
Expand All @@ -336,6 +341,39 @@ TextLayoutManager::PreparedLayout TextLayoutManager::prepareLayout(
minimumSize.height,
maximumSize.height))};
});

// PreparedTextCacheKey allows equality of layouts which are the same
// display-wise, but ShadowView fragments (and thus react tags) may have
// changed.
const auto& fragments = attributedString.getFragments();
const auto& cacheKeyFragments = key->attributedString.getFragments();
bool needsNewReactTags = [&] {
for (size_t i = 0; i < fragments.size(); i++) {
if (fragments[i].parentShadowView.tag !=
cacheKeyFragments[i].parentShadowView.tag) {
return true;
}
}
return false;
}();

if (needsNewReactTags) {
std::vector<int> reactTags(fragments.size());
for (size_t i = 0; i < reactTags.size(); i++) {
reactTags[i] = fragments[i].parentShadowView.tag;
}

auto javaReactTags = jni::JArrayInt::newArray(fragments.size());
javaReactTags->setRegion(
0, static_cast<jsize>(reactTags.size()), reactTags.data());

const auto& fabricUIManager =
contextContainer_->at<jni::global_ref<jobject>>("FabricUIManager");
return PreparedLayout{jni::make_global(reusePreparedLayoutWithNewReactTags(
fabricUIManager, preparedText->get(), javaReactTags.get()))};
} else {
return PreparedLayout{*preparedText};
}
}

TextMeasurement TextLayoutManager::measurePreparedLayout(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,24 +38,20 @@ class SimpleThreadSafeCache {
*/
ValueT get(const KeyT& key, CacheGeneratorFunction<ValueT> auto generator)
const {
std::lock_guard<std::mutex> lock(mutex_);

if (auto it = map_.find(key); it != map_.end()) {
// Move accessed item to front of list
list_.splice(list_.begin(), list_, it->second);
return it->second->second;
}
return getMapIterator(key, std::move(generator))->second->second;
}

auto value = generator();
// Add new value to front of list and map
list_.emplace_front(key, value);
map_[key] = list_.begin();
if (list_.size() > maxSize_) {
// Evict least recently used item (back of list)
map_.erase(list_.back().first);
list_.pop_back();
}
return value;
/*
* Returns pointers to both the key and value from the map with a given key.
* If the value wasn't found in the cache, constructs the value using given
* generator function, stores it inside a cache and returns it.
* Can be called from any thread.
*/
std::pair<const KeyT*, const ValueT*> getWithKey(
const KeyT& key,
CacheGeneratorFunction<ValueT> auto generator) const {
auto it = getMapIterator(key, std::move(generator));
return std::make_pair(&it->first, &it->second->second);
}

/*
Expand All @@ -79,6 +75,29 @@ class SimpleThreadSafeCache {
using EntryT = std::pair<KeyT, ValueT>;
using iterator = typename std::list<EntryT>::iterator;

auto getMapIterator(
const KeyT& key,
CacheGeneratorFunction<ValueT> auto generator) const {
std::lock_guard<std::mutex> lock(mutex_);

if (auto it = map_.find(key); it != map_.end()) {
// Move accessed item to front of list
list_.splice(list_.begin(), list_, it->second);
return it;
}

auto value = generator();
// Add new value to front of list and map
list_.emplace_front(key, value);
auto [it, _] = map_.insert_or_assign(key, list_.begin());
if (list_.size() > maxSize_) {
// Evict least recently used item (back of list)
map_.erase(list_.back().first);
list_.pop_back();
}
return it;
}

size_t maxSize_;
mutable std::mutex mutex_;
mutable std::list<EntryT> list_;
Expand Down
Loading