TextFieldValue and onValueChange callback issues [172239032]

Fixed

Feature Request

Status Update

No update yet.

Description

si...@google.com

created issue #1

Nov 2, 2020 04:50PM

The current structure of the editing API where we accept a TextFieldValue and provide changes in onValueChange callback
- is very verbose
- the constraints that composition, selection and text have to be returned at once (~atomically) creates challenges for developers
- assumes that the new value will immediately be provided and this does not work for cases such as

b/160257648

We previously talked that a state object similar to Scoll(er) might be better.

Comments

si...@google.com <si...@google.com> #2Nov 2, 2020 04:54PM

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

si...@google.com <si...@google.com> Nov 3, 2020 01:23AM

Accepted by si...@google.com.

si...@google.com <si...@google.com> #3Nov 16, 2020 07:04AM

While working on the new APi here are the things we want to update in the current API so that we can clear some of the bugs:

Make TextFieldValue.composition readonly (aosp/1499610)
I figured that when composition is "reset" to null (which should happen automatically when text has changed), we do not inform the IME about this fact. This requires us to reset the IME with the new information at hand.
[Optional] provide mechanism to prevent users creating a new TextFieldValue in onValueChange forgetting to set selection, composition. One of the ideas here was to have TextRange.Unspecified that is the default value for selection and composition. This way we can understand the intention of the developer, if they wanted to reset those fields or if they did not change them. This is in order to prevent cases such as b/171860947

si...@google.com <si...@google.com> #4Nov 17, 2020 12:25AM

Nona's comment on naming of the clearComposition:

"commitComposition/cancelComposition" is better than "clearComposition" it is not clear.

I will change the TextFieldValue.clearComposition as TextFieldValue.commitComposition.

Nona also mentioned that cancelComposition would be the version where the composed text is removed and composition range is cleared (which is the naming in EditorBuffer). I will add it when requested.

ap...@google.com <ap...@google.com> #5Nov 17, 2020 04:26AM

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

commit 5520c565b94ed4735ae3f6ed52961e60b8df79cc
Author: Siyamed Sinir <siyamed@google.com>
Date: Sun Nov 15 21:48:58 2020

Make TextFieldValue.composition readonly

- Maked TextFieldValue.composition readonly
- Removed the exception for invalid selection range, and instead
constrains this value. The exception thrown for invalid ranges
created issues for developers while creating a new copy of
TextFieldValue
- Made the three argument constructor internal so that composition
can be set by the original owner of the object, which is
EditProcessor.
- Changed the way that String overloads remembers the previous
selection/composition since now composition cannot be set by those.
- Removed `data class` from TextFieldValue in order to support above
use cases, adds auto generated equals/hashCode methods.
- Added TextFieldValue.commitComposition in order to clear the
composition range in TextFieldValue.

Test: ./gradlew compose:ui:ui-text:test
Test: ./gradlew compose:ui:ui-text:connectedAndroidTest
Test: ./gradlew compose:foundation:foundation:test
Test: ./gradlew compose:foundation:foundation:connectedAndroidTest
Test: ./gradlew compose:material:material:test
Test: ./gradlew compose:material:material:connectedAndroidTest

RelNote: "Updated TextFielValue API
- made TextFieldValue.composition readonly
- removed exception thrown for invalid selection range"

Bug: 172239032

Change-Id: I4a67592c05ab384ad5614cccf50ad6e79be52b55

M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeVariousInputField.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldOnValueChangeTextFieldValueTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldTest.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/BasicTextField.kt
M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldDelegate.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/ui/ui-text/api/current.txt
M compose/ui/ui-text/api/public_plus_experimental_current.txt
M compose/ui/ui-text/api/restricted_current.txt
M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/TextFieldValue.kt
M compose/ui/ui-text/src/test/java/androidx/compose/ui/text/input/TextFieldValueTest.kt

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

ap...@google.com <ap...@google.com> #6Nov 18, 2020 09:07PM

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

commit fe038a6d89484699e921a2a546e5d15e976fb9cf
Author: Siyamed Sinir <siyamed@google.com>
Date: Sun Nov 15 21:48:58 2020

Reset input when composition or text changes

When the text or composition changes by the application
IME needs to be informed about this fact. Otherwise
IME thinks that their data is correct and in synch. An easy
culprit for this issue is: if onValueChange always rejects the
given input, keyboard still shows/updates suggestions thinking
that the data that was previously sent is visible to user.

When the IME is not informed what is happening, also the duplicate
text issues occurs since IME still thinks that the composition
range is correct, and that is what the user sees.

