1369681 - Custom tabs: seeing flash when launch Custom Tabs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Julian Chu [:walkingice]]

According to feedback from PM, when launching CustomTabsActivity, it is possible seeing a flash with other tab's content.

Its similar to Bug 1347611, but this happens in launch stage.

Comment 1

[Julian Chu [:walkingice]]

I think this problem could be resolved once we have GeckoView based CustomTabs.

For a temporary workaround, I would like to hide the content (R.id.gecko_layout) in `onCreate`, after 300ms delay then set it to be visible. It is not a good implementation but would be removed soon. (Once GeckoView based implementation ready).

Does it make sense to you?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

> I think this problem could be resolved once we have GeckoView based CustomTabs.

Yeah, that should solve it.

> For a temporary workaround, I would like to hide the content (R.id.gecko_layout) in `onCreate`, after 300ms delay then set it to be visible.

Try it. I'm not sure if it's possible to find a value that works for every device in every circumstance but if you are able to make it better for most cases until we can switch to the new implementation then it might be worth doing it.

I read somewhere that we are planning to ship the current state to beta only (until we switch to GeckoView) - In the worst case I would be okay to ship this to Beta as-is (but not release).

Comment 3

[Jan Henning [:JanH]]

I can't guarantee not to have overlooked some edge case, but in principle
- if you're coming through onCreate or onNewIntent, I think you'll always receive either onTabOpenFromIntent or onTabSelectFromIntent afterwards
- if you're just going through onResume, then by the time GeckoApp.onResume() has run (i.e. after calling super.onResume() if overriding in SingleTabActivity) the correct tab should have been selected as well.

So in principle, you could hide the content view going into the background/before resuming and then if resuming through onCreate/onNewIntent, wait for onTabOpenFromIntent/onTabSelectFromIntent, or else just unhide after onResume in GeckoApp.

I've no idea how well that would actually work, though, and of course the above events are relative to when the correct tab has been opened/selected on the UI side - Gecko might need a little more time to catch up.

Comment 4

[Julian Chu [:walkingice]]

Thanks for Sebastian and Jan's advice. I will do this trick only in first launch of CustomTabs. To trigger visibility changing in `onTabOpenFromIntent` looks great.

Comment 5

[Julian Chu [:walkingice]]

Attachment: Bug 1369681 - CustomTab hide content to prevent flash

In some cases, user might see flash in first launch of CustomTabs.
This issue could be fixed once we have GeckoView based implementation.
Before it comes, we just hide content for a while as a workaround.

Review commit: https://reviewboard.mozilla.org/r/145558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145558/

Comment 6

[Jan Henning [:JanH]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

https://reviewboard.mozilla.org/r/145558/#review149484

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:57
(Diff revision 1)
>  
>  import java.util.List;
>  
>  import static org.mozilla.gecko.Tabs.TabEvents;
>  
>  public class CustomTabsActivity extends SingleTabActivity implements Tabs.OnTabsChangedListener {

I'd prefer if you'd fix this in SingleTabActivity, so that web apps can benefit from this as well.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:60
(Diff revision 1)
>  import static org.mozilla.gecko.Tabs.TabEvents;
>  
>  public class CustomTabsActivity extends SingleTabActivity implements Tabs.OnTabsChangedListener {
>  
>      private static final String LOGTAG = "CustomTabsActivity";
> +    private static final long SHOW_CONTENT_DELAY = 300;

Now that we're waiting until we've actually opened/selected a tab (at least on the Java side, that is), maybe a shorter delay would do as well?

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java:101
(Diff revision 1)
>      protected void onTabOpenFromIntent(Tab tab) {
>          super.onTabOpenFromIntent(tab);
>  
> +        // Bug 1369681 - a workaround to prevent flash in first launch.
> +        // These lines will be removed once we have GeckoView based implementation.
> +        if (contentView.getVisibility() == View.INVISIBLE) {

You must split this out into a separate function and call this both from here and from `onTabSelectFromIntent`.

Think about the case of running with "Don't keep activities". This means that if the user backgrounds us, the activity could be destroyed, but Gecko might keep running with the corresponding tab powering the custom tab activity remaining open. In that case, if the user returns to the custom tab actvitiy, we go through onCreate and find that our tab is still open and select it again [1], in which case you'll receive `onTabSelectFromIntent`, *not* `onTabOpenFromIntent`.

[1] On the one hand it'll go away soon anyway, but if you've got a few minutes to spare, just trace the code flow through `SingleTabActivity` to see this for yourself.

Comment 7

[Julian Chu [:walkingice]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145558/diff/1-2/

Comment 8

[Julian Chu [:walkingice]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

https://reviewboard.mozilla.org/r/145558/#review149484

> Now that we're waiting until we've actually opened/selected a tab (at least on the Java side, that is), maybe a shorter delay would do as well?

If this delay not long enough, we might still see the flash in beginning (on some old device). Yeah I think this value is pretty to estimate :-/

Comment 9

[Julian Chu [:walkingice]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145558/diff/2-3/

Comment 10

[Julian Chu [:walkingice]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145558/diff/3-4/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

https://reviewboard.mozilla.org/r/145558/#review150070

See comments below (especially regarding overdraw) - I'll r+ anyways here: JanH can take this on and once he approves I'm okay with landing this. :)

::: mobile/android/base/java/org/mozilla/gecko/SingleTabActivity.java:179
(Diff revision 4)
> +            final Handler handler = new Handler(getMainLooper());
> +            handler.postDelayed(new Runnable() {

Use ThreadUtils for this. If there's no method for "delayed" then let's add one.

::: mobile/android/base/resources/layout/customtabs_activity.xml:33
(Diff revision 4)
>      <view class="org.mozilla.gecko.GeckoApp$MainLayout"
>          android:id="@+id/main_layout"
>          android:layout_width="match_parent"
>          android:layout_below="@id/actionbar"
>          android:layout_height="match_parent"
> -        android:background="@android:color/transparent">
> +        android:background="@android:color/white">

Can you check how this behaves when enabling the developer setting for seeing overdraw? I guess this could have a negative impact once we actually draw the web content on top of white all the time? Maybe we could set the background dynamically like we do with the visibility now?

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

https://reviewboard.mozilla.org/r/145558/#review150070

> Use ThreadUtils for this. If there's no method for "delayed" then let's add one.

We have - `ThreadUtils.postDelayedToUiThread()`.

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

https://reviewboard.mozilla.org/r/145558/#review150358

Looks fine now.

Comment 14

[Julian Chu [:walkingice]]

Comment on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145558/diff/4-5/

Comment 15

[Julian Chu [:walkingice]]

Thanks for feedback. Added modifications:
1. Use ThreadUtils
2. change background color along with view visibility

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9b5c00e6352
CustomTab hide content to prevent flash r=JanH,sebastian

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b9b5c00e6352