Closed
Bug 1063750
Opened 10 years ago
Closed 10 years ago
Message to users that the Remote Tabs tray is now a home panel
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox35+ verified)
VERIFIED
FIXED
Firefox 35
People
(Reporter: nalexander, Assigned: nalexander)
References
Details
Attachments
(5 files, 1 obsolete file)
173.09 KB,
image/png
|
Details | |
44.46 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
4.69 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
5.61 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
4.18 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Now that the Remote Tabs home panel (Bug 1014994) has landed on Nightly, we want to remove the existing Remote Tabs tray. However, we have a UI affordance that we should use to smoothly message to users where they can find their synced tabs. We might even want to explain why it's better (device icons, favicons, context menu, more items shown in space, panels can be re-arranged/configured, ...).
UX, can you suggest a visual design that we could show in this panel that replaces the existing Sync calls to action or list of Remote Tabs, and that points the user to the new home panel?
Assignee | ||
Comment 1•10 years ago
|
||
antlam: I reckon this is on you, at least to start?
mcomella: you might care how this is implemented.
margaret: this is the ticket that tracks the first part of the answer to one of your questions in a different bug.
Flags: needinfo?(alam)
Assignee | ||
Comment 2•10 years ago
|
||
[Tracking Requested - why for this release]: this should hit release with the new Remote Tabs panel.
Comment 3•10 years ago
|
||
Alright I'll whip one up!
Assignee | ||
Comment 4•10 years ago
|
||
[Tracking Requested - why for this release]: as above: this should hit release with the new Remote Tabs panel.
tracking-firefox35:
--- → ?
Updated•10 years ago
|
Comment 5•10 years ago
|
||
Something like this?
I've taken the same styling from the empty state that me and Mcomella worked on as a part of bug 1021356 earlier in the year.
Flags: needinfo?(alam) → needinfo?(nalexander)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8491670 [details]
prev_mob_traypromo1.png
Sure. I personally refer to it as a "Home Panel" rather than just a "Panel". NI-ing margaret to see if she's okay with the copy around "Panel".
Should we try to sell the new panel? "Your new panel makes it easy to see the tabs you want and to hide old devices."
margaret: do you think we should do more? Visual of the panel? Link to configure your panels? Learn more SUMO link?
Flags: needinfo?(nalexander) → needinfo?(margaret.leibovic)
Comment 7•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #6)
> Comment on attachment 8491670 [details]
> prev_mob_traypromo1.png
>
> Sure. I personally refer to it as a "Home Panel" rather than just a
> "Panel". NI-ing margaret to see if she's okay with the copy around "Panel".
I think "Home Panel" is clearer than "Panel". Or maybe something like "panel on your home page"? It feels a bit weird to me to have "Panel" in quotes, so maybe we should just s/"Panel"/panel on your home page/. Although we'd also need to update the rest of the sentence to make sure it doesn't feel too long-winded.
> Should we try to sell the new panel? "Your new panel makes it easy to see
> the tabs you want and to hide old devices."
>
> margaret: do you think we should do more? Visual of the panel? Link to
> configure your panels? Learn more SUMO link?
Link to configure panels sounds like a good idea. The way our settings logic works, we should be able to open a specific settings page.
Is the plan to eventually remove this tabs tray panel altogether? Keep this informational panel here for one release, then remove it in the next?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to :Margaret Leibovic from comment #7)
> Is the plan to eventually remove this tabs tray panel altogether? Keep this
> informational panel here for one release, then remove it in the next?
Yes, that's the plan.
Assignee | ||
Comment 9•10 years ago
|
||
margaret, lucasr: any thoughts on how to implement the desired link that opens the home pager at the Remote Tabs home panel? The test code uses Robotium to swipe/click things; in BrowserApp, there are comments suggesting this is hard-coded in enterEditingMode(); I know of no other place we do this. Guidance needed.
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 10•10 years ago
|
||
mcomella: seems like you're the best person for this. Notes:
1) the link doesn't do anything on click yet. I have ni to lucasr for
how to open a particular panel.
2) This will live exactly one release, so I elected to just use the
private tab styles rather than duplicate and have to remove styles
later.
3) I'll push a try build sometime before landing, just in case I've
mangled a Robocop test.
Attachment #8497033 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 11•10 years ago
|
||
antlam:
Phone portrait screenshot: https://www.dropbox.com/s/3pnkk8zfantwd0l/Moved.Tabs.02.Landscape.png?dl=0
Phone landscape screenshot: https://www.dropbox.com/s/05cmyfkb7bce6gr/Moved.Tabs.01.Portrait.png?dl=0
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #11)
> antlam:
>
> Phone portrait screenshot:
> https://www.dropbox.com/s/3pnkk8zfantwd0l/Moved.Tabs.02.Landscape.png?dl=0
> Phone landscape screenshot:
> https://www.dropbox.com/s/05cmyfkb7bce6gr/Moved.Tabs.01.Portrait.png?dl=0
Bugzilla busts embedding images like this, so you'll have to copy-paste these links. Whee!
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Comment on attachment 8497033 [details] [diff] [review]
Part 1: Replace Remote Tabs tray with messaging to Remote Tabs home panel. r=mcomella
Review of attachment 8497033 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/resources/layout/remote_tabs_panel.xml
@@ +5,5 @@
> + - file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +-->
> +
> +<merge xmlns:android="http://schemas.android.com/apk/res/android"
> + xmlns:gecko="http://schemas.android.com/apk/res-auto" >
nit: align the attributes
::: mobile/android/base/tabs/RemoteTabsPanel.java
@@ +17,1 @@
> class RemoteTabsPanel extends FrameLayout implements PanelView {
nit: Like you've told me oh-so-many times before, class comment. :D
@@ +51,5 @@
>
> @Override
> public boolean shouldExpand() {
> + final LinearLayout container = (LinearLayout) findViewById(R.id.container);
> + return container == null || container.getOrientation() == LinearLayout.VERTICAL;
Why would container ever be null here?
Attachment #8497033 -
Flags: review?(michael.l.comella) → feedback+
(In reply to Nick Alexander :nalexander from comment #10)
> 1) the link doesn't do anything on click yet. I have ni to lucasr for
> how to open a particular panel.
Not on my dev machine to follow the code path completely but it seems like this might help: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePager.java#407
Comment 15•10 years ago
|
||
(In reply to Nick Alexander :nalexander from comment #9)
> margaret, lucasr: any thoughts on how to implement the desired link that
> opens the home pager at the Remote Tabs home panel? The test code uses
> Robotium to swipe/click things; in BrowserApp, there are comments suggesting
> this is hard-coded in enterEditingMode(); I know of no other place we do
> this. Guidance needed.
You can do this by opening a new tab pointing to about:home with a specific panel ID. Something like:
Tabs.getInstance().loadUrl("about:home?panel=" + HomeConfig.REMOTE_TABS_PANEL_ID, LOADURL_NEW_TAB);
Flags: needinfo?(lucasr.at.mozilla)
Assignee | ||
Comment 16•10 years ago
|
||
mcomella: I fixed the three things you asked for (class comment,
indentation, remove null check), but why did this need re-review?
Attachment #8497033 -
Attachment is obsolete: true
Attachment #8497868 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 17•10 years ago
|
||
Deriving the correct panel UUID from the active Home Panel configuration
is challenging. This is easy and will only exist for one release. (The
Remote Tabs tray icon will disappear after this release; see Bug
1063753.) This solves the problem in essentially all the situations we
expect to see.
Attachment #8497869 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 18•10 years ago
|
||
Duplicate test types make it difficult to keep tests and code in
lock-step. (They do mean fewer Proguard annotations.)
Attachment #8497870 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 19•10 years ago
|
||
There are no consumers of this (that I can see) in the tree, so we need
to test that it doesn't go away.
Attachment #8497871 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 20•10 years ago
|
||
Assignee | ||
Comment 21•10 years ago
|
||
mcomella: review ping. This is the last thing for Remote Tabs home panel 35 landing. If you don't like the testing changes, let's split another ticket off to mop them up.
Comment on attachment 8497868 [details] [diff] [review]
Part 1: Replace Remote Tabs tray with messaging to Remote Tabs home panel. r=mcomella
Review of attachment 8497868 [details] [diff] [review]:
-----------------------------------------------------------------
(In reply to Nick Alexander :nalexander from comment #16)
> mcomella: I fixed the three things you asked for (class comment,
> indentation, remove null check), but why did this need re-review?
If something is a question, I like to have it answered before I r+. For example, if you needed the null check for the container, I'd like to double check the code and its assumptions before r+'ing.
Attachment #8497868 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8497869 [details] [diff] [review]
Part 2: Implement link from Remote Tabs tray to Remote Tabs home panel. r=mcomella
Review of attachment 8497869 [details] [diff] [review]:
-----------------------------------------------------------------
By the way, sorry for the delay before reviewing this.
::: mobile/android/base/home/HomeConfig.java
@@ +1495,5 @@
> public void setOnReloadListener(OnReloadListener listener);
> }
>
> + // UUIDs used to create PanelConfigs for default built-in panels. These are
> + // public because they can be used in "about:home?panel=UUID" query strings
If you want to maintain the access level to these Strings and make the use case more obvious, you can create a method such as, `HomeConfig.getPanelURL(PanelType)` which would return,
`AboutPages.HOME + "?panel=" + <panel_id>`
::: mobile/android/base/tabs/RemoteTabsPanel.java
@@ +40,5 @@
> + // catastrophic.) Querying the current configuration to
> + // determine if the panel is present is not worth the effort; we
> + // expect very few configurations to not include the Remote Tabs
> + // panel.
> + Tabs.getInstance().loadUrl(AboutPages.HOME + "?panel=" + HomeConfig.REMOTE_TABS_PANEL_ID,
I think I'd prefer, `Tabs.getInstance().loadHomePanel(HomeConfig.PanelType, int);`
Attachment #8497869 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8497870 [details] [diff] [review]
Part 3: Remove duplicated test-only PanelType. r=mcomella
Review of attachment 8497870 [details] [diff] [review]:
-----------------------------------------------------------------
Nice cleanup.
Attachment #8497870 -
Flags: review?(michael.l.comella) → review+
Comment on attachment 8497871 [details] [diff] [review]
Part 4: Test navigating to about:home?panel= URLs. r=mcomella
Review of attachment 8497871 [details] [diff] [review]:
-----------------------------------------------------------------
Just a little cleanup.
::: mobile/android/base/tests/components/AboutHomeComponent.java
@@ +203,5 @@
> + * of the panel to navigate to.
> + * @return self, for chaining.
> + */
> + public AboutHomeComponent navigateToPanelById(String panelId) {
> + Tabs.getInstance().loadUrl(AboutPages.HOME + "?panel=" + panelId);
In the spirit of UI testing, this should not be accessing internals to open the url - we should type the url into the toolbar.
Thus, to combine both this should probably be a method of NavigationHelper, and for ease of use, allow the caller to pass in a PanelType. Then we can call `NavigationHelper.enterAndLoadUrl(String)` with the correct panel url.
::: mobile/android/base/tests/testAboutHomeVisibility.java
@@ +51,5 @@
> + .assertCurrentPanel(PanelType.HISTORY);
> +
> + // Navigating to a non-existent panel opens the default panel. When
> + // testing, we assume said default panel is TOP_SITES.
> + mAboutHome.navigateToPanelById("garbage-panel-uuid")
Given my other comments here, you can call enterAndLoadURL with a garbage UUID here.
Attachment #8497871 -
Flags: review?(michael.l.comella) → review-
Comment on attachment 8497871 [details] [diff] [review]
Part 4: Test navigating to about:home?panel= URLs. r=mcomella
Review of attachment 8497871 [details] [diff] [review]:
-----------------------------------------------------------------
I should read the comments sooner.
(In reply to Nick Alexander :nalexander from comment #21)
> mcomella: review ping. This is the last thing for Remote Tabs home panel 35
> landing. If you don't like the testing changes, let's split another ticket
> off to mop them up.
Sure, I'm down for that. Feel free to clean up part 2 there as well, if you agree with my suggestions.
Attachment #8497871 -
Flags: review- → review+
Assignee | ||
Comment 27•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #23)
> Comment on attachment 8497869 [details] [diff] [review]
> Part 2: Implement link from Remote Tabs tray to Remote Tabs home panel.
> r=mcomella
>
> Review of attachment 8497869 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> By the way, sorry for the delay before reviewing this.
>
> ::: mobile/android/base/home/HomeConfig.java
> @@ +1495,5 @@
> > public void setOnReloadListener(OnReloadListener listener);
> > }
> >
> > + // UUIDs used to create PanelConfigs for default built-in panels. These are
> > + // public because they can be used in "about:home?panel=UUID" query strings
>
> If you want to maintain the access level to these Strings and make the use
> case more obvious, you can create a method such as,
> `HomeConfig.getPanelURL(PanelType)` which would return,
>
> `AboutPages.HOME + "?panel=" + <panel_id>`
>
> ::: mobile/android/base/tabs/RemoteTabsPanel.java
> @@ +40,5 @@
> > + // catastrophic.) Querying the current configuration to
> > + // determine if the panel is present is not worth the effort; we
> > + // expect very few configurations to not include the Remote Tabs
> > + // panel.
> > + Tabs.getInstance().loadUrl(AboutPages.HOME + "?panel=" + HomeConfig.REMOTE_TABS_PANEL_ID,
>
> I think I'd prefer, `Tabs.getInstance().loadHomePanel(HomeConfig.PanelType,
> int);`
I totally agree on both your counts, but it's actually quite hard to do! That's what the comments are about. There is no static map from PanelTypes to UUIDs. The UUIDs are only used to identify the panels when we build the default configuration. That's what the commit comment was intended to convey: it's actually hard to get a partial-map from the current configuration's PanelTypes to the relevant UUIDs. Rather than shave that yak, I exposed these internals.
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #23)
> Comment on attachment 8497869 [details] [diff] [review]
> Part 2: Implement link from Remote Tabs tray to Remote Tabs home panel.
> r=mcomella
>
> Review of attachment 8497869 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> By the way, sorry for the delay before reviewing this.
>
> ::: mobile/android/base/home/HomeConfig.java
> @@ +1495,5 @@
> > public void setOnReloadListener(OnReloadListener listener);
> > }
> >
> > + // UUIDs used to create PanelConfigs for default built-in panels. These are
> > + // public because they can be used in "about:home?panel=UUID" query strings
>
> If you want to maintain the access level to these Strings and make the use
> case more obvious, you can create a method such as,
> `HomeConfig.getPanelURL(PanelType)` which would return,
>
> `AboutPages.HOME + "?panel=" + <panel_id>`
I did this one, refactoring (trivially) some code in HomeConfig and adding the helper in AboutPages (since that's where you're most likely to look for this thing, once you know that getting to a panel is done with an about:home URL).
Thanks for this suggestion, it encapsulated some things that already existed and improved the expression of this thought.
> ::: mobile/android/base/tabs/RemoteTabsPanel.java
> @@ +40,5 @@
> > + // catastrophic.) Querying the current configuration to
> > + // determine if the panel is present is not worth the effort; we
> > + // expect very few configurations to not include the Remote Tabs
> > + // panel.
> > + Tabs.getInstance().loadUrl(AboutPages.HOME + "?panel=" + HomeConfig.REMOTE_TABS_PANEL_ID,
>
> I think I'd prefer, `Tabs.getInstance().loadHomePanel(HomeConfig.PanelType,
> int);`
I decided not to add a home panel thing to the Tabs interface, which is already bulky.
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #25)
> Comment on attachment 8497871 [details] [diff] [review]
> Part 4: Test navigating to about:home?panel= URLs. r=mcomella
>
> Review of attachment 8497871 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Just a little cleanup.
>
> ::: mobile/android/base/tests/components/AboutHomeComponent.java
> @@ +203,5 @@
> > + * of the panel to navigate to.
> > + * @return self, for chaining.
> > + */
> > + public AboutHomeComponent navigateToPanelById(String panelId) {
> > + Tabs.getInstance().loadUrl(AboutPages.HOME + "?panel=" + panelId);
>
> In the spirit of UI testing, this should not be accessing internals to open
> the url - we should type the url into the toolbar.
>
> Thus, to combine both this should probably be a method of NavigationHelper,
> and for ease of use, allow the caller to pass in a PanelType. Then we can
> call `NavigationHelper.enterAndLoadUrl(String)` with the correct panel url.
I kept things in AboutHomeComponent for no strong reason. In general, I'm against "typing into the URL bar" for /all/ of our tests, 'cuz it's error prone.
> ::: mobile/android/base/tests/testAboutHomeVisibility.java
> @@ +51,5 @@
> > + .assertCurrentPanel(PanelType.HISTORY);
> > +
> > + // Navigating to a non-existent panel opens the default panel. When
> > + // testing, we assume said default panel is TOP_SITES.
> > + mAboutHome.navigateToPanelById("garbage-panel-uuid")
>
> Given my other comments here, you can call enterAndLoadURL with a garbage
> UUID here.
With the helper from part 2, it ended up being odd to navigate to a panel (type) that doesn't exist. (The helper throws an IllegalArgumentException if you don't give a known built-in panel.) So I just dropped this part of the test.
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/439792036cc7
https://hg.mozilla.org/mozilla-central/rev/918c616ddc23
https://hg.mozilla.org/mozilla-central/rev/8b8ce071a7e5
https://hg.mozilla.org/mozilla-central/rev/ba324f45f0b5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox35:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment 32•10 years ago
|
||
Verified as fixed on
Build: Firefox for Android 35.0a2 (2014-10-15)
Devices:
Nexus 4 (Android 4.4.4)
Asus Transformer Pad TF300T (Android 4.2.1)
Status: RESOLVED → VERIFIED
Comment 33•10 years ago
|
||
> we want to remove the existing Remote Tabs tray.
Pleeease don't. I understand the rational for the new Synced Tabs site. But the removal of the old Remote Tab tray can make things quite cumbersome: If you keep a lot of tabs open you most likely see a website when starting Firefox and not the New Tab page. So you would have to:
1. Click on the Tab icon next to the URL bar.
2. Click on the "plus".
3. Swipe to the left *trice* (or change the default page).
Compared to the straight forward and inuitive way of -> 1. Tab-icon -> 2. Remote Tabs tray.
If the Tab tray remains in place why not have both ways available to reach the synced tabs?
Assignee | ||
Comment 34•10 years ago
|
||
(In reply to Per from comment #33)
> > we want to remove the existing Remote Tabs tray.
>
> Pleeease don't. I understand the rational for the new Synced Tabs site. But
> the removal of the old Remote Tab tray can make things quite cumbersome: If
> you keep a lot of tabs open you most likely see a website when starting
> Firefox and not the New Tab page. So you would have to:
Sorry, but the decision has been made and the work is done. It will be in Firefox 35.
> 1. Click on the Tab icon next to the URL bar.
> 2. Click on the "plus".
> 3. Swipe to the left *trice* (or change the default page).
Or click the URL bar, then start swiping. Or tapping in the titles, which I grant is only helpful if your screen is large enough to display the Synced Tabs title. But you are aware that you can re-order the panels and/or change the default panel.
> Compared to the straight forward and inuitive way of -> 1. Tab-icon -> 2.
> Remote Tabs tray.
>
> If the Tab tray remains in place why not have both ways available to reach
> the synced tabs?
Two reasons, I suppose:
1) Synced Tabs never made sense in the Tabs Tray -- it doesn't look or feel like either of the other two sets of tabs we show there.
2) We wanted some features that were technically very challenging to do with the existing code. In particular, we wanted to allow long-tap to show menus and let the user modify the device list (hide/show devices). The way to do that is to make Synced Tabs a home panel.
Sorry that we're breaking your flow, but I think you can actually get a better one (if you so care): tap URL bar, get Synced Tabs panel as your default :)
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
•