Repository navigation
Conversation
…or consistent paragraph rendering
|
This is quite a change in behaviour. Should these be separate properties so developers can choose which one they want? |
|
@javache it does change the behavior, but I don’t think it introduces a second valid selection mode that needs an opt-in.
An app that intentionally wants “long press copies this whole block” has clearer, explicit ways to implement that interaction (for example, a long-press handler that copies a known string). And implementing this correctly still allows for the previous behavior: you can select the entire block of text and still copy it—so not really a “breaking” change, though slightly different in the UX. It is behavior-changing for apps that may have relied on it, but it’s appropriately a fix: it brings the documented native-selection affordance into line with Android and normal iOS expectations. |
|
I was about to say the exact same thing. Adding a new prop will add a lot of additional work if we consider that approach. P.S. - Solved the CI error, which trimmed the code by a few lines |
|
Lets make the changelog entry here more explicit, and call out that the default behaviour for selectable is quite different with this change. |
Abbondanzo
left a comment
There was a problem hiding this comment.
@javache's comment is fair--this is bringing iOS to parity with Android but should be marked as a breaking change just so we can call it out in the changelog. I was poking through and found some rendering issues with the new approach that should get fixed up first (especially pressable text)
…electableTextView for improved text selection handling
|
Good catch. The cause was one thing: the Split those two jobs:
Your review also turned up a regression I had missed: removing the old Added P.S. - Update the changelog please check |
|
I solved the merge conflicts, and it seems the CI didn't fail due to my changes |
|
Created a patch fix based on this PR and ran it in a production app on React Native 0.86.3, Expo SDK 57. Body text there is one Where the nested nodes are styling only, selection works: handles, and Copy, Look Up, Translate, Search Web and Share acting on the selected range. A paragraph holding a nested pressable One change did not fit 0.86.3. That release has neither Happy to retest once pressable text is sorted. Looking forward to seeing this merged. |
|
@ivannzme thank you for patching this into a real app and testing it. That report found a genuine defect, and it would not have shown up in the examples I had. The cause. A press on a nested paints a highlight, which changed the attributed string. The selection view rebuilt itself in the middle of the touch, so UITextView lost the long press it had already started. The fix. The rebuild now follows the layout string instead of the painted one, so a paint-only change no longer recreates the view. That exposed a second problem: onPress fired while selecting, because React Native's touch handler and the selection gesture track the same touch. The paragraph now gives up the touch once a selection shows. Checked by hand on RNTester, iOS 18.6: pressable children select, decorations survive, scrolling and ordinary link taps still work. @ivannzme a retest would be helpfu |
|
Retested the updated branch on React Native 0.86.3, Expo SDK 57. The pressable-text fix works: a paragraph carrying a nested pressable One new problem, on recycled views. It reads as intermittency from the outside. After a fresh launch the first article selects fine. Once enough screens have been visited for the view pool to warm up, every paragraph is dead, including articles opened for the first time. Creating the layout manager lazily in if (_selectionLayoutManager == nil) {
_selectionLayoutManager = [RCTTextLayoutManager new];
}Eight of eight paragraphs across four article opens after that, including a re-opened article and one with an inline link. |
…add corresponding test
|
@ivannzme thanks, I updated the PR |
|
@Abbondanzo are we good with the changes? Is anything pending on my end? |
|
@Abbondanzo has imported this pull request. If you are a Meta employee, you can view this in D122903141. |
Abbondanzo
left a comment
There was a problem hiding this comment.
@raajnadar Received some fair feedback internally that I'd like to pass along:
- Could you split out the new
RCTSelectableTextViewinterface and implementation into a separate file(s)? - Could you put this change behind a feature gate? The existing behavior should be retained when the gate is off (default off for now). We can run an experiment in turn across our apps to ensure this is rolled out safely
|
@Abbondanzo, thanks for passing this along. Both points are done:
Now, as the PR is non-breaking, I updated the title and the changelog entry |
|
As 0.88.0 is released, I think we can put this on a minor release |
cortinico
left a comment
There was a problem hiding this comment.
Review automatically exported from Phabricator review in Meta.
cipolleschi
left a comment
There was a problem hiding this comment.
Thanks for working on this, partial text selection on iOS is a long-standing request, and having it behind a feature flag is the right call.
I left some inline comments. My main concerns are touch handling (inline children, pointerEvents, concurrent touches), backgrounds covering the selection highlight, and the cost of a second TextKit stack for every selectable paragraph. Could you also add the missing tests and some perf measurements to the Test Plan?
| BOOL needsRebuild = _selectableTextView == nil || ![layoutText isEqualToAttributedString:_selectionRenderedText] || | ||
| !CGSizeEqualToSize(drawingFrame.size, _selectionRenderedSize); |
There was a problem hiding this comment.
The rebuild check only compares the attributed string and the frame size, but the builder also reads maximumNumberOfLines, ellipsizeMode, adjustsFontSizeToFit and the minimum font scale. What happens if a fixed-size paragraph switches from ellipsizeMode="head" to "tail"? As far as I can tell, the selection container keeps the old configuration, and a props-only update never marks the selection as dirty.
Could we include the paragraph attributes in the invalidation check and schedule an update when they change? A test that changes these props on a fixed-frame paragraph would help. The current tests only cover the initial render.
| // Under the drawn paragraph, which is how a native text view stacks the two: | ||
| // UIKit paints the selection, and the glyphs go on top of it. The drawn | ||
| // paragraph passes touches through, so the text view still gets them. | ||
| UIView *container = _textView.superview; | ||
| if (container != nil) { | ||
| [container insertSubview:_selectableTextView belowSubview:_textView]; | ||
| } else { | ||
| [self addSubview:_selectableTextView]; | ||
| } |
There was a problem hiding this comment.
Inserting the text view right below _textView raises a few questions:
- Inline children: they're mounted starting at index 0, so this view ends up above them. Hit testing goes in reverse order and the drawing view returns
nil. So which view gets the touch when the user taps an inlinePressable,TextInputor native control? I think it's the selectionUITextView, even when nothing is selected. The test plan checks that inline views are positioned correctly, but not that they're still interactive. pointerEvents="box-none": before, a plain paragraph let touches through because the drawing view returned no hit. Now theUITextViewis a child that returns a hit. Wouldn't it steal touches from the controls underneath?pointerEvents="box-only": hit testing stops at the paragraph, so theUITextViewrecognizers never see the touch. Now that the paragraph-level long-press recognizer is gone, can the user still select text in this mode?- Backgrounds: the selection view sits below the paragraph's background pass, not just below the glyphs. If a nested
<Text>has an opaquebackgroundColor,drawBackgroundForGlyphRange:still paints it in the drawing view above. Wouldn't that cover the selection highlight? Stripping the background attributes from the selection storage doesn't change what the upper view paints.
Could we keep inline children ahead of the selection view in hit testing (also after a rebuild), preserve the pointerEvents behaviors without the auxiliary view becoming a touch target, and keep the order background → selection → glyphs? A render test that selects a range inside a nested text with an opaque background would catch the last one.
| - (void)_cancelSurfaceTouches | ||
| { | ||
| for (UIView *ancestor = self.superview; ancestor != nil; ancestor = ancestor.superview) { | ||
| for (UIGestureRecognizer *recognizer in ancestor.gestureRecognizers) { | ||
| if ([recognizer isKindOfClass:[RCTSurfaceTouchHandler class]] && recognizer.isEnabled) { | ||
| recognizer.enabled = NO; | ||
| recognizer.enabled = YES; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Disabling and re-enabling RCTSurfaceTouchHandler resets all of its active touches, not just the one that started the selection. What happens if the user holds a finger on another control and selects text with a second finger? I believe the other control's touch gets cancelled too. Also, with nested hosting, the ancestor walk may reset more than the nearest handler. Is that expected?
Could we scope the cancellation to the selection interaction? A regression test with a second, concurrent touch outside the paragraph would cover this.
| if (_selectableTextView.selectedRange.length > 0) { | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
While there's a selection, this returns a null emitter for any touch on the paragraph, wherever it lands. Is that intended? For example, take a padded paragraph inside a Pressable. A tap in the padding, outside the selection view, doesn't fall back to the parent. Dismissing the selection on release can't restore the emitter captured on touch down either.
What about W3C pointer events? Pointer dispatch calls this getter again for pointerUp and pointerCancel, but the selection only resets the touch handler. If the selection becomes non-empty after pointerDown, isn't the final pointer event dropped? Can you verify the callback ordering and the pointer-capture cleanup on a device?
| * A `UITextView` binds its text container at initialisation, so the selectable | ||
| * text view is rebuilt only when the text or the available size changes. | ||
| */ | ||
| - (void)updateSelectableTextViewWithDrawingFrame:(CGRect)drawingFrame |
There was a problem hiding this comment.
This creates a UITextView and a separate TextKit stack for every selectable paragraph, before the user interacts with it. It also doesn't reuse the stack we already built for measurement. Even when nothing needs rebuilding, we convert the whole attributed string, strip it and compare it. When the flag is first enabled, both _updateTextViewFrame and the pending-update branch in finalizeUpdates call this method, so the work happens twice.
Could we do this once per mutation? And can you attach flag-off/flag-on measurements for main-thread mounting, scroll frame times and retained memory? The interesting cases are long paragraphs with rich attributes, text updates and recycled list cells.
| XCTAssertNil(attributes[NSUnderlineStyleAttributeName]); | ||
| XCTAssertNil(attributes[NSStrikethroughStyleAttributeName]); | ||
| XCTAssertNil(attributes[NSShadowAttributeName]); |
There was a problem hiding this comment.
A few gaps in the tests:
- The paint-stripping assertions don't prove much. The fixture uses a wavy decoration (our custom attribute) and no shadow, so the native underline, strikethrough and shadow attributes are
nilfrom the start. Could we add fixtures with a solid decoration and a real shadow, and first assert that the input contains them? - The layout fixtures use default paragraph attributes and a synthetic size, and they don't compare the selection rects with the painted glyphs. How do we know the two match with
adjustsFontSizeToFit/minimumFontScale, truncation, or a compressedlineHeight? - Hiding the
UITextViewstops VoiceOver from reading the text twice (testSelectionTextViewIsHiddenFromAccessibility), but the existing accessibility elements don't expose the selection. Can a VoiceOver user start, adjust and copy a selection? Can you verify that?
Visual regressionNo visual differences at android: no capture |
Summary:
On iOS,
<Text selectable>can only copy the whole paragraph. There is no highlight and no drag handles, so you cannot select part of it. Android has allowed this since the prop was added.Fixes #13938, open since 2017. Also covers #55187, which reports the same gap as an iOS 26 regression‚ it is not a regression. iOS selection has always copied the whole block.
Changelog:
[IOS] [ADDED] - Add the
enableIOSPartialTextSelectionfeature flag. When the flag is on, a long press on<Text selectable>selects a range of text, and Copy copies only that range.Test Plan:
Tested on RNTester on an iPhone 16 Pro simulator, iOS 18.6, built with:
Before and after were captured on the same simulator, with the same paragraph and the same gesture
Each remaining case rendered a selectable paragraph beside an identical non-selectable control, so any difference is attributable to this change.
onPresson a nested<Text>numberOfLines={2}<View>childyarn formatreports no changes on the three files.