TextField API encourages duplication of state [162234081]

Obsolete

Bug

Status Update

No update yet.

Description

se...@google.com

created issue #1

Jul 27, 2020 04:45PM

Compose: dev14

The current API of TextField requiring a TextFieldValue promotes developers duplicating state inside a composable.

The common code is something similar to this:

@Composable
fun DuplicatedState(text: String, onTextChanged: (String) -> Unit) {
    var textFieldValue by savedInstanceState { TextFieldValue(text)}
    textFieldValue = textFieldValue.copy(text = text)
    TextField(value = textFieldValue, onValueChange = {
        onTextChanged(it.text)
        textFieldValue = it
    })
}

Developers have express that they don't feel like the selection state (etc) of the TextField is appropriate state to hoist to a parent (e.g. a ViewModel).

We should provide primitives overrides for TextField similar to the Material TextField options to allow developers to express this more simply:

@Composable
fun NewApi(text: String, onTextChange: (String) -> Unit) {
    TextField(value = text, onValueChange = onTextChange)
}

Comments

so...@google.com <so...@google.com> #2Jul 27, 2020 04:55PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ma...@google.com <ma...@google.com> #3Jul 27, 2020 05:26PM

do we want to have the primitive override in a single 'core' text field

I don't think so. This whole bug is a side effect of the naming problem we've already fixed. I would suggest to close this bug as fixed because of renaming and original problem being infeasible anymore.

If someone is reimplementing their TextField for their design need, I expect them to be ok with TextFieldValue hoisting or them be able to wrap this API with their string overload.

It seems reasonable to separate selection range and value long term, but maybe I'm missing some cases there where this might not work. Over to Siyamed to give some input here

Message last modified on Jul 27, 2020 05:27PM

se...@google.com <se...@google.com> #4Aug 6, 2020 02:58AM

I'm not actually sure that the Material TextFields fully replace this API – it's pretty common to need text input in various contexts that aren't well supported by the styling of a material TextField.

In particular, the label is often undesired in interactive inputs (search fields / inline data entry) and typical applications will need to use BaseTextField in typical application coding.

se...@google.com <se...@google.com> #5Aug 6, 2020 02:59AM

I'm currently working on a TODO sample app and am copy/pasting the code from TextField to manage state into the sample. I can't use either of the material text fields in this UI location.

@OptIn(ExperimentalFoundationApi::class)
@Composable
fun TodoTextField(value: String, onValueChange: (String) -> Unit, modifier: Modifier = Modifier) {
    // this code is used to manage the selection and composition state of the BaseTextField
    var textFieldValue by state { TextFieldValue() }
    if (textFieldValue.text != value) {
        @OptIn(InternalTextApi::class)
        textFieldValue = TextFieldValue(
            text = value,
            selection = textFieldValue.selection.constrain(0, value.length)
        )
    }
    BaseTextField(
        value = textFieldValue,
        onValueChange = {
            val previousValue = textFieldValue.text
            textFieldValue = it
            if (previousValue != it.text) {
                onValueChange(it.text)
            }
        },
        modifier = modifier
    )
}

It seems in general if we find that a material component requires an API like this for usability the core equivalent should be exposed similarly.

Message last modified on Aug 6, 2020 03:11AM

so...@google.com <so...@google.com> #6Aug 6, 2020 10:50AM

One thought is that maybe the label parameter should be optional in material text fields? Even though you can do label = {}, but label parameter being non-optional maybe makes it confusing.

ap...@google.com <ap...@google.com> #7Aug 27, 2020 01:22PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit 9e43642ea25a9c3f3a63cc7ddd6ef7eafc6348f0
Author: Anastasia Soboleva <soboleva@google.com>
Date: Wed Aug 12 10:10:17 2020

Make placeholder animated and label optional

Before this change label was a required slot API. But often label is not needed, and having it non-optional sends a wrong signal. With this change label is optional similar to other slots inside material text fields.

Placeholder animates on focus/unfocus as per MD specs.

Test: material tests passed + checked the demo
Fixes: 161519460
Bug: 162234081
Relnote: "Label became an optional parameter inside TextField and OutlinedTextField"
Change-Id: I267f6ada96a3371aaa99bdaa4007229ab7efddab

M compose/material/material/api/current.txt
M compose/material/material/api/public_plus_experimental_current.txt
M compose/material/material/api/restricted_current.txt
M compose/material/material/samples/src/main/java/androidx/compose/material/samples/TextFieldSamples.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/OutlinedTextField.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/TextField.kt
M compose/material/material/src/commonMain/kotlin/androidx/compose/material/TextFieldImpl.kt

https://android-review.googlesource.com/1398697

ma...@google.com <ma...@google.com> #8Jun 14, 2021 01:19PM

Assigned to so...@google.com.

Assigning to Anastasia for future work or to close

so...@google.com <so...@google.com> #9Jul 1, 2021 03:01PM

Status: Won't Fix (Obsolete)

We now provide both String and TextFieldValue overrides in both material and foundation text fields.

Message last modified on Jul 1, 2021 03:01PM