-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Enable selection by default for password text field and expose api to… #34676
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
Conversation
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.
end sentences with a .
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 half-sentence just repeats what the first paragraph already said?
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.
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?)
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.
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.
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 think changing it to include the assert will be fine. That we don't have it is more like a bug...
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.
we dont have that because we default it based on obscureText. I will fix this, do you think this will be a breaking change?
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.
why not use macros for these as well?
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.
It has a different default from editable text. same for copy
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.
Is this the right way to get the full length of the text? @GaryQian may know.
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.
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.
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 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.
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.
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.
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.
half-sentence?
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.
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?
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.
nit: space after :
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.
nit: space after :
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.
Maybe include a sentence indicating what the toolbar is used for?
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.
Can this (and the others) just be this.copy = false?
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.
And then just assert that they are not null?
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.
The comment says there should be 3, why did that change?
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.
it used to be copy, paste, and cut.
after this change it by default can only paste
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.
same here.
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.
move this to the line above (or indent).
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.
same
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.
Why a double tap? Shouldn't a single tap be enough deselect?
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.
Can you just use longPressAt (https://master-api.flutter.dev/flutter/flutter_test/WidgetController/longPressAt.html) here and elsewhere where you needed a long press?
|
Addressed all comments @goderbauer |
justinmc
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. 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.
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.
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.
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.
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.
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.
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.
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.
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 :)
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.
Can you explain this section here? Doesn't "isCollapsed true" mean that the text is not selected, or am I reading this wrong?
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.
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.
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.
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.
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.
oh I forgot to update the comment, thanks for catching it
goderbauer
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
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.
| /// press the [EditableText]. It includes several options, cut, copy, paste, | |
| /// press the [EditableText]. It includes several options: cut, copy, paste, |
… turn on and off context menu options
…e api to turn on and off context menu options (flutter#34676)" This reverts commit 0d0af31.
… api to turn on and off context menu options (flutter#34676) This reverts commit 48c7090.
… turn on and off context menu options (flutter#34676)
…e api to turn on and off context menu options (flutter#34676)" (flutter#37055) This reverts commit 0d0af31.
… 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.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?