AppCompatTextHelper.onSetTextAppearance does not set textColorHint or textColorLink [154702995]

Fixed

Bug

Status Update

No update yet.

Description

at...@monzo.com

created issue #1

Apr 22, 2020 03:58PM

Artifact used: androidx.appcompat:appcompat
Version: 1.2.0-beta01
Theme: Theme.MaterialComponents.Light.NoActionBar
Devices/Android versions: API 22 emulator

Issue

Setting the text appearance programmatically with TextViewCompat.setTextAppearance(TextView, Int) on an AppCompatEditText does not take into account theme attrs being used in ColorStateList as android:textColorHint (or android:textColorLink).

So on Lollipop, such a hint color is rendered as red (see screenshot).

Cause

Internally, it delegates to AppCompatTextHelper.onSetTextAppearance, which doesn't handle these two attributes - it only considers android:textColor:

if (Build.VERSION.SDK_INT < 23 && a.hasValue(R.styleable.TextAppearance_android_textColor)) {
    // If we're running on < API 23, the text color may contain theme references
    // so let's re-set using our own inflater
    final ColorStateList textColor = a.getColorStateList(R.styleable.TextAppearance_android_textColor);
    if (textColor != null) {
        mView.setTextColor(textColor);
    }
}

However, it does consider them in loadFromAttributes() so it seems like it's an oversight.

Feature request/bug fix

Update AppCompatTextHelper to handle theme attributes in CSLs for android:textColorHint and android:textColorLink in onSetTextAppearance() as it's done in loadFromAttributes().

Workaround

For folks in the meantime - call this too, after calling TextViewCompat.setTextAppearance().

/**
 * Reapply text color and textColorHint to support CSLs on Lollipop
 */
private fun TextView.resetTextColorsFromTextAppearance(@StyleRes textAppearance: Int) {
    if (Build.VERSION.SDK_INT >= 23) {
        // this fix is only necessary on API 21/22 because we need to load the ColorStateLists ourselves
        return
    }

    val attributes = intArrayOf(android.R.attr.textColor, android.R.attr.textColorHint)
    val typedArray = context.obtainStyledAttributes(textAppearance, attributes)
    val textColorRes = typedArray.getResourceId(attributes.indexOf(android.R.attr.textColor), -1)
    val textColorHintRes = typedArray.getResourceId(attributes.indexOf(android.R.attr.textColorHint), -1)
    typedArray.recycle()

    if (textColorRes != -1) {
        setTextColor(AppCompatResources.getColorStateList(context, textColorRes))
    }

    if (textColorHintRes != -1) {
        setHintTextColor(AppCompatResources.getColorStateList(context, textColorHintRes))
    }
}

79970016-33584e80-848a-11ea-81bf-94879c26575e.png

53 KB

View

Download

Comments

ki...@google.com <ki...@google.com> #2Apr 27, 2020 01:18PM

Assigned to ki...@google.com.

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

ki...@google.com <ki...@google.com> #3Apr 27, 2020 01:22PM

The code in question dates back to 2016 for hint color, and to 2017 to link color.

Not worth spinning another beta for, imo.

he...@ataulm.com <he...@ataulm.com> #4Apr 27, 2020 01:25PM

>In your workaround you probably intended to add code for TextAppearance_android_textColorHint and TextAppearance_android_textColorLink

:+1:

>Not worth spinning another beta for, imo.

I don't follow whether this means not fixing it at all, or not fixing for a specific version?

ki...@google.com <ki...@google.com> #5Apr 27, 2020 01:31PM

We'll definitely fix it, but most probably for 1.3.0-alpha01

he...@ataulm.com <he...@ataulm.com> #6Apr 27, 2020 01:36PM

Perfect thanks!

For us, there's no urgency as we're wrapping usages of `TextViewCompat.setTextAppearance` with our own function and applying this workaround on Lollipop, but would be great to ditch it at some point because no doubt this sort of thing is the source of subtle bugs/divergences in intended behaviour.

ki...@google.com <ki...@google.com> May 4, 2020 05:03PM

Accepted by ki...@google.com.

ki...@google.com <ki...@google.com> #7May 5, 2020 05:08PM

https://android-review.googlesource.com/c/platform/frameworks/support/+/1302103 under review

ap...@google.com <ap...@google.com> #8May 5, 2020 06:50PM

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

commit 7869860590c185a90287c9b94e1de151b0c75504
Author: Kirill Grouchnikov <kirillg@google.com>
Date: Tue May 05 12:55:01 2020

Consistent resolution of text hint and link colors

Same as for changing set appearance and resolving the main
text color - do the same for text link and text hint colors.

Test: ./gradlew appcompat:appcompat:connectedCheck --info --daemon -Pandroid.testInstrumentationRunnerArguments.class=androidx.appcompat.widget.AppCompatEditTextTest
Test: ./gradlew appcompat:appcompat:connectedCheck --info --daemon -Pandroid.testInstrumentationRunnerArguments.class=androidx.appcompat.widget.AppCompatTextViewTest
Bug: 154702995
Change-Id: Iddb7f1e7f17b9bceb14c20ca0b3468b10092b8f9

M appcompat/appcompat/src/androidTest/AndroidManifest.xml
M appcompat/appcompat/src/androidTest/java/androidx/appcompat/widget/AppCompatEditTextTest.java
M appcompat/appcompat/src/androidTest/java/androidx/appcompat/widget/AppCompatTextViewTest.java
A appcompat/appcompat/src/androidTest/res/color/color_state_list_hint.xml
A appcompat/appcompat/src/androidTest/res/color/color_state_list_refs12.xml
A appcompat/appcompat/src/androidTest/res/color/color_state_list_refs34.xml
M appcompat/appcompat/src/androidTest/res/layout/appcompat_edittext_activity.xml
A appcompat/appcompat/src/androidTest/res/values/attrs.xml
M appcompat/appcompat/src/androidTest/res/values/strings.xml
M appcompat/appcompat/src/androidTest/res/values/styles.xml
M appcompat/appcompat/src/main/java/androidx/appcompat/widget/AppCompatTextHelper.java

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

ki...@google.com <ki...@google.com> May 5, 2020 06:50PM

Marked as fixed.