Skip to content

Conversation

@chunhtai
Copy link
Contributor

… turn on and off context menu options

Description

Enable password select by default, also expose the api to dynamically turn on and off context menu options

Related Issues

#32845

Tests

I added the following tests:

'An obscured TextField has correct default context menu'
'An obscured TextField is selected as one word'
'cut and paste are disabled in read only mode even if explicit set'
'can dynamically disable options in toolbar'

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
  • No, this is not a breaking change.

@shihaohong shihaohong added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. a: text input Entering text in a text field or keyboard related problems and removed f: material design flutter/packages/flutter/material repository. labels Jun 18, 2019
Copy link
Member

Choose a reason for hiding this comment

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

end sentences with a .

Copy link
Member

Choose a reason for hiding this comment

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

this half-sentence just repeats what the first paragraph already said?

Copy link
Member

Choose a reason for hiding this comment

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

The docs say enableInteractiveSelection is true by default. What does it mean if it is null? (Are we just missing an assert in the constructor that this cannot be null?)

Copy link
Contributor Author

@chunhtai chunhtai Jun 19, 2019

Choose a reason for hiding this comment

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

it default to null, I have though about default it to true in constructor and just do bool get selectionEnabled => enableInteractiveSelection

but that will require assert(enableInteractiveSelection!= null) which will change api behavior, previously we allow user pass in null.

Copy link
Member

Choose a reason for hiding this comment

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

I think changing it to include the assert will be fine. That we don't have it is more like a bug...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we dont have that because we default it based on obscureText. I will fix this, do you think this will be a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

why not use macros for these as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has a different default from editable text. same for copy

Copy link
Member

Choose a reason for hiding this comment

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

Is this the right way to get the full length of the text? @GaryQian may know.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, this will indeed produce the length of the text content of the text object. However, to properly and explicitly handle things like PlaceholderSpan/WidgetSpans, you may have to look deeper.

toPlainText will return the concatenated contents for TextSpan. When encountering PlaceholderSpans, the placeholder span will be ignored unless includePlaceholders is true, in which case a 0xFFFC character of length 1 will be included. Whether or not you want this replacement character is a policy choice. In addition, if another text widget is nested using a WidgetSpan, the length of the nested contents will not be included.

Copy link
Contributor

@GaryQian GaryQian Jul 1, 2019

Choose a reason for hiding this comment

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

This looks like it is for obscuring text in a password like environment, which I expect would not be using PlaceholderSpans. It is hard to say what developers would expect if one were to, say, embed all of flutter gallery inline within a password field. How many character lengths is a gallery equivalent to?

Just setting the includePlaceholders flag to true seems to me like a reasonable compromise, where all placeholders/inline widgets will be represented as one character.

Copy link
Contributor Author

@chunhtai chunhtai Jul 10, 2019

Choose a reason for hiding this comment

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

the includePlaceholders is defaulted to true. As of now, there is no way to use any other inlinespan other than textspan in RenderEditable. There is no plan to add it in until we figure out how do we serialize them. We only need to worry about text span for now.

Edit, I was wrong, user can do anything if they use RenderEditable directly... but still we don't know how should we deal with non textspan. I believe let includePlaceholders defaults to true will be good enough.

Copy link
Member

Choose a reason for hiding this comment

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

half-sentence?

Copy link
Member

Choose a reason for hiding this comment

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

These already have a lot of options... would it maybe make sense to give them a single toolbarOptions property that encapsulates all of these and nicely groups them?

@goderbauer goderbauer added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. labels Jun 19, 2019
Copy link
Member

Choose a reason for hiding this comment

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

nit: space after :

Copy link
Member

Choose a reason for hiding this comment

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

nit: space after :

Copy link
Member

Choose a reason for hiding this comment

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

Maybe include a sentence indicating what the toolbar is used for?

Copy link
Member

Choose a reason for hiding this comment

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

