-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fabric : Implements selectable prop for <Text> #15473
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
base: main
Are you sure you want to change the base?
Conversation
…ndows into text_selectable
…ndows into text_selectable
…t-native-windows into text_selectable
|
Awesome work ! , |
thank you!, for now default color is only supported (windows blue accent color) but will be taken up as soon this is merged(small change !). |
vineethkuttan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
Please create a task to notify the app of selection change (JS). |
|
I remember you talking about SHIFT + click selection. Please create a task for this too to avoid it slipping through the cracks. |
|
[Nit]
reads much nicer than
This is RNW so Windows is implicitly understood. Keep it minimal without data loss. |
|
Please fix formatting issues in What and Changelog sections. |
|
Just pointing out that this change is only about selecting text within a <View style={styles.selectionTestContainer}>
<Text selectable={true} style={styles.sectionTitle}>Text Selection Test</Text>
<Text selectable={true} style={styles.selectableText}>
This text is SELECTABLE. Try clicking and dragging to select it.
</Text>
</View> |
Good point!, this matches iOS and Android. The React Native docs say selectable (ref: https://reactnative.dev/docs/text#selectable ) enables "native copy and paste functionality" - and native text views (UITextView on iOS, TextView on Android) don't support cross-view selection either. Each text view manages its own selection state independently. |
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Outdated
Show resolved
Hide resolved
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.cpp
Outdated
Show resolved
Hide resolved
…t-native-windows into text_selectable
tracking in ref: #15481 |
tracking in ref: #15481 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted comments. Please address them.
vnext/Microsoft.ReactNative/Fabric/Composition/ParagraphComponentView.h
Outdated
Show resolved
Hide resolved
| // Clears any active text selection in the application. | ||
| void ClearCurrentTextSelection() noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from void ClearSelection()? I see different comments b/w them:
// Clears any active text selection in the application.
void ClearCurrentTextSelection() noexcept;
// Called when losing focus, when another text starts selection, or when clicking outside text bounds.
void ClearSelection() noexcept;However, for an implementation (leaf-node) method, when something is done doesn't matter. Only the doing matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that ClearCurrentTextSelection is a free-standing function, not a method of ParagraphComponentView. However, it should be a method of the parent entity which holds all the paragraph component view objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, once you've a strong pointer to a ParagraphComponentView holding the current selection, just call ClearSelection on it. This ClearCurrentTextSelection can be dropped entirely.
| // Returns character position at the given point, or -1 if outside text bounds | ||
| int32_t getTextPositionAtPoint(facebook::react::Point pt) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::optional<uint32_t> GetTextPositionAtPoint(facebook::react::Point pt) noexcept;Consider switching to std::optional instead of using -1 (or other in-band signalling of error conditions) as it's easy to get it wrong. C++17 introduced this exclusively for cases like these. It'd be a must if this were a public method.
Interested to knowing more? Refer std::optional: How, when, and why -- devblogs.microsoft.com.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch to PascalCase.
| float offsetY, | ||
| float pointScaleFactor) noexcept; | ||
| void updateTextAlignment(const std::optional<facebook::react::TextAlignment> &fbAlignment) noexcept; | ||
| bool isTextSelectableAtPoint(facebook::react::Point pt) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be consistent with method naming style. Except for upstream's functions, which use camelCase with override specified, most class functions follow PascalCase.
| int32_t getTextPositionAtPoint(facebook::react::Point pt) noexcept; | ||
|
|
||
| // Returns the currently selected text, or empty string if no selection | ||
| std::string getSelectedText() const noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch to PascalCase.
| int32_t selStart = std::min(m_selectionStart, m_selectionEnd); | ||
| int32_t selEnd = std::max(m_selectionStart, m_selectionEnd); | ||
| if (selStart < 0 || selEnd <= selStart || !m_textLayout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Const correctness all throughout.
| float clampedX = std::max(0.0f, std::min(pt.x, textMetrics.width)); | ||
| float clampedY = std::max(0.0f, std::min(pt.y, textMetrics.height)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Const correctness.
| renderTarget.GetDpi(&oldDpiX, &oldDpiY); | ||
| renderTarget.SetDpi(dpi, dpi); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is risky. Everywhere there's a function exit (normally or due to some error condition) you'd have to remember to reset old DPI on render target. Use a RAII wrapper, a unique_ptr for instance, which would undo this for you with no intervention of your's.
How? Examples with comments: 1 (only line 64 and below are relevant) and 2. Once you have such a type made, you can create an object and upon destruction it'd auto-revert to old DPI.
This is a very useful concept. Once you learn it you can avoid manual resource management for the most part using std::unique_ptr.
| for (UINT32 i = 0; i < actualCount; i++) { | ||
| const auto &metric = hitTestMetrics[i]; | ||
| D2D1_RECT_F rect = {metric.left, metric.top, metric.left + metric.width, metric.top + metric.height}; | ||
| renderTarget.FillRectangle(&rect, selectionBrush.get()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID2D1RenderTarget::FillRectangle already takes DIP (logical) why play with RT's DPI? We should be operating only in the logical coordinates space, no?
Refer Create a simple Direct2D application - learn.microsoft.com where FillRectangle is just in DIP and no mucking around with RT's DPI.
| int32_t selStart = std::min(m_selectionStart, m_selectionEnd); | ||
| int32_t selEnd = std::max(m_selectionStart, m_selectionEnd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking before every usage, you can fix this when writing to these values to always have the lower limit to start and upper limit to end.
Please be mindful that such checks will fail in RTL text (Arabic, etc.). Doing it in one place, taking care of all these nuances, will rid this check off other places. They can blindly read these values without worrying about constructing meaning out of them.
Description
Implements selectable prop for Text component for windows
Includes :
Type of Change
Why
Parity with RN Android/IOS.
Resolves #13112
What
Made upstream changes to fix the issue where the selectable prop wasn’t being passed to native due to a macro conversion problem.
Ref: facebook/react-native#52599
Also updated logic in the Paragraph component view and composition event handler to correctly handle all selection-related scenarios, including text selection, pointer events, copy-to-clipboard, and other related behaviors.
Screenshots
text_selection.mp4
Testing
Tested in playground
Changelog
Should this change be included in the release notes: _indicate yes
Add a brief summary of the change to use in the release notes for the next release.
Adds text Component selection support for Fabric
Microsoft Reviewers: Open in CodeFlow