This CL changes the EditProcessor behavior to compare the previously
suggested state to the new state it gets. If
- the text is different
- the composition is different

It resets the input.

Test: Added new tests
Test: ./gradlew compose:ui:ui-text:test
Test: ./gradlew compose:ui:ui-text:connectedAndroidTest
Test: ./gradlew compose:ui:ui:test
Test: ./gradlew compose:ui:ui:connectedAndroidTest
Test: ./gradlew compose:foundation:foundation:test
Test: ./gradlew compose:foundation:foundation:connectedAndroidTest

RelNote: Added resetInput parameter to TextInputService#onStateUpdated

Bug: 172239032
Bug: 171860947
Change-Id: I3e8f5404553921bd94ae424d2840ca5595b6f90b

M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/TextDemos.kt
A compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/TextFieldValueDemo.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldTest.kt
M compose/integration-tests/src/main/java/androidx/ui/integration/test/core/text/TextFieldToggleTextTestCase.kt
M compose/ui/ui-text/api/current.txt
M compose/ui/ui-text/api/public_plus_experimental_current.txt
M compose/ui/ui-text/api/restricted_current.txt
M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/EditProcessor.kt
M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/TextFieldValue.kt
M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/TextInputService.kt
M compose/ui/ui-text/src/test/java/androidx/compose/ui/text/TextInputServiceTest.kt
M compose/ui/ui-text/src/test/java/androidx/compose/ui/text/input/EditProcessorTest.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/text/input/RecordingInputConnection.kt
M compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/text/input/TextInputServiceAndroid.kt
M compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/platform/DesktopPlatformInput.kt
A compose/ui/ui/src/test/kotlin/androidx/compose/ui/input/TextInputServiceAndroidOnStateUpdateTest.kt

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

ap...@google.com <ap...@google.com> #7Nov 20, 2020 05:47PM

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

commit d320f0fc8de7e207c27bc475da32f252421fe526
Author: Siyamed Sinir <siyamed@google.com>
Date: Thu Nov 19 19:32:16 2020

Fix EditProcessor selection update

We passed selection.start and end, where as we should have
passed selection.min and max since Editing buffer expects
correct ordering.

Bug: 173728118
Bug: 172239032
Bug: 171860947

Test: ./gradlew compose:ui:ui-text:test

Change-Id: If1c04102da5da59cecd127f0d8759b46b153e086

M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/EditProcessor.kt
M compose/ui/ui-text/src/test/java/androidx/compose/ui/text/input/EditProcessorTest.kt

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

ap...@google.com <ap...@google.com> #8Nov 20, 2020 10:40PM

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

commit 663ecc0e55e3b1ca5b8a4391215a5ae074fd1197
Author: Siyamed Sinir <siyamed@google.com>
Date: Mon Nov 16 22:51:52 2020

Cleanup TextFieldValue usage

This CLs removes the TextFieldValue usage if it is not
required, and replaces with string versions.

Test: Treehugger
Test: Ran the demo app and checked some screens
Test: ./gradlew compose:foundation:foundation:test
Test: ./gradlew compose:foundation:foundation:connectedAndroidTest
Test: ./gradlew compose:material:material:test
Test: ./gradlew compose:material:material:connectedAndroidTest

Bug: 172239032
Change-Id: Ifa2b14d53be4259e45a6bfd7506712111354d1a5

