fix(android-styling): correctly detect if drawable uses ColorFilter

[edusperoni]

PR Checklist

• The PR title follows our guidelines: https://github.com/NativeScript/NativeScript/blob/master/CONTRIBUTING.md#commit-messages.
• There is an issue for the bug/feature this PR is for. To avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it.
• You have signed the CLA.
• All existing tests are passing: https://github.com/NativeScript/NativeScript/blob/master/DevelopmentWorkflow.md#running-unit-tests.
• Tests for the changes are included - https://github.com/NativeScript/NativeScript/blob/master/WritingUnitTests.md.

I reran the tests on an emulator and they all succeeded. iOS tests output the same as without this change.

What is the current behavior?

Changing border-radius makes you unable to change color on Button if border-radius returns to 0

What is the new behavior?

When a background has border-radius and becomes a BorderDrawable, it is no longer considered a background which must have it's color changed by setColorFilter.

Fixes #6341

[edusperoni]

Just ran the tests on iOS, they all passed except for a few that are unrelated to this change.

[vakrilov]

Hey @edusperoni,
Thanks for the issue and the PR. We will review it gat back to you within a few days.
Indeed this PR should not cause any IOS tests to fail - so don't worry about that.

[vakrilov]

test

[manoldonev]

test

[vakrilov]

test

[vakrilov]

test

[vakrilov]

Hey @edusperoni
Sorry for the delay.

All unit tests are green, however we picked a strange behavior that has changed in our e2e tests for android.

It can be reproduced by running this app (make sure you do npm run setup or link the tns-core-modules package in the app). Here are the steps:

1. Run the app
2. Type 3164 and navigate to the issue-3164 page
3. Click on "set border to 3" button (it sets border color and width)
4. Click on "set border to 0" button (it sets the border width back to 0)

We expect the button to regain its original drawable, but with this PR - it doesn't. I noticed that removing the line that clears the _cachedDrawable restores the original behavior. Unit tests seem to pass too. Can you please share whats the reason for adding that code?

[edusperoni]

The reason I added that line was that the moment we changed the background, the cacheddrawable is dirty. This also means the previous background is no longer being referenced, only it's constantState.

This line was added to make sure we're not caching old drawables when creating new ones. So once the view gets a new drawable, it no longer returns to an old drawable.

Now I see it may cause this issue on the specific cases (like button) when setting background to empty means returning to the original (not null/BorderDrawable).

We also had experienced some issues in our projects where if we changed the background to a color and back (something like class="selected" sets a background color, and when removed there is no color), it would try to use the previous state that had been GCed by that point (NativeScript/android#887 on this specific line

NativeScript/tns-core-modules/ui/styling/background.android.ts

Line 78 in 9719a53

defaultDrawable = cachedDrawable.newDrawable(nativeView.getResources())

), so even referencing this native object in a member variable isn't enough to make the GC not collect it.

I believe that line may be removed, but there's probably some underlying issue in caching this StateDrawable,

[radeva]

Hi @edusperoni ,

Congratulations on being one of the winners in the {N} First-time contributors contest!

You can claim your prize by contacting us at nativescriptwinners[at]progress.com not later than Nov 30th 2018 . Make sure you send us the following info: 
👉Your full name 
👉Your email 
👉Your country or residence 

Best regards,
The NativeScript Team

[vakrilov]

Hey @edusperoni,
I would suggest removing the line that clears the cache as it breaks the test.

Do you think you can reproduce any of the other issues you mentioned? It looks to me that they are not related to this PR. Maybe we can work on them separately as long as this PR does not cause this bug to appear more often.

[vakrilov]

test

[SvetoslavTsenov]

test

[dtopuzov]

test

[dtopuzov]

test

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.