[Android] Make some spans clickable (pills, links) in TextView #860
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #860 +/- ##
============================================
+ Coverage 87.44% 89.34% +1.90%
============================================
Files 165 86 -79
Lines 18816 15247 -3569
Branches 1024 0 -1024
============================================
- Hits 16453 13622 -2831
+ Misses 2077 1625 -452
+ Partials 286 0 -286
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| MotionEvent.ACTION_UP -> { | ||
| // Find selection matching the pointer coordinates | ||
| val offset = getOffsetForPosition(event.x, event.y) | ||
| // Look for clickable spans in that position | ||
| val spans = (text as? Spanned)?.getSpans(offset, offset, Any::class.java) ?: return false |
There was a problem hiding this comment.
Nice idea! A few thoughts which may or may not not be issues:
- Have you tested if there are any conflicts using
setMovementMethod(LinkMovementMethod)after this change? - What happens if an app returns a custom clickable span in the mention display handler?
- Could this trigger if a user scrolls by dragging a link/pill/mention?
There was a problem hiding this comment.
- Have you tested if there are any conflicts using
setMovementMethod(LinkMovementMethod)after this change?
I think LinkMovementMethod needs the TextView to be selectable, which we don't allow at the moment, but I'd have to double check. Maybe instead of returning true in this case I should call the super function. Nice catch.
- What happens if an app returns a custom clickable span in the mention display handler?
I think internally it would be represented by a CustomMentionSpan, which is a ReplacementSpan and can't contain a ClickableSpan. With TextDisplay.Custom you can just pass a ReplacementSpan if I'm not mistaken.
- Could this trigger if a user scrolls by dragging a link/pill/mention?
Oh, nice catch. Maybe I should perform this same search in ACTION_DOWN, keep a reference to the selected spans and then check if I get the same ones in ACTION_UP.
There was a problem hiding this comment.
I couldn't add any automated tests for the 1st question, but I can confirm it works. I also added some new tests and made sure scrolling won't trigger a click (which apparently does happen with the default ClickableSpan).
| @@ -128,4 +134,38 @@ open class EditorStyledTextView : AppCompatTextView { | |||
| } | |||
| ) | |||
| } | |||
|
|
|||
| override fun onTouchEvent(event: MotionEvent?): Boolean { | |||
There was a problem hiding this comment.
Please could you also add a UI test for the new behaviour?
There was a problem hiding this comment.
I'll try to, but it's not that simple to select a pill in the TextView. Maybe if I make the only content of the TextView it'll make things easier.
There was a problem hiding this comment.
No problem - thanks for adding these tests!
- Made sure autodetected links are clickable. - Make sure touch events are properly recognised and can't affect scrolling. - Add missing tests.
| if (span.url == null) continue | ||
| onLinkClickedListener?.invoke(span.url) | ||
| MotionEvent.ACTION_MOVE -> { | ||
| val distance = initialTouchCoordinates?.let { hypot(it.first - event.x, it.second - event.y) } ?: 0f |
There was a problem hiding this comment.
We could skip a this calculation too by doing if (!maybeClickEvent) return here.
There was a problem hiding this comment.
I guess we could, but I think ACTION_MOVE can only happen after ACTION_DOWN, so maybeClickEvent should always be true at that point. I can add it, just in case.
There was a problem hiding this comment.
I thought ACTION_MOVE could be called many times during scrolling before ACTION_UP so thought of skipping any after maybeClickEvent gets reset here. Maybe I missed something though?
There was a problem hiding this comment.
No, you're right, I completely forgot it could be called several times. In any case, using the GestureDetector makes this code no longer needed, so I'll remove it.
| onLinkClickedListener?.invoke(span.url) | ||
| MotionEvent.ACTION_MOVE -> { | ||
| val distance = initialTouchCoordinates?.let { hypot(it.first - event.x, it.second - event.y) } ?: 0f | ||
| // If the distance between the initial coordinates and the current ones is too big (4dp), we can assume it's not a click |
There was a problem hiding this comment.
I am worried that manual detection of scroll / click gestures could be a bit error prone and lead to something which is either too sensitive or not sensitive enough. I wonder if there is a way to avoid implementing this ourselves manually?
I saw GestureDetector which looks like it does something similar - is it worth looking into?
There was a problem hiding this comment.
SimpleGestureDetector does just this and simplifies the code a lot. Thanks for the suggestion!
| @@ -128,4 +134,38 @@ open class EditorStyledTextView : AppCompatTextView { | |||
| } | |||
| ) | |||
| } | |||
|
|
|||
| override fun onTouchEvent(event: MotionEvent?): Boolean { | |||
There was a problem hiding this comment.
No problem - thanks for adding these tests!
|
Kudos, SonarCloud Quality Gate passed!
|
|
Oops, I merged this before the CI passed for a couple of jobs. I guess I'm too used to the 'auto merge when all required jobs pass' feature on the EXA repo 😅 . |
urltoPillSpanandCustomReplacementSpan.CustomReplacementSpantoCustomMentionSpan.EditorStyledTextView.onTouchEventto detect clicks in these clickable spans.onLinkClickedListenerto get notified when they are clicked.