nhaarman / acorn

Mastering Android navigation :chipmunk:
https://nhaarman.github.io/acorn
Apache License 2.0
181 stars 7 forks source link

Activity theme not applied into child views #150

Closed gumil closed 4 years ago

gumil commented 4 years ago

Activity theme not applied into child views Child views does not inherit activity theme might be because setContentView is not called.

To Reproduce I have created all views programmatically and does not depend on XML. Issues found so far:

  1. Child views did not inherit colors from theme
  2. selectableItemBackground does not resolve ripple animation since it does not get the right theme.

Expected behavior Activity theme should be applied into child views

Environmental info (please complete the following information):

nhaarman commented 4 years ago

Could you provide some sample code of how you're creating the views?

Note that views should be created using the Activity context which contains the proper theme, and when not created from XML you should pass the proper defStyleAttr to the View's constructor (e.g. android.R.attr.textViewStyle for TextView).

Whether or not setContentView is used should not play any role here.

gumil commented 4 years ago

Views are created using ViewProvidingScene's createViewController parent context parameter. The colors were not inherited like colorPrimary when Acorn was used. But setting the view directly to setContentView it got the right colors.

nhaarman commented 4 years ago

Are you using AcornAppCompatActivity?

gumil commented 4 years ago

Yes, and I am relying to the colorPrimary etc.

nhaarman commented 4 years ago

Hm, this should work actually. The parent: ViewGroup provided in ViewProvidingScene is derived from the FrameLayout provided by the system through android.R.id.content, which obviously contains the theme info as necessary.

Activity.setContentView delegates to PhoneWindow.setContentView, which uses the same Activity context instance as provided through the parent instance, so there's no further magic happening there to make themes work.

Without a reproducible example there's not really much to go on from here. Would it be possible to create a sample project that consistently reproduces the issue?

gumil commented 4 years ago

With Acorn Scenes: Screenshot_1568651983

class MainActivity : AcornAppCompatActivity() {
    override fun provideNavigatorProvider(): NavigatorProvider {
        return DemoNavigatorProvider
    }
}

setContentView: Screenshot_1568652005

class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        setContentView(HomeView(this, DemoModel.values().toList()))
    }
}

The whole code is here https://github.com/gumil/acorn-demo

nhaarman commented 4 years ago

Thanks! That should help a lot.

nhaarman commented 4 years ago

The issue is more easily reproduced like so:

class MainActivity : AppCompatActivity() {

    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)
        //setContentView(TextView(this).apply { text = "HELLO WORLD" })
        setContentView(TextView(findViewById<ViewGroup>(R.id.content).context).apply { text = "HELLO WORLD" })
    }
}

The assumption I made earlier is that the Context passed was the same instance in both cases seems to be wrong: the second instance (from the android.R.id.content View) is a ContextThemeWrapper. The baseContext of this wrapper is in fact the MainActivity instance, so this 'fixes' the theming issue again:

 val context = (findViewById<ViewGroup>(R.id.content).context as ContextThemeWrapper).baseContext
 setContentView(TextView(context).apply { text = "HELLO WORLD" })
nhaarman commented 4 years ago

It seems AppCompat does some hacky things under the hood, by removing the android.R.id.content id from the original View and applying it to its own view:

https://android.googlesource.com/platform/frameworks/support/+/d25af35/v7/appcompat/src/android/support/v7/app/AppCompatDelegateImplV7.java#390

// Change our content FrameLayout to use the android.R.id.content id.
// Useful for fragments.
decorContent.setId(View.NO_ID);
abcContent.setId(android.R.id.content);

This abcContent View is created using an 'emulated' ContextThemeWrapper:

/**
--
* This needs some explanation. As we can not use the android:theme attribute
* pre-L, we emulate it by manually creating a LayoutInflater using a
* ContextThemeWrapper pointing to actionBarTheme.
*/
TypedValue outValue = new TypedValue();
mContext.getTheme().resolveAttribute(R.attr.actionBarTheme, outValue, true);
themedContext = new ContextThemeWrapper(mContext, outValue.resourceId);

This ContextThemeWrapper is the instance we're seeing in our issue here, which resolves to the ActionBarTheme and not the app's theme..

I'm not sure yet why LayoutInflater.from that ContextThemeWrapper uses proper theming though (which is why I haven't seen this issue before).

nhaarman commented 4 years ago

A quick-and-dirty fix could be to insert a FrameLayout in between with the proper Context instance like so:

class MainActivity : AcornAppCompatActivity() {

    private lateinit var rootView: ViewGroup

    override fun onCreate(savedInstanceState: Bundle?) {
        rootView = FrameLayout(this)
        setContentView(rootView)
        super.onCreate(savedInstanceState)
    }

    override fun provideNavigatorProvider(): NavigatorProvider {
        return DemoNavigatorProvider
    }

    override fun provideRootView(): ViewGroup {
        return rootView
    }
}

I'll see and find if there's a 'proper' fix for this, or if something like this will be included in AcornAppCompatActivity.

gumil commented 4 years ago

Thank you for further investigating this!

I think this workaround works. Seems also like an edge case since I am just experimenting on Acorn and programmatic views with Contour. I just have this idea that when Jetpack Compose is released, programmatic views will be more common.

nhaarman commented 4 years ago

Yeah it's a basic case that really should work though. Thanks again for the repro!

nhaarman commented 4 years ago

An investigative issue on the android tracker: https://issuetracker.google.com/issues/141726320