M compose/androidview/androidview/integration-tests/androidview-demos/src/main/java/androidx/compose/androidview/demos/FocusInteropAndroidInCompose.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeInputField.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeInputFieldFocusTransition.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeInputFieldMinMaxLines.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeInputFieldTrickyUseCase.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/ComposeVariousInputField.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/TailFollowingTextField.kt
M compose/foundation/foundation/integration-tests/foundation-demos/src/main/java/androidx/compose/foundation/demos/text/TextFieldWIthScroller.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/SoftwareKeyboardTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldCursorTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldDefaultWidthTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldFocusTest.kt
M compose/foundation/foundation/src/androidAndroidTest/kotlin/androidx/compose/foundation/TextFieldTest.kt
M compose/integration-tests/demos/src/main/java/androidx/compose/integration/demos/DemoApp.kt
M compose/integration-tests/demos/src/main/java/androidx/compose/integration/demos/DemoFilter.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/textfield/OutlinedTextFieldTest.kt
M compose/material/material/src/androidAndroidTest/kotlin/androidx/compose/material/textfield/TextFieldTest.kt
M compose/runtime/runtime/samples/src/main/java/androidx/compose/runtime/samples/ModelSamples.kt
M compose/runtime/runtime/samples/src/main/java/androidx/compose/runtime/samples/MutableStateListSamples.kt
M compose/runtime/runtime/samples/src/main/java/androidx/compose/runtime/samples/MutableStateMapSample.kt
M compose/ui/ui-test/src/androidAndroidTest/kotlin/androidx/compose/ui/test/TextActionsTest.kt
M compose/ui/ui/integration-tests/ui-demos/src/main/java/androidx/compose/ui/demos/PopupDemo.kt
M compose/ui/ui/integration-tests/ui-demos/src/main/java/androidx/compose/ui/demos/autofill/ExplicitAutofillTypesDemo.kt
M compose/ui/ui/src/androidAndroidTest/kotlin/androidx/compose/ui/AccessibilityIteratorsTest.kt
M navigation/navigation-compose/integration-tests/navigation-demos/src/main/java/androidx/navigation/compose/demos/NavByDeepLinkDemo.kt
M navigation/navigation-compose/integration-tests/navigation-demos/src/main/java/androidx/navigation/compose/demos/NavSingleTopDemo.kt
M navigation/navigation-compose/integration-tests/navigation-demos/src/main/java/androidx/navigation/compose/demos/NavWithArgsDemo.kt

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

ap...@google.com <ap...@google.com> #9Nov 20, 2020 10:40PM

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

commit 7077c279c44c31eee08cd0db86e63df1d118ab36
Author: Siyamed Sinir <siyamed@google.com>
Date: Fri Nov 20 11:53:59 2020

Prevent extra recomposition in TextField String overload

Test: Ran the TextToggleBenchmark and TextFieldToggleBenchmark

Bug: 172239032
Bug: 173795358
Change-Id: I61a3e343cc4696fe0f74b6f8f96c1d9a360f1daa

M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/BasicTextField.kt
M compose/integration-tests/benchmark/src/androidTest/java/androidx/compose/ui/TextFieldToggleTextBenchmark.kt
M compose/integration-tests/src/main/java/androidx/ui/integration/test/core/text/TextFieldToggleTextTestCase.kt
M compose/integration-tests/src/main/java/androidx/ui/integration/test/core/text/TextToggleTextTestCase.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

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

ap...@google.com <ap...@google.com> #10Nov 21, 2020 03:11AM

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

commit 59dcdf10d45c422addfb863a0d98c27a782145da
Author: Siyamed Sinir <siyamed@google.com>
Date: Fri Nov 20 14:55:42 2020

Fix selection not being updated in edit processor

Cursor position and composition clear was being applied
to EditProcessor and therefore we could not know about the
state that came from IME vs internal state changes.

Moved blur and cursor positioning to be an update on the
existing last known TextFieldValue instead of applying
changes to buffer.

Test: ./gradlew compose:foundation:foundation:test
Test: ./gradlew compose:foundation:foundation:connectedAndroidTest
Test: ./gradlew compose:ui:ui-text:test
Test: ./gradlew compose:ui:ui-text:connectedAndroidTest
Test: Manual test in the demo app

Bug: 172239032
Change-Id: I7c7763ace19b754bad27e1649c2504869b32aef9

M compose/foundation/foundation/src/commonMain/kotlin/androidx/compose/foundation/text/TextFieldDelegate.kt
M compose/foundation/foundation/src/test/kotlin/androidx/compose/foundation/text/TextFieldDelegateTest.kt
M compose/ui/ui-text/src/commonMain/kotlin/androidx/compose/ui/text/input/EditProcessor.kt
M compose/ui/ui-text/src/test/java/androidx/compose/ui/text/input/EditProcessorTest.kt

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

tc...@google.com <tc...@google.com> #11Jan 8, 2021 02:41AM

This feature is currently on the Compose Beta hotlist or one of its dependencies. Can you please confirm that you intend to complete it in time for Beta by adding it to one of the upcoming iterations or remove it from the hotlist? Thank you!

si...@google.com <si...@google.com> Jun 15, 2021 04:18PM

Marked as fixed.

na...@gmail.com <na...@gmail.com> #12Jun 16, 2021 06:42PM

Could you please elaborate on how this was fixed? I am still experiencing an issue related to this (160257648) in Compose beta09.

si...@google.com <si...@google.com> #13Jun 16, 2021 08:42PM

Some work was done here. The issue that initiated this ticket b/160257648 is still open.