Closed Bug 1369681 Opened 2 years ago Closed 2 years ago

Custom tabs: seeing flash when launch Custom Tabs

Categories

(Firefox for Android :: General, defect)

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: walkingice, Assigned: walkingice)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

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.
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?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(jh+bugzilla)
> 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).
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(jh+bugzilla)
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 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 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 :-/
Assignee: nobody → walkingice0204
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?
Attachment #8874098 - Flags: review?(s.kaspari) → review+
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 on attachment 8874098 [details]
Bug 1369681 - CustomTab hide content to prevent flash

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

Looks fine now.
Attachment #8874098 - Flags: review?(jh+bugzilla) → review+
Thanks for feedback. Added modifications:
1. Use ThreadUtils
2. change background color along with view visibility
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9b5c00e6352
CustomTab hide content to prevent flash r=JanH,sebastian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b9b5c00e6352
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.