843619 - Remove tab tray menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Simplified tab menu

Having lived with this in Nightly for a couple weeks, having a menu icon that changes its contents depending on your mode feels... wrong. It's not uncommon to see this behaviour on Android, but given the contents of our tab menu, I don't feel it's totally necessary here.

I propose we remove it, leaving only the "new tab" icon on the far right. In terms of what we lose by killing the menu, "New Tab" and "New Private Tab" can already be accessed elsewhere in the UI, and we can probably move "Close all tabs" into the tab UI list if we want to keep it. Though I am open to removing it altogether as well.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Given that this menu is in 20, wouldn't we be regressing a feature that users are used to for 12 weeks, if we land now in 22?

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This removes all the code related to the small little menu, that made me cry (esp. on tablets).

Comment 3

[Sriram Ramasubramanian [:sriram]]

Wrong flags :(

Comment 4

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

Comment on attachment 716789 [details] [diff] [review]
Patch

We could uplift this to Aurora to minimize the 12 weeks to 6 weeks.

The new Tabs UI code landed so it might have bitrotted this patch. Watch out!

Comment 5

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8134dfa481

Comment 6

[Aaron Train [:aaronmt]]

farewell menu!

Comment 7

[Ian Barlow (:ibarlow)]

we hardly knew ye...

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for robocop failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5e95d8db02

https://tbpl.mozilla.org/php/getParsedLog.php?id=20084878&tree=Mozilla-Inbound

0 INFO SimpleTest START
1 INFO TEST-START | testTabsTrayMenu
2 INFO TEST-PASS | testTabsTrayMenu | Checking elements - all elements present
3 INFO TEST-PASS | testTabsTrayMenu | Initial number of tabs correct - 1 should equal 1
Robocop called click on an element with no listener
4 INFO TEST-UNEXPECTED-FAIL | testTabsTrayMenu | checking that tabs menu clicked - tabs menu element clicked
Exception caught during test!
junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testTabsTrayMenu | checking that tabs menu clicked - tabs menu element clicked
	at junit.framework.Assert.fail(Assert.java:47)
	at org.mozilla.fennec.FennecMochitestAssert._logMochitestResult(FennecMochitestAssert.java:107)
	at org.mozilla.fennec.FennecMochitestAssert.ok(FennecMochitestAssert.java:136)
	at org.mozilla.fennec.tests.testTabsTrayMenu.addPrivateTab(testTabsTrayMenu.java:100)
	at org.mozilla.fennec.tests.testTabsTrayMenu.testTabsTrayMenu(testTabsTrayMenu.java:49)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:204)
	at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:194)
	at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:186)
	at org.mozilla.fennec.tests.BaseTest.runTest(BaseTest.java:130)
	at junit.framework.TestCase.runBare(TestCase.java:127)
	at junit.framework.TestResult$1.protect(TestResult.java:106)
	at junit.framework.TestResult.runProtected(TestResult.java:124)
	at junit.framework.TestResult.run(TestResult.java:109)
	at junit.framework.TestCase.run(TestCase.java:118)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169)
	at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154)
	at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:520)
	at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1447)
5 INFO TEST-UNEXPECTED-FAIL | testTabsTrayMenu | Exception caught - junit.framework.AssertionFailedError: 4 INFO TEST-UNEXPECTED-FAIL | testTabsTrayMenu | checking that tabs menu clicked - tabs menu element clicked
6 INFO TEST-END | testTabsTrayMenu | finished in 3586ms
7 INFO TEST-START | Shutdown

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Remove test

This patch removes the Robocop test for tabs tray menu button -- as the button is now removed.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=98e8b81bf723

Comment 11

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cacfde64decd
https://hg.mozilla.org/integration/mozilla-inbound/rev/0db8acdceffa

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cacfde64decd
https://hg.mozilla.org/mozilla-central/rev/0db8acdceffa

Comment 13

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 22.0a1 (2013-02-28)
-device: Samsung Galaxy Nexus
-OS: Android 4.2.2

Comment 14

[Markus Popp]

Is it smart to introduce the menu in Firefox 20, only to drop it in Firefox 22? If you really want it gone, it should be gone in Firefox 20 already, before people get used to it.

I especially find the "Close all Tabs" menu item useful (using Firefox 20 Beta currently), and will certainly miss it.

Comment 15

[Aaron Train [:aaronmt]]

Looks like bug 817716 should cover; need some new UX for where it should live

Comment 16

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

Sriram - We need to back this out of Fx20 and Fx21 too. Please create whatever patches we need and request approval.

Comment 17

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: For Aurora

This patch is almost same as https://bugzilla.mozilla.org/attachment.cgi?id=716789 , however re-based it with aurora.

Comment 18

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 718535 [details] [diff] [review]
Patch: Remove test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: No user impact. Try server will fail.
Testing completed (on m-c, etc.): Landed in m-c long back.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: For Beta

This is similar to Aurora. Just a minor rebase in Makefile.in

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 718535 [details] [diff] [review]
Patch: Remove test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature.
User impact if declined: None. This is for saving try server from burning.
Testing completed (on m-c, etc.): Landed in m-c long back.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 21

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

Comment on attachment 725085 [details] [diff] [review]
Patch: For Aurora

Do a local build to make sure things work OK

Comment 22

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

Comment on attachment 725086 [details] [diff] [review]
Patch: For Beta

Same

Comment 23

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Comment on attachment 725085 [details] [diff] [review]
> Patch: For Aurora
> 
> Do a local build to make sure things work OK

I've tested this on a local build. Everything works fine -- just like in nightly.

Comment 24

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 725085 [details] [diff] [review]
Patch: For Aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature.
User impact if declined: Menu button will show a menu -- which is good. But we are removing it in 22. Users will be confused.
Testing completed (on m-c, etc.): Verfied on 02/28.
Risk to taking this patch (and alternatives if risky): None. Removing stuff is (usually) not a problem.
String or UUID changes made by this patch: None.

Comment 25

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 725086 [details] [diff] [review]
Patch: For Beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature.
User impact if declined: Menu button will show a menu -- which is good. But we are removing it in 22. Users will be confused.
Testing completed (on m-c, etc.): Verfied on 02/28.
Risk to taking this patch (and alternatives if risky): None. Removing stuff is (usually) not a problem.
String or UUID changes made by this patch: None.

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 725086 [details] [diff] [review]
Patch: For Beta

Shipping the final feature spec is desirable, when low risk. Approving.

Comment 27

[Sriram Ramasubramanian [:sriram]]

Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/cd1d1f737528
https://hg.mozilla.org/releases/mozilla-aurora/rev/5cc89947ea81

Comment 28

[Sriram Ramasubramanian [:sriram]]

Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/7eea2fbec71e
https://hg.mozilla.org/releases/mozilla-beta/rev/73dcbe89d3c3

Comment 29

[Andreea Pod]

Verified fixed on:
-build: Firefox for Android 20.0b6 (2013-03-21)
-device: Samsung Galaxy S II
-OS: Android 4.0.3

Comment 30

[Kevin Brosnan [Ex-Mozilla]]

This never made it to the release channel so we should not relnote.