Can this (and the others) just be this.copy = false?

Copy link
Member

Choose a reason for hiding this comment

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

And then just assert that they are not null?

Copy link
Member

Choose a reason for hiding this comment

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

The comment says there should be 3, why did that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it used to be copy, paste, and cut.
after this change it by default can only paste

Copy link
Member

Choose a reason for hiding this comment

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

same here.

Copy link
Member

Choose a reason for hiding this comment

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

move this to the line above (or indent).

Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Member

Choose a reason for hiding this comment

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

Why a double tap? Shouldn't a single tap be enough deselect?

Copy link
Member

Choose a reason for hiding this comment

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

@chunhtai
Copy link
Contributor Author

Addressed all comments @goderbauer

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM. This seems like a great full solution to a lot of the confusion that people were having with this. They'll be happy to be able to control the items in the selection menu now.

One thing to think about for the future: What if we allow custom selection menu items? I think this solution should still work but it's something to think through.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you think about asserting or throwing an exception if cut/paste is true and readOnly is true? I'm not sure which is better, just wondering if you thought about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems weird to me, but asserting the property of a toolbarOptions is not a constant expression. This prevent me from doing the check in constant constructor. I might be able to do the check in build, but do you think it is a good approach?
As of whether we should enforce this, I think we shouldn't because It probably will be very common to switch between readonly and non-readonly. It feel weird to me that if I switch to read only i have to explicit disable the option.
But I do want to enforce it in selectable text though but was thinking whether worth it to make the constructor non-const or checking it outside the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hmm I thought you could add an assert in the initializer list while still being const. If not then maybe it's not worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe write this "Toolbar" instead of "Tool bar" (here and below)? I think putting a space in between implies that the B should be capitalized when camel cased ("ToolBar"), but it's not.

This is a definite nitpick though, totally up to you :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this section here? Doesn't "isCollapsed true" mean that the text is not selected, or am I reading this wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if it is selectable, a long press will cause it to select the whole word and the selection will not be collapsed.

I inverted this test and the test above because we are inverted the selection behavior of password textfield.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the comments reversed then? Here it says "Long press does select text." and then it asserts that isCollapsed is true, meaning that there is no selection. Above it says "Long press doesn't select anything." and then asserts isCollapsed is false. Or maybe I'm just confusing myself haha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I forgot to update the comment, thanks for catching it

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// press the [EditableText]. It includes several options, cut, copy, paste,
/// press the [EditableText]. It includes several options: cut, copy, paste,

@chunhtai chunhtai merged commit 0d0af31 into flutter:master Jul 26, 2019
chunhtai added a commit to chunhtai/flutter that referenced this pull request Jul 27, 2019
…e api to turn on and off context menu options (flutter#34676)"

This reverts commit 0d0af31.
chunhtai added a commit that referenced this pull request Jul 27, 2019
…e api to turn on and off context menu options (#34676)" (#37055)

This reverts commit 0d0af31.
chunhtai added a commit to chunhtai/flutter that referenced this pull request Jul 30, 2019
… api to turn on and off context menu options (flutter#34676)

This reverts commit 48c7090.
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
johnsonmh pushed a commit to johnsonmh/flutter that referenced this pull request Jul 30, 2019
…e api to turn on and off context menu options (flutter#34676)" (flutter#37055)

This reverts commit 0d0af31.
chunhtai added a commit that referenced this pull request Jul 30, 2019
… api to turn on and off context menu options (#34676) (#37183)

This reverts commit 48c7090.
goderbauer added a commit that referenced this pull request Jul 31, 2019
…d expose api to turn on and off context menu options (#34676) (#37183)"

This reverts commit 7eb09a8.
goderbauer added a commit that referenced this pull request Jul 31, 2019
…d expose api to turn on and off context menu options (#34676) (#37183)" (#37295)

This reverts commit 7eb09a8.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: text input Entering text in a text field or keyboard related problems f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants