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

nested text onPress not working on last character #30928

Closed

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Feb 10, 2021

Summary

This issue fixes #22747 nested text does not allow you to press on the last character.

The method reactTagForTouch filters touches based on coordinates x and y. Nested Texts are converted into Spans on Android

for (int i = 0, length = textShadowNode.getChildCount(); i < length; i++) {
ReactShadowNode child = textShadowNode.getChildAt(i);

https://developer.android.com/guide/topics/text/spans

reactTagForTouch iterates over the span and triggers the onPress handler if the x,y coordinates correspond to one of the span characters.

Changelog

[Android] [Fixed] - Nested Text Android onPress does not work with last character

Test Plan

This changes fix the Java API which can not be tested as explained in commit 709a441
The java TextTest was excluded from the test suite in commit 709a441 as they need the Yoga libraries to run

TEST - Clicking on the last letter triggers the callback

Clicking on the last letter does not invoke the onPress callback (in this case a console.warn)

BEFORE

Clicking on the last letter does invoke the onPress callback (in this case a console.warn)

AFTER

TEST - Adding and removing Text

2021-02-10.17-41-44.mp4

TEST - Different type of languages

2021-02-10.17-44-20.mp4

TEST - Testing other Examples that use onPress handler

2021-02-10.17-46-24.mp4

TEST - Text with Inline Images

Inline View Inline Image is clipped

@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 Feb 10, 2021
@analysis-bot
Copy link

analysis-bot commented Feb 10, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 4324ca8

@@ -359,7 +359,7 @@ public int reactTagForTouch(float touchX, float touchY) {
for (int i = 0; i < spans.length; i++) {
int spanStart = spannedText.getSpanStart(spans[i]);
int spanEnd = spannedText.getSpanEnd(spans[i]);
if (spanEnd > index && (spanEnd - spanStart) <= targetSpanTextLength) {
if (spanEnd >= index && (spanEnd - spanStart) <= targetSpanTextLength) {
Copy link
Contributor Author

@fabOnReact fabOnReact Feb 10, 2021

Choose a reason for hiding this comment

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

I am asking myself why it did work on non-nested Text.
I need to find the reason spanEnd reaches the index value on the last characted when the Text is nested in another Text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue was not experienced on non-nested text as normal Text will not generate Spans. Spans are generated by nested Text react tags

@fabOnReact
Copy link
Contributor Author

fabOnReact commented Feb 11, 2021

I investigated the current failures in the CI and they not are related to this pr. I tried but could not find a solution to the failures, which relate to AppCompat (Error inflating class com.android.internal.widget.ActionBarContextView)

Unluckily the Java Unit Test for ReactTextTest were disabled for an issue with Yoga linking t14964130

# TODO Disabled temporarily until Yoga linking is fixed t14964130
# srcs = glob(['**/*.java']),

Related Discussions #11570 #11511 (comment)

I will invest my time writing Java tests for other functionalities as this seems pretty hard issues to solve. Thanks

@analysis-bot
Copy link

analysis-bot commented Feb 11, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,891,081 +4
android hermes armeabi-v7a 8,394,800 +1
android hermes x86 9,380,336 +6
android hermes x86_64 9,323,049 -9
android jsc arm64-v8a 10,341,057 +6
android jsc armeabi-v7a 9,827,875 -1
android jsc x86 10,391,813 +0
android jsc x86_64 10,974,795 +4

Base commit: 4324ca8

@fabOnReact fabOnReact marked this pull request as ready for review February 23, 2021 14:55
@parveen-bhatia
Copy link

parveen-bhatia commented Sep 17, 2021

Facing same issue. What are the plans to move it forward ?

Thanks @fabriziobertoglio1987 for this. This patch is working for us.

@charlesbdudley charlesbdudley self-assigned this Sep 20, 2021
@facebook-github-bot
Copy link
Contributor

@charlesbdudley has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Nested Text Android] onPress hitbox has an incorrect size.
5 participants