Skip to content
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

Use our themes to fix the default tintMode to SRC_IN. #2902

Merged
merged 1 commit into from
Feb 21, 2019

Conversation

zestyping
Copy link
Contributor

@zestyping zestyping commented Feb 21, 2019

Overview

As of https://github.com/opendatakit/collect/tree/db01cdcf, all disabled image buttons appear the same as enabled image buttons.

This appearance change occurred when colors.xml was edited in b41b817#diff-1f517f1a91abad16231a83752cef21a4 to change the icon colors for disabled buttons (lightIconColorDisabled and darkIconColorDisabled) from solid grey #aaaaaa to partial-opacity-black #61000000 or partial-opacity-white #61ffffff.

The actual cause is that Android uses an incorrect default blending mode for image colour tints.

The blending mode should be SRC_IN, which is the default blending mode everywhere else in Android. However, ImageView uses the default mode SRC_ATOP, which ignores the alpha channel when drawing the icon image.

To fix this problem across our entire app, this PR sets the android:tintMode attribute to src_in in our two themes, light and dark.

What has been done to verify that this works as intended?

I built the app at db01cdcf and ran it in an emulator running Android 6.0, and confirmed that the problem existed (disabled buttons appear black in GeoTrace and GeoShape).

I then built the app with this change and ran it in an emulator running Android 6.0, and confirmed that disabled buttons now appear grey, and they change between grey and black as they should when enabled and disabled. I also tested the dark theme, and the buttons were grey when disabled and white when enabled, as expected.

The tint mode functionality was introduced in Android 5.0, so I also tested the app both with and without this change in an emulator running Android 4.1.1 (API 16, the lowest level we support). Instead of turning grey, the disabled buttons become semitransparent when disabled, and that remains the case both with and without this change. Only the square background of the button becomes semitransparent; the icon remains solid black or solid white. This was also the case in version 1.19.0, before any alpha values were specified in colors.xml.

Why is this the best possible solution? Were any other approaches considered?

I considered the following possible alternative approaches:

  1. Add android:tintMode="src_in" attributes to all <AppCompatButton> tags in our layout XML files. There are 24 occurrences across 5 files.
  2. Extend AppCompatButton with a subclass that calls setImageTintMode(SRC_IN) upon initialization, and use the subclass everywhere instead of AppCompatButton.
  3. Extend AppCompatButton with a subclass that overrides setEnabled to call setImageAlpha with a hardcoded alpha value, and use the subclass everywhere instead of AppCompatButton.

All three of these options involve touching every XML file that uses an image button, and wouldn't solve the problem for future image buttons (though, for Options 2 and 3, we could establish a convention that we always use our subclass instead of AppCompatButton, and even enforce that convention with a lint check).

Options 1 and 2 would have the same visible effect as this PR: buttons turn grey in Android 5.0 and higher, buttons turn semitransparent in Android 4.x.

Option 3 is the only solution I can think of that would actually turn the icon itself grey in all versions of Android. However, it is also by far the most expensive change, and we've lived with semitransparent non-grey buttons in Android 4.x so far, so it doesn't seem worth it.

Editing the theme, as this PR does, is the cheapest option that gets us back to the button appearance that we had in v1.19.0.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

This is a bug fix. Users shouldn't notice any changes.

Do we need any specific form for testing your changes? If so, please attach one.

You can use the Geo map widgets in the "All Widgets" form to see some enabled and disabled image buttons.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

Copy link
Contributor

@cooperka cooperka left a comment

Choose a reason for hiding this comment

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

Simple and effective. Assuming full manual testing comes back positive, this looks good 👍

@yanokwa yanokwa added high priority Should be looked at before other PRs/issues needs testing labels Feb 21, 2019
@yanokwa yanokwa merged commit 9553a18 into getodk:master Feb 21, 2019
@zestyping
Copy link
Contributor Author

In Android 6.0, at db01cdcf (light theme on the left, dark theme on the right):
2019-02-21 21-38-29 2019-02-21 21-37-49

In Android 6.0, after this change:
2019-02-21 21-29-35 2019-02-21 21-30-35

Android 4.1.1 looks the same in v1.19.0, at db01cdcf, and after this change:
2019-02-21 21-02-27 2019-02-21 21-02-54

@mmarciniak90
Copy link
Contributor

Tested with success

Tested on Android 4.2, 4.4, 6.0, 7.0, 8.1

I noticed two possibly related, not critical problems, so I created separate issues for them: #2909 #2908

Verified cases

  • Google/OSM maps
  • light/dark theme
  • GeoShape/GeoTrace widgets

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants