997288 - "Open in [New|Private] Tab" should have a quick switch-to-tab feature

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

Use case: There are a couple news sites that I visit that limit the number of articles you can read per month. A simple workaround for these sites is to open the article in a private tab, as that tab doesn't have the cookie present that tells the website that you are a frequent visitor. In this case, I open the link in a new private tab, but I want to view that tab's content immediately.

It would be nice if the notification bubble that appears, saying "New private tab opened", was clickable such that it would take me directly to the private tab.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Comparing to desktop, if you right-click on a link on desktop Firefox and choose "Open in Private Window", the new private window is foregrounded and visible right away.

So maybe on Android "Open in Private Tab" should just switch to the new tab immediately?

Comment 2

[Nick Alexander :nalexander [he/him]]

I want this for "Open in new tab" as well!  So glad somebody else filed this.

I'm happy to mentor this bug.  It'll be a little involved:

The two places that spawn the toast are:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#487
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#499

There's support for adding a button to a toast already; I'm not sure what it looks like, but it's there.  I see a single example in the code base already:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/ContentDispatchChooser.js#56

If you can get the button displaying, then we can figure out how to switch the browser to that newly created tab.  Which looks pretty easy, actually: keep a reference to the newly created in addTab, and then, in the button callback, use selectedTab = tab.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Nick Alexander :nalexander from comment #2)

> The two places that spawn the toast are:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#487
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#499

These two place are entry points for the Web Content context menu which is displayed when you long tap on a link in a web page. There are other entry points for the Home panel context menu. It's native Java and the toast is created here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#177

I don't think we have an API for Super Toast from Java, but I'm sure we could make one. Let's not block on the Java part though.

> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> ContentDispatchChooser.js#56

Yep. This is a Super Toast, which has a tapped action on the right end of the toast. This is what we want to use for this feature.

> If you can get the button displaying, then we can figure out how to switch
> the browser to that newly created tab.  Which looks pretty easy, actually:
> keep a reference to the newly created in addTab, and then, in the button
> callback, use selectedTab = tab.

Yep. This is exactly what you need to do.

Comment 5

[Wesley Johnston (:wesj)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Nick Alexander :nalexander from comment #2)
> 
> > The two places that spawn the toast are:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > browser.js#487
> > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> > browser.js#499
> 
> These two place are entry points for the Web Content context menu which is
> displayed when you long tap on a link in a web page. There are other entry
> points for the Home panel context menu. It's native Java and the toast is
> created here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/
> HomeFragment.java#177
> 
> I don't think we have an API for Super Toast from Java, but I'm sure we
> could make one. Let's not block on the Java part though.

We do! See http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#2366

Also, for some reason I called it "ButtonToast" in Java. Separate bug, but maybe we should just rename it SuperToast some day.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #5)

> Also, for some reason I called it "ButtonToast" in Java. Separate bug, but
> maybe we should just rename it SuperToast some day.

ButtonToast sounds like a solid, proper name. Named for exactly what it does.

Comment 7

[Nick Alexander :nalexander [he/him]]

Attachment: Part 1: Add go-to-tab button to "New [private] tab opened" toast from web context menu. r=wesj

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Part 2: Add go-to-tab button to "New [private] tab opened" toast from about:home. r=wesj

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: new.tab.opened.png

ibarlow: I used an existing "forward" icon, and didn't add button text.  wesj sniffed at the short divider bar, but this may be a platform thing.  I'll grab a screenshot from a KitKat device.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: new.private.tab.opened.png

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: new.tab.opened.kitkat.png

... and it looks terrible on Kitkat, so we'll need a new icon.  ibarlow, can you add this to your queue?

Comment 12

[Nick Alexander :nalexander [he/him]]

Attachment: new.tab.opened.kitkat.png

Comment 13

[Ian Barlow (:ibarlow)]

Handing this over to Anthony for some visuals. 

Anthony, Nick is working on a special toast notification that a user sees when they open a new private tab in the background (I'm assuming this is being built for normal tabs too?). Basically a user sees a notification that the tab has been opened, and they have an icon they can tap to switch to that tab right away, if they want.

The toast is structured like

--------------------------------------
New tab opened                |   ->
--------------------------------------

or

--------------------------------------
New private tab opened        |   ->
--------------------------------------



Can you please work on an icon for switching to the new tab? Thanks!

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 8422710 [details] [diff] [review]
Part 1: Add go-to-tab button to "New [private] tab opened" toast from web context menu. r=wesj

Review of attachment 8422710 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks fine, but we need the icons.

I'm worried you wind up rat-holing to make these buttons look better, but I think they could probably in general use some clean up. We've iterated a few times on adding icons/text in different configurations and never managed to land anything, so I think all the current button toasts just use text. i.e. Ping me if you need me to take any bugs.

::: mobile/android/chrome/content/browser.js
@@ +493,5 @@
>  
>          let newtabStrings = Strings.browser.GetStringFromName("newtabpopup.opened");
>          let label = PluralForm.get(1, newtabStrings).replace("#1", 1);
> +        NativeWindow.toast.show(label, "long", {
> +          button: { label: "",

Wrap the label: property to its own line. Same below.

@@ +512,5 @@
> +        NativeWindow.toast.show(label, "long", {
> +            button: { label: "",
> +                      icon: "drawable://ic_menu_forward",
> +                      callback: () => { BrowserApp.selectTab(tab); },
> +            } });

And wrap the second set of brackets onto their own line as well.

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8422711 [details] [diff] [review]
Part 2: Add go-to-tab button to "New [private] tab opened" toast from about:home. r=wesj

Review of attachment 8422711 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. One nit.

::: mobile/android/base/widget/ButtonToast.java
@@ +113,5 @@
>          mCurrentToast = t;
>          mButton.setEnabled(true);
>  
> +        if (null != t.message) {
> +            mMessageView.setText(t.message);

I think you have to be careful here. We reuse this view for different toasts, and I'm not sure we reset its state every time its hidden. i.e. You may need to set this to "" if its not set.

Comment 16

[Nick Alexander :nalexander [he/him]]

antlam: you were supposed to be NI on https://bugzilla.mozilla.org/show_bug.cgi?id=997288#c13.

Comment 17

[Nick Alexander :nalexander [he/him]]

Attachment: Add select tab button to "New [private] tab opened" toasts. r=wesj

I folded these two together for easier re-review.

Great catch on the toast re-use.  This uncovered other issues that
were hidden by NativeJSObject (see my m-f-d post about optString and
null) so I reworked the JS side that passes the button parameters
across to Java.

I added a new resource with the old PNGs; we'll drop in new PNGs when
:antlam provides.

Comment 18

[Wesley Johnston (:wesj)]

Comment on attachment 8427424 [details] [diff] [review]
Add select tab button to "New [private] tab opened" toasts. r=wesj

Review of attachment 8427424 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/HomeFragment.java
@@ +183,5 @@
> +            final int newTabId = newTab.getId(); // We don't want to hold a reference to the Tab.
> +
> +            final String message = isPrivate ?
> +                    getResources().getString(R.string.new_private_tab_opened) :
> +                    getResources().getString(R.string.new_tab_opened);

Someone just beat you to this fix in a different bug :)

::: mobile/android/base/widget/ButtonToast.java
@@ +118,5 @@
> +            mMessageView.setText(t.message);
> +        } else {
> +            mMessageView.setText("");
> +        }
> +        if (null != t.buttonMessage) {

Add empty lines between the different sections here. I'd almost prefer a ternary:

mMessageView.setText((t.message != null) ? t.message : "");

::: mobile/android/chrome/content/browser.js
@@ +1771,5 @@
> +        };
> +        // null is badly handled by the receiver, so try to avoid including nulls.
> +        if (aOptions.button.label) {
> +          msg.button.label = aOptions.button.label;
> +        }

Ahh, this is what started the optString emails :) Thanks for fixing :) Also, add a blank line between the different sections here.

Comment 19

[Nick Alexander :nalexander [he/him]]

Landed with the original (bad on KitKat) assets; we'll land an updated asset in follow-up.  Flagged as keep-open for the sheriffs.

https://hg.mozilla.org/integration/fx-team/rev/c87fe78a40b8

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screen Shot 2014-05-23 at 1.55.42 PM.png

Posting a WIP..

Right now I'm just deciding between the two icons. I'm leaning towards the second one because it scales better (looks less like some random shapes).

I do have some questions for the future though. For one, how much control do we have over the look of this toast? I've used Roboto and the same vertical divider there but can we swap these out? or even add padding? Also, are animated icons here a possibility?

Side note: we should implement a transition when they hit this button and "switch tabs".

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #20)
> Created attachment 8427975 [details]
> Screen Shot 2014-05-23 at 1.55.42 PM.png
> 
> Posting a WIP..
> 
> Right now I'm just deciding between the two icons. I'm leaning towards the
> second one because it scales better (looks less like some random shapes).

I notice you are changing the text from "New tab opened" to "Switch to tab". This is a departure from the pattern used by button toast, where the text is acting as confirmation of the previous action and the button allows an optional continuation of a new action.

Looking at other apps (usually Google ones) that use toasts like this, we'd see:

"New tab opened | Switch >"  or  "New tab opened | > Switch"

where ">" is a small icon.

> I do have some questions for the future though. For one, how much control do
> we have over the look of this toast? I've used Roboto and the same vertical
> divider there but can we swap these out? or even add padding? Also, are
> animated icons here a possibility?

We try to make the toast look like the native toast, but tweaks are possible. Animated icons are possible, but not free and might cause some jank in the transition that displays the toast.

> Side note: we should implement a transition when they hit this button and
> "switch tabs".

Sounds a little out of scope for this bug. There have been plans to add transitions when switching between tabs, just lower priority. Those transitions would need to play nice with the toast transitions too.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: gmail-super-toast-crop.png

Example toast from GMail

Comment 24

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks for all the feedback Mark.

> I notice you are changing the text from "New tab opened" to "Switch to tab".
> This is a departure from the pattern used by button toast, where the text is
> acting as confirmation of the previous action and the button allows an
> optional continuation of a new action.

That's a good point. My intention was to present some form of the word "switch" in the copy such that the user would gain insight into what hitting this toast would do. Also, the typical workflow would prompt a user several times over with some sort of "Open in a new private/tab" and I thought that would be enough. Now that I see the example toast again I think that's a better place to put it.

> We try to make the toast look like the native toast, but tweaks are
> possible. Animated icons are possible, but not free and might cause some
> jank in the transition that displays the toast.

I thought so - animated icons should wait.

> Sounds a little out of scope for this bug. There have been plans to add
> transitions when switching between tabs, just lower priority. Those
> transitions would need to play nice with the toast transitions too.

Can you point me in the direction of the bug? I was just about to create one.

Comment 25

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: Screen Shot 2014-05-26 at 12.24.13 PM.png

Attaching revisions.

Comment 26

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Question: have we thought about the interaction/transition that will follow after a user taps on this button?

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #24)

> > Sounds a little out of scope for this bug. There have been plans to add
> > transitions when switching between tabs, just lower priority. Those
> > transitions would need to play nice with the toast transitions too.
> 
> Can you point me in the direction of the bug? I was just about to create one.

You should create one. I don't think we ever filed a bug.

(In reply to Anthony Lam (:antlam) from comment #26)
> Question: have we thought about the interaction/transition that will follow
> after a user taps on this button?

Not if the user taps on the button, since that would be the same as a tab switch (I think). But we did do some ad-hoc thoughts about showing a tab open in the background (i.e. the opposite situation of pressing the button). We talked about showing some icon/image moving from the center of the screen to the top of the screen. This was based on the way Firefox Desktop does file download animations, to let people know about the downloads button.

Comment 28

[Lucas Rocha (:lucasr)]

Attachment: Screenshot_2014-05-26-10-16-33.png

Just saw this feature in Nightly. Maybe it's just me but I think the layout could be improved a bit. IMO, the toast notification is looking a bit strange for few of reasons:
- Icon is a bit too large compared to the text.
- The left and right paddings are too different.
- The separator looks strangely small.
- The icon '3d' style is looking a bit alien there. Haven't we moved on to a style with solid colors?
 
So, here's what I suggest:
- Reduce the size of the icon a bit.
- Make sure the paddings are visually consistent somehow.
- Make the separator as tall as the icon (or just a bit taller).

Comment 29

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8428869 [details]
Screen Shot 2014-05-26 at 12.24.13 PM.png

How does this feel to you, lucasr?  I like it.

Comment 30

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #26)
> Question: have we thought about the interaction/transition that will follow
> after a user taps on this button?

I have not thought about it at all.  Right now, it swaps to the tab with no transition at all.

Comment 31

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #29)
> Comment on attachment 8428869 [details]
> Screen Shot 2014-05-26 at 12.24.13 PM.png
> 
> How does this feel to you, lucasr?  I like it.

This is looking a lot better, thanks!

Comment 32

[Ian Barlow (:ibarlow)]

(In reply to Nick Alexander :nalexander from comment #30)
> (In reply to Anthony Lam (:antlam) from comment #26)
> > Question: have we thought about the interaction/transition that will follow
> > after a user taps on this button?
> 
> I have not thought about it at all.  Right now, it swaps to the tab with no
> transition at all.

This is nice. Little drive by nit -- no periods, please :)

Comment 33

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Mark Finkle (:mfinkle) from comment #27)
> (In reply to Anthony Lam (:antlam) from comment #24)
> 
> > > Sounds a little out of scope for this bug. There have been plans to add
> > > transitions when switching between tabs, just lower priority. Those
> > > transitions would need to play nice with the toast transitions too.
> > 
> > Can you point me in the direction of the bug? I was just about to create one.
> 
> You should create one. I don't think we ever filed a bug.

Bug 1016133 - Improve interactions for "Open Link in New/Private Tab" and "Switch-to-tab" feature :)

Comment 34

[:Margaret Leibovic]

What's remaining in this bug that's keeping it open? I find it easier to follow follow-up work and discussion when it happens in separate bugs :)

Comment 35

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #34)
> What's remaining in this bug that's keeping it open? I find it easier to
> follow follow-up work and discussion when it happens in separate bugs :)

Landing updated visuals.  I'm working on it right now.  If you insist on a separate bug, file it, and assign to me :)

Comment 36

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #25)
> Created attachment 8428869 [details]
> Screen Shot 2014-05-26 at 12.24.13 PM.png
> 
> Attaching revisions.

antlam: can I get this icon in mdpi, hdpi, and xhdpi, please?

Comment 37

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: switchto_icon.zip

Here you go Nick. These should work.. the icon's edge to edge in this case. I haven't baked in the padding into the .PNGs.

Comment 38

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #35)
> (In reply to :Margaret Leibovic from comment #34)
> > What's remaining in this bug that's keeping it open? I find it easier to
> > follow follow-up work and discussion when it happens in separate bugs :)
> 
> Landing updated visuals.  I'm working on it right now.  If you insist on a
> separate bug, file it, and assign to me :)

Okay, that's fine with me. I'm just being a bugzilla pedant!

Comment 39

[Nick Alexander :nalexander [he/him]]

Attachment: Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr

Here's the simple part of this: new string, new drawables.  I elected
to rename the drawable to be more consistent with the string.

Comment 40

[Nick Alexander :nalexander [he/him]]

Attachment: Follow-up, Part 2: Style ButtonToast. r=lucasr

And here's the complicated part.  This is awful, but what came earlier
was *worse*.  It was almost impossible to modify the original because
the margins on pre-19, and the curves on post-19, were all built as 9
patches.  This uses corner radius and plain old margins and padding.
Unfortunately, getting those correct in the two configurations, *and
making the button press highlight look good*, requires some hijinks.

I'm not even sure this needs /review/, per se -- it's really a matter
of trying it out and making sure it feels right.  But perhaps I've
missed some technique that makes this mess of styles simpler?

Comment 41

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.19.Phone.png

Comment 42

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.19.Phone.Pressed.png

Comment 43

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.pre19.Phone.png

Comment 44

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.pre19.Phone.Pressed.png

Comment 45

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.pre19.Tablet.png

Comment 46

[Nick Alexander :nalexander [he/him]]

Attachment: Button.Toast.After.pre19.Tablet.Pressed.png

Comment 47

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8429748 [details]
Button.Toast.After.pre19.Tablet.Pressed.png

lucasr, antlam: I've attached the resulting screenshots.

Comment 48

[Lucas Rocha (:lucasr)]

Comment on attachment 8429686 [details] [diff] [review]
Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr

Review of attachment 8429686 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I'm not keen on having uppercase as a translation guideline though. The uppercase style on the button message is more of a UI design matter i.e. translators shouldn't really have to care about it. The right thing to do here is probably just setting the button message string to be all uppercase to enforce consistency. Check with ibarlow/antlam if they agree.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +65,5 @@
>  <!ENTITY new_private_tab_opened "New private tab opened">
> +<!-- Localization note (switch_button_message): This string should be as short
> +     as possible because it's shown as a label in a toast.  Ideally, this string
> +     is upper-case, to match Google and Android's convention. -->
> +<!ENTITY switch_button_message "SWITCH">

We should ensure the uppercase in code instead.

Comment 49

[Lucas Rocha (:lucasr)]

Comment on attachment 8429690 [details] [diff] [review]
Follow-up, Part 2: Style ButtonToast. r=lucasr

Review of attachment 8429690 [details] [diff] [review]:
-----------------------------------------------------------------

Looking great. Probably just needs to tweak button corner radius so that the pressed state looks nice and tidy inside the more rounded shape on post-19. Giving r+ anyway. Feel free to request another review if fixing this involves a non-trivial tweak on the resource files.

::: mobile/android/base/resources/drawable/toast_button_background.xml
@@ +4,5 @@
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> +    <item android:state_pressed="true">
> +	    <shape android:shape="rectangle">

nit: get rid of these tabs.

@@ +6,5 @@
> +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> +    <item android:state_pressed="true">
> +	    <shape android:shape="rectangle">
> +	    	<solid android:color="@color/toast_button_pressed" />
> +	    	<corners android:radius="@dimen/toast_button_corner_radius" />

Does this mean that the button will have rounded corners on left and right? How does it interact visually with the separator? Maybe we should set corner radius only on topRightRadius and bottomRightRadius instead?

Comment 50

[Lucas Rocha (:lucasr)]

(In reply to Lucas Rocha (:lucasr) from comment #48)
> Comment on attachment 8429686 [details] [diff] [review]
> Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr
> 
> Review of attachment 8429686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I'm not keen on having uppercase as a translation guideline
> though. The uppercase style on the button message is more of a UI design
> matter i.e. translators shouldn't really have to care about it. The right
> thing to do here is probably just setting the button message string to be
> all uppercase to enforce consistency. Check with ibarlow/antlam if they
> agree.

In terms of implementation, maybe you could simply replace the Button view with our own AllCapsTextView?

Comment 51

[Lucas Rocha (:lucasr)]

Comment on attachment 8429748 [details]
Button.Toast.After.pre19.Tablet.Pressed.png

Looks good. I'd prefer the tap area to cover full width between separator and left edge of the toast and full toast height. Not a big deal though. Up to you and antlam.

Comment 52

[Ian Barlow (:ibarlow)]

Comment on attachment 8422714 [details]
new.tab.opened.png

Don't mind me, just clearing some old flags :)

Comment 53

[Nick Alexander :nalexander [he/him]]

(In reply to Lucas Rocha (:lucasr) from comment #48)
> Comment on attachment 8429686 [details] [diff] [review]
> Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr
> 
> Review of attachment 8429686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I'm not keen on having uppercase as a translation guideline
> though. The uppercase style on the button message is more of a UI design
> matter i.e. translators shouldn't really have to care about it. The right
> thing to do here is probably just setting the button message string to be
> all uppercase to enforce consistency. Check with ibarlow/antlam if they
> agree.

Personally, I agree, but in #mobile yesterday, Pike told me explicitly that the right way to do this is to define the English string as upper-case.  He claimed that some locales have a different interpretation of upper-case, and even converting to upper-case programmatically can be error-prone, so that it's best to just define the string.  However, I made the note -- Pike, can you take a look:

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +65,5 @@
>  <!ENTITY new_private_tab_opened "New private tab opened">
> +<!-- Localization note (switch_button_message): This string should be as short
> +     as possible because it's shown as a label in a toast.  Ideally, this string
> +     is upper-case, to match Google and Android's convention. -->
> +<!ENTITY switch_button_message "SWITCH">

Comment 54

[Nick Alexander :nalexander [he/him]]

> @@ +6,5 @@
> > +<selector xmlns:android="http://schemas.android.com/apk/res/android">
> > +    <item android:state_pressed="true">
> > +	    <shape android:shape="rectangle">
> > +	    	<solid android:color="@color/toast_button_pressed" />
> > +	    	<corners android:radius="@dimen/toast_button_corner_radius" />
> 
> Does this mean that the button will have rounded corners on left and right?
> How does it interact visually with the separator? Maybe we should set corner
> radius only on topRightRadius and bottomRightRadius instead?

Hmm, I liked (see screenshot) how the button fits into the rounded corner.  I could certainly make the button take up the full vertical, and do the rounded corners thing (which is busted on older Androids, but can be worked around).  That might even make things simpler.  I'll investigate.  Thanks for catching my tabs, I thought I looked at all the hunks :(

Comment 55

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #53)
> (In reply to Lucas Rocha (:lucasr) from comment #48)
> > Comment on attachment 8429686 [details] [diff] [review]
> > Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr
> > 
> > Review of attachment 8429686 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good. I'm not keen on having uppercase as a translation guideline
> > though. The uppercase style on the button message is more of a UI design
> > matter i.e. translators shouldn't really have to care about it. The right
> > thing to do here is probably just setting the button message string to be
> > all uppercase to enforce consistency. Check with ibarlow/antlam if they
> > agree.
> 
> Personally, I agree, but in #mobile yesterday, Pike told me explicitly that
> the right way to do this is to define the English string as upper-case.  He
> claimed that some locales have a different interpretation of upper-case, and
> even converting to upper-case programmatically can be error-prone, so that
> it's best to just define the string.  However, I made the note -- Pike, can
> you take a look:

What I had in mind was using something like toUpperCase(Locale)

http://docs.oracle.com/javase/7/docs/api/java/lang/String.html#toUpperCase%28java.util.Locale%29

...which should, in theory, do the right thing for us. Unless I'm missing something?

Comment 56

[Lucas Rocha (:lucasr)]

Comment on attachment 8429686 [details] [diff] [review]
Follow-up, Part 1: Replace "Switch to tab" button icon and label. r=lucasr

Flipping to r+ based on l10n feedback.

Comment 57

[Nick Alexander :nalexander [he/him]]

Landing with the note as is.  Any follow-up can go in a new ticket.

https://hg.mozilla.org/integration/fx-team/rev/94c7c9f4e27e
https://hg.mozilla.org/integration/fx-team/rev/25579bde4b81

Comment 58

[Nick Alexander :nalexander [he/him]]

Sigh:

https://hg.mozilla.org/integration/fx-team/rev/62b743e5a4f9

Comment 59

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/94c7c9f4e27e
https://hg.mozilla.org/mozilla-central/rev/25579bde4b81
https://hg.mozilla.org/mozilla-central/rev/62b743e5a4f9

Comment 60

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8429748 [details]
Button.Toast.After.pre19.Tablet.Pressed.png

(In reply to Lucas Rocha (:lucasr) from comment #51)
> Comment on attachment 8429748 [details]
> Button.Toast.After.pre19.Tablet.Pressed.png
> 
> Looks good. I'd prefer the tap area to cover full width between separator
> and left edge of the toast and full toast height. Not a big deal though. Up
> to you and antlam.

I'd agree with Lucas here. Padding the visual hit area seems a bit awkward.. Sorry for the late reply this one must've slipped through.

Comment 61

[Nick Alexander :nalexander [he/him]]

(In reply to Anthony Lam (:antlam) from comment #60)
> Comment on attachment 8429748 [details]
> Button.Toast.After.pre19.Tablet.Pressed.png
> 
> (In reply to Lucas Rocha (:lucasr) from comment #51)
> > Comment on attachment 8429748 [details]
> > Button.Toast.After.pre19.Tablet.Pressed.png
> > 
> > Looks good. I'd prefer the tap area to cover full width between separator
> > and left edge of the toast and full toast height. Not a big deal though. Up
> > to you and antlam.
> 
> I'd agree with Lucas here. Padding the visual hit area seems a bit awkward..
> Sorry for the late reply this one must've slipped through.

I should have said as much, but I landed with the changes lucasr suggested.  It makes the experience better, and it made the code simpler.  Try it!

Comment 62

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Nick Alexander :nalexander from comment #61
> I should have said as much, but I landed with the changes lucasr suggested. 
> It makes the experience better, and it made the code simpler.  Try it!

Looks good!

Comment 63

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think I overlooked the lack of transparency in the mocks. My apologies! Could we give the new toasts (with transparency) a try? :)

Comment 64

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

^ oh, and maybe a minor positioning tweak to help it feel less "in-your-face"

Comment 65

[Ryan VanderMeulen [:RyanVM]]

This was reverted on Aurora for Fx32 in bug 1023407. It remains on trunk for Fx33.

Comment 66

[:Margaret Leibovic]

I would call this disabled rather than wontfix. We may look to re-enable this feature after the follow-up work bakes on Nightly.

Comment 67

[Teodora Vermesan (:TeoVermesan)]

Opening a page/link in Private Tab has the quick switch-to-tab feature: a notification "New private tab opened - Switch" is displayed, so:
Verified fixed on:
Device: LG Nexus 4
OS: Android 4.4.2
Build: Firefox for Android 33.0a1 (2014-07-02)

Comment 68

[Cristina Madaras, QA [:CristinaM]]

Test cases added in Moztrap: 
https://moztrap.mozilla.org/manage/case/14066/ - Switch to the new opened tab
https://moztrap.mozilla.org/manage/case/14067/ - Switch to the new private opened tab