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

Support selectable={true} property on Text fields on Android. #8028

Closed
wants to merge 1 commit into from

Conversation

@mikelambert
Copy link
Contributor

mikelambert commented Jun 9, 2016

Explain the motivation for making this change. What existing problem does the pull request solve?

This adds support for a text field that the user can click-and-drag to select text (which can then be copied using the native selected-text-hover-widget).

I'd love to add this to iOS too, but iOS appears to draw glyphs directly to the screen for its widget (versus using UITextField), so it might be too difficult to support there. But at least I can support my Android users with this change.

Let me know if/what kind of "demonstrate the code is solid" you would like for this. A screenshot of selected text with this property set?

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 9, 2016

By analyzing the blame information on this pull request, we identified @sahrens and @grabbou to be potential reviewers.

@@ -90,6 +91,10 @@ const Text = React.createClass({
*/
onLongPress: React.PropTypes.func,
/**
* Lets the user select text, to use the native copy and paste functionality.

This comment has been minimized.

Copy link
@grabbou

grabbou Jun 9, 2016

Collaborator

I believe you should add @platform android to the doc block along with selectableAndroid (kind of thing) so it's clear it's not supported on the iOS.

This comment has been minimized.

Copy link
@mikelambert

mikelambert Jun 10, 2016

Author Contributor

Added the @platform android, thanks for the reminder. But I'm confused what you mean by selectableAndroid?

This comment has been minimized.

Copy link
@andreicoman11

andreicoman11 Jun 15, 2016

Contributor

I think selectable is correct here, we will want to support this on iOS as well. The platform suffix makes more sense when the prop is platform specific and cannot/will not be supported on the other platform.

@mikelambert mikelambert force-pushed the mikelambert:master branch 2 times, most recently from afdda00 to cbfc6f7 Jun 10, 2016
@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Jun 14, 2016

Looks reasonable, @andreicoman11 what do you think?

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Jun 14, 2016

but iOS appears to draw glyphs directly to the screen for its widget

Does this mean iOS UITextField or whatever class we're using doesn't have a way to enable / disable selection?

Let me know if/what kind of "demonstrate the code is solid" you would like for this. A screenshot of selected text with this property set?

Yes screenshots with on / off would be helpful for the review. 👍

@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 14, 2016

@mkonicek from what I understand, you don't actually use a UITextField on iOS. (I wish, it would be so much simpler for me, if so!) Instead it looks like <Text> is RCTText.m, which is a raw UIView whose drawRect calls drawGlyphsForGlyphRange instead.

Yes, <TextInput multiline={true}> does use a RCTTextView.m object which is a UITextView, but my last attempt at trying to repurpose this to support a <TextInput multiline={true} editable={false} selectable={true} /> didn't work.

Anyways, I'll grab some screenshots for you.

@@ -97,6 +97,11 @@ public void setLineHeight(ReactTextView view, float lineHeight) {
}
}

@ReactProp(name = "selectable")
public void setSelectable(ReactTextView view, boolean placeholder) {

This comment has been minimized.

Copy link
@andreicoman11

andreicoman11 Jun 15, 2016

Contributor

nit: isSelectable instead of placeholder

@andreicoman11

This comment has been minimized.

Copy link
Contributor

andreicoman11 commented Jun 15, 2016

I think an example and/or test for this prop would help here.

@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 15, 2016

Selected text in the demo app

It's basically using the off-the-shelf example app, with:

  • A branch of RN 0.27-stable with my change cherrypicked in
  • Adding selectable={true} to the three fields
@satya164

This comment has been minimized.

Copy link
Contributor

satya164 commented Jun 16, 2016

That's so cool @mikelambert. Thanks for doing this.

@satya164

This comment has been minimized.

Copy link
Contributor

satya164 commented Jun 16, 2016

Also would be super-awesome if you could add this to UIExplorer.

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Jun 17, 2016

@mikelambert Thanks for the screenshot! Please remove the argument placeholder to isSelectable and let's merge this 🎉

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Jun 17, 2016

Adding an example in the TextExample would be awesome and quite easy, could you do that please?
https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/TextExample.android.js

Here's how to run the UIExplorer:
https://github.com/facebook/react-native/tree/master/Examples/UIExplorer#running-on-android

@mikelambert mikelambert force-pushed the mikelambert:master branch from cbfc6f7 to d20a96c Jun 19, 2016
@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 19, 2016

Fixed parameter name, and added UIExplorer example.

@andreicoman11

This comment has been minimized.

Copy link
Contributor

andreicoman11 commented Jun 20, 2016

@facebook-github-bot-0 shipit

@mikelambert mikelambert force-pushed the mikelambert:master branch from 45654c9 to 18823c6 Jun 20, 2016
@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 21, 2016

Thanks! Unfortunately, this didn't auto-submit, so I merged to master and re-pushed. And now it's still not submitting, looks like there's an issue with the travis CI build, unrelated to my change?

@andreicoman11

This comment has been minimized.

Copy link
Contributor

andreicoman11 commented Jun 21, 2016

Can you rebase and try again? cc @bestander are there issues with the travis CI build?

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Jun 21, 2016

Yeah, travis occasionally pull this thing on us.
I restarted the build

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Jun 21, 2016

Yeah, @andreicoman11, looks like Travis got wacky in the last 2 days, on it now

@mikelambert mikelambert force-pushed the mikelambert:master branch from 18823c6 to 345f08d Jun 21, 2016
@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 22, 2016

Okay, I've gotten two of three checks completed (thanks bestander), but now it's stuck waiting for status to be reported on "no-direct-merges", with no Details link or hint as to what I have (or it has) done wrong. Or should I be more patient and keep waiting?

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Jun 22, 2016

Ignore th no-direct-merges

This is a technical check that hides merge button.

On Wednesday, 22 June 2016, Mike Lambert notifications@github.com wrote:

Okay, I've gotten two of three checks completed (thanks bestander), but
now it's stuck waiting for status to be reported on "no-direct-merges",
with no Details link or hint as to what I have (or it has) done wrong. Or
should I be more patient and keep waiting?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8028 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ACBdWHwLDq-LX9PAQPCpfyLS3tCKq84Fks5qOVgRgaJpZM4IyKwp
.

@mikelambert

This comment has been minimized.

Copy link
Contributor Author

mikelambert commented Jun 23, 2016

Ahh thanks. But now I am confused what blocking this pull request from being merged, given that andreicoman11 already said "shipit"?

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Jun 23, 2016

@ghost

This comment has been minimized.

Copy link

ghost commented Jun 23, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@bestander

This comment has been minimized.

Copy link
Contributor

bestander commented Jun 23, 2016

The bot just likes me more :)

@ghost ghost closed this in 6cd7127 Jun 23, 2016
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 6, 2016
Summary:
Explain the **motivation** for making this change. What existing problem does the pull request solve?

This adds support for a text field that the user can click-and-drag to select text (which can then be copied using the native selected-text-hover-widget).

I'd love to add this to iOS too, but iOS appears to draw glyphs directly to the screen for its <Text> widget (versus using UITextField), so it might be too difficult to support there. But at least I can support my Android users with this change.

Let me know if/what kind of "demonstrate the code is solid" you would like for this. A screenshot of selected text with this property set?
Closes facebook/react-native#8028

Differential Revision: D3474196

Pulled By: bestander

fbshipit-source-id: 8d3656681265a0e6229bfa13ff2ae021e894d3cd
samerce added a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
Explain the **motivation** for making this change. What existing problem does the pull request solve?

This adds support for a text field that the user can click-and-drag to select text (which can then be copied using the native selected-text-hover-widget).

I'd love to add this to iOS too, but iOS appears to draw glyphs directly to the screen for its <Text> widget (versus using UITextField), so it might be too difficult to support there. But at least I can support my Android users with this change.

Let me know if/what kind of "demonstrate the code is solid" you would like for this. A screenshot of selected text with this property set?
Closes facebook#8028

Differential Revision: D3474196

Pulled By: bestander

fbshipit-source-id: 8d3656681265a0e6229bfa13ff2ae021e894d3cd
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Explain the **motivation** for making this change. What existing problem does the pull request solve?

This adds support for a text field that the user can click-and-drag to select text (which can then be copied using the native selected-text-hover-widget).

I'd love to add this to iOS too, but iOS appears to draw glyphs directly to the screen for its <Text> widget (versus using UITextField), so it might be too difficult to support there. But at least I can support my Android users with this change.

Let me know if/what kind of "demonstrate the code is solid" you would like for this. A screenshot of selected text with this property set?
Closes facebook#8028

Differential Revision: D3474196

Pulled By: bestander

fbshipit-source-id: 8d3656681265a0e6229bfa13ff2ae021e894d3cd
@drewvolz drewvolz mentioned this pull request Nov 16, 2016
12 of 20 tasks complete
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.