Closed
Bug 1369681
Opened 7 years ago
Closed 7 years ago
Custom tabs: seeing flash when launch Custom Tabs
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: walkingice, Assigned: walkingice)
References
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
> 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)
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → walkingice0204
Comment 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-review-reply |
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•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Thanks for feedback. Added modifications:
1. Use ThreadUtils
2. change background color along with view visibility
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•