Closed Bug 1209967 Opened 9 years ago Closed 9 years ago

Toolbar: Always show the overflow menu

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 fixed, firefox46 fixed, fennec45+)

Status:
RESOLVED FIXED
Milestone:
Firefox 46
Tracking Flags:
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
fennec 45+ ---

People

(Reporter: sebastian, Assigned: mcomella)

References

Details

Attachments

(9 files, 5 obsolete files)

HTC Desire HD (Android 2.3.5) with overflow menu
945.77 KB, image/jpeg
Details
MozReview Request: Bug 1209967 - Toolbar: Always show the overflow menu.
40 bytes, text/x-review-board-request
sebastian
: review+
Sylvestre
: approval-mozilla-aurora+
Details
MozReview Request: Bug 1209967 - Remove HardwareUtils.hasMenuButton. r=sebastian
40 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1209967 - Remove android:icons from GB menu. r=sebastian
40 bytes, text/x-review-board-request
sebastian
: review+
Details
New GB menu
30.56 KB, image/png
antlam
: feedback+
Details
MozReview Request: Bug 1209967 - Remove Asserts from BrowserApp. r=me
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1209967 - Fix AppMenuComponent to work on GB. r=sebastian
40 bytes, text/x-review-board-request
sebastian
: review+
Details
MozReview Request: Bug 1209967 - review: Set menu buttons visible in xml & remove dynamic visibility. r=me
40 bytes, text/x-review-board-request
Details
Screenshot_2016-02-04-11-20-25.png
120.16 KB, image/png
Details
Currently we only show the overflow menu if the device has no menu button. On Gingerbread we show an old Android menu and on Android 4 phones with menu button we render the fancier overflow menu like an old Android menu at the bottom of the screen. Let's just always show the overflow menu without those special cases. Why? 1) Greatly reduce code complexity. 2) We have been thinking about unifying the UI in bug 1202076 and this is a step in this direction. 3) We are considering to switch to the toolbar API in bug 1126061. This would kill all the special cases too and reducing the complexity beforehand will make switching to the toolbar API easier. 4) Some devices / OS combinations are just broken: bug 1209902 5) Easier UI testing.
Attached patch 1209967-overflow-menu.patch (obsolete) — — Splinter Review
This is a small patch for enabling the overflow menu everywhere just in Nightly. The idea is to have this in Nightly and see if this triggers any regressions. If not we can start to actually switch and remove code. I'm going to push that patch to try to see what tests fail.
Comment on attachment 8670877 [details] [diff] [review] 1209967-overflow-menu.patch Review of attachment 8670877 [details] [diff] [review]: ----------------------------------------------------------------- If you're planning to land this in Nightly, could you just short-circuit the `hasMenuButton` method directly?
(In reply to :Margaret Leibovic from comment #4) > If you're planning to land this in Nightly, could you just short-circuit the > `hasMenuButton` method directly? Yeah, I thought about this. I didn't do it because I wanted to avoid that hasMenuButton() is effectively lying. But for a temporary test in Nightly this might be okay. There's one call to hasMenuButton() in FxAccountStatusFragment that I did not look at yet.
(In reply to Sebastian Kaspari (:sebastian) from comment #6) > Created attachment 8671258 [details] [diff] [review] > 1209967-menubutton.patch > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5af95fff8e I wonder if anything wonky might be happening here due to our testing infrastructure... Also, feel free to order a test device on Service Now if you need one with a problematic menu button configuration.
Assignee: nobody → s.kaspari
* testNewTab, testUITelemetry, testAboutHomeVisibility: Those passe locally. Maybe I need to create a restrained build and test with that one again. * testSessionHistory fails locally too: > FAIL The overflow menu button is visible > FAIL Exception caught
(In reply to Sebastian Kaspari (:sebastian) from comment #9) > Maybe I need to create a restrained build and test with that one again. Yep. I can see some of the errors now. I can see bug 1114621 too.
A bunch of things are shielded by Versions.feature11Plus and that gets only visible in a constrained build (because then: MIN_SDK_VERSION >= 10 -> Versions.feature11Plus = false).
Attachment #8670877 - Attachment is obsolete: true
Attachment #8671258 - Attachment is obsolete: true
NI sebastian to get a screenshot of the menu to figure out how much work this might be.
Flags: needinfo?(s.kaspari)
Attached image IMG_20151106_141437.jpg (obsolete) —
With the crashes fixes I end up with the following menu (see screenshot). This is only happening with MOZ_ANDROID_RESOURCE_CONSTRAINED set. So I guess that there's more menu code shielded by AppConstants.Versions.
Flags: needinfo?(s.kaspari)
Bug 1209967 - Toolbar: Always show the overflow menu.
Bug 1209967 - WIP: Use GeckoInflater and custom panel views everywhere.
I pushed my quite hacky wip patches to reviewboard.
Thanks Sebastian! NI self to investigate.
Flags: needinfo?(michael.l.comella)
fwiw, I think the imperfect menu in comment 13 could be just as good if not better than the standard one used on GB – it shows more options on my Nexus S than the stock menu. If we reorder it to keep the same options in the top 5, I think it would be shipable. That being said, I'll dig into to try to find out why the menu icons are not being used. I see some MenuItemCompat.getActionView calls in here [1] – maybe related. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=3543772a390b#2999
(In reply to Michael Comella (:mcomella) from comment #18) > fwiw, I think the imperfect menu in comment 13 could be just as good if not > better than the standard one used on GB – it shows more options on my Nexus > S than the stock menu. If we reorder it to keep the same options in the top > 5, I think it would be shipable. That's a good point. I was so focused on enabling the same menu that I did not think of that. And: Users with an Android 4+ phone with a menu button (or the poor folks with a OnePlus X/2) will get the nice overflow menu - only GB users will have this light menu. We could get this into a landable patch and continue to get the full menu on GB in a follow-up bug.
MenuItemCompat.getActionView defers to SupportMenuItem.getActionView if it's a SupportMenuItem [1] or the MenuItemCompat implementation based on version if it's not. The Honeycomb implementation calls out to MenuItem.getActionView [2, 2.5] and the BaseImplementation (pre-v11) [3] returns null. That's probably the issue. I'll see if I can't override it. re comment 19, a follow-up sounds good. I'll review these patches and see if there's anything else to clean up. Sebastian, do you think this is feasible to uplift to Aurora? I'm chasing bug 1215079. [1]: http://androidxref.com/5.1.1_r6/xref/frameworks/support/v4/java/android/support/v4/view/MenuItemCompat.java#321 [2]: http://androidxref.com/5.1.1_r6/xref/frameworks/support/v4/java/android/support/v4/view/MenuItemCompat.java#175 [2.5]: http://androidxref.com/5.1.1_r6/xref/frameworks/support/v4/honeycomb/android/support/v4/view/MenuItemCompatHoneycomb.java#39 [3]: http://androidxref.com/5.1.1_r6/xref/frameworks/support/v4/java/android/support/v4/view/MenuItemCompat.java#130
Flags: needinfo?(s.kaspari)
Anthony, how would you feel about shipping the menu in comment 13 on GB devices? We can re-order to make sure the items in the current menu are shown in the initial showing of the new menu. We could try implementing the full menu in a follow-up bug (that will probably never get done because it's GB).
Flags: needinfo?(alam)
Flags: needinfo?(michael.l.comella)
First row ltr: *Reload *Forward *Bookmark Second row ltr: *New tab *New private tab *More
(In reply to Michael Comella (:mcomella) from comment #20) > Sebastian, do you think this is feasible to uplift to Aurora? I'm chasing > bug 1215079. I remember testing the normal (not constrained) build with menus on my GB phone and it worked nicely. I don't know how the situation is on Honeycomb - but with some testing on GB/HC I feel like this should be okay to uplift. Robocop tests could be an issue though. (In reply to Michael Comella (:mcomella) from comment #20) > MenuItemCompat.getActionView defers to SupportMenuItem.getActionView if it's > a SupportMenuItem [1] or the MenuItemCompat implementation based on version > if it's not. The Honeycomb implementation calls out to > MenuItem.getActionView [2, 2.5] and the BaseImplementation (pre-v11) [3] > returns null. In my patch there's also some showAsAction code commented out. You'll probably need that too.
https://reviewboard.mozilla.org/r/24501/#review22859 For time purposes, I'm just going to fold these and make the changes myself. I'm using https://sriramramani.wordpress.com/2012/08/30/custom-menu/ as a guide to make sure it's roughly correct. ::: mobile/android/base/GeckoApp.java:344 (Diff revision 1) > - if (Versions.feature11Plus) { > + if (Versions.feature11Plus || AppConstants.NIGHTLY_BUILD) { I'd like to uplift this so maybe we should can the NIGHTLY_BUILD flags. Alternatively, if this is unsafe, I can back out the Theme.AppCompat stuff but that sounds like a nightmare. ::: mobile/android/base/GeckoApp.java:398 (Diff revision 1) > public View onCreatePanelView(int featureId) { If we're going the Nightly flag route, I think this may break in v11+ builds.
https://reviewboard.mozilla.org/r/24503/#review22877 ::: mobile/android/base/GeckoApp.java (Diff revision 1) > - if (Versions.feature11Plus && featureId == Window.FEATURE_OPTIONS_PANEL) { Why did you remove `Window.FEATURE_OPTIONS_PANEL`? When I call these methods on GB, `featureId == 0 == Window.FEATURE_OPTIONS_PANEL`. While I don't fully understand the flag because I don't know where else `onPreparePanel` and friends is called, it seems more correct to leave this guard in. ::: mobile/android/base/menu/GeckoMenuInflater.java:133 (Diff revision 1) > + //item.showAsAction = a.getInt(R.styleable.MenuItem_android_showAsAction, 0); I'll write a post-patch to bring these back. They're API 11+ so I'll add guards. ::: mobile/android/base/resources/menu/browser_app_menu.xml:12 (Diff revision 1) > <item android:id="@+id/back" I like the idea of combining these but we may need to reorder pending UX approval. Also, android:icon might preload an icon, meaning we're using more resources on GB than we'd otherwise need to.
Bug 1209967 - Toolbar: Always show the overflow menu. This required us to use the custom menu inflater and panels on GB. One side effect of this change is that the menu button no longer closes the menu on GB. I tried to fix this but BrowserApp.onKeyDown is not fired when the menu is open on GB and I'm not sure why that is. However, I don't think it's worth my time to fix since it's GB.
Attached image New GB menu —
Assignee: s.kaspari → michael.l.comella
Flags: needinfo?(alam)
Attachment #8688742 - Flags: feedback?(alam)
Anthony, what do you think of the new menu design? For the first five items, I used the same order as the grid on the current GB menu and the remaining order is from the menu on HC+ devices.
Flags: needinfo?(s.kaspari)
This crashes on non-GB builds, probably because I screwed with the resources – investigating: 1-17 17:31:40.562 E/GeckoCrashHandler(10461): android.content.res.Resources$NotFoundException: Resource ID #0x7f0200e2 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at android.content.res.Resources.getValue(Resources.java:2036) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at android.content.res.Resources.getDrawable(Resources.java:1611) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuItem.getIcon(GeckoMenuItem.java:159) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.MenuItemActionBar.initialize(MenuItemActionBar.java:35) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuItem.setShowAsAction(GeckoMenuItem.java:402) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuItem.setShowAsAction(GeckoMenuItem.java:375) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuInflater.setValues(GeckoMenuInflater.java:159) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuInflater.parseMenu(GeckoMenuInflater.java:108) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.menu.GeckoMenuInflater.inflate(GeckoMenuInflater.java:61) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.BrowserApp.onCreateOptionsMenu(BrowserApp.java:2824) 11-17 17:31:40.562 E/GeckoCrashHandler(10461): at org.mozilla.gecko.GeckoApp.onCreatePanelMenu(GeckoApp.java:426) When we uplift, we only really need to uplift the first changeset.
Comment on attachment 8688742 [details] New GB menu Considering the restraints we're working with here, this is probably our best option. I think this is OK :)
Attachment #8688742 - Flags: feedback?(alam) → feedback+
Comment on attachment 8688741 [details] MozReview Request: Bug 1209967 - Remove android:icons from GB menu. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25397/diff/1-2/
Turns out I removed `drawable-*dpi/ic_menu_back.png` but there wasn't an ic_menu_back in the v11 folders so I moved it there instead of fully removing it. No crashes on 2.3 or 4+. Still remaining: fix the tests.
Comment on attachment 8688739 [details] MozReview Request: Bug 1209967 - Toolbar: Always show the overflow menu. https://reviewboard.mozilla.org/r/25393/#review23363 LGTM. I wanted to test the series of patches on my GB phone but I've some conflicts when applying.
Attachment #8688739 - Flags: review?(s.kaspari) → review+
Attachment #8688740 - Flags: review?(s.kaspari) → review+
Comment on attachment 8688740 [details] MozReview Request: Bug 1209967 - Remove HardwareUtils.hasMenuButton. r=sebastian https://reviewboard.mozilla.org/r/25395/#review23365 ::: mobile/android/base/tabs/TabsPanel.java:370 (Diff revision 1) > - mMenuButton.setVisibility(View.VISIBLE); > + mMenuButton.setVisibility(View.VISIBLE); NIT: We could just make it visible in the layout and remove this line. Or does this need to be GONE? ::: mobile/android/base/toolbar/BrowserToolbar.java:322 (Diff revision 1) > - menuButton.setVisibility(View.VISIBLE); > + menuButton.setVisibility(View.VISIBLE); NIT: Here too. ::: mobile/android/base/toolbar/BrowserToolbarPreHC.java (Diff revision 1) > - if (!HardwareUtils.hasMenuButton()) { > - // Prevent tabs through the editing mode cancel button (bug 1001243). > - menuButton.setEnabled(!isEditing); Shouldn't we keep this line?
Comment on attachment 8688741 [details] MozReview Request: Bug 1209967 - Remove android:icons from GB menu. r=sebastian https://reviewboard.mozilla.org/r/25397/#review23367 Nice!
Attachment #8688741 - Flags: review?(s.kaspari) → review+
As I mention in bug 1215079 comment 28, this bug will fix bug 1215079, which we're only writing a patch for 44 for so I'm tracking 45 here so a fix is not neglected.
tracking-fennec: --- → 45+
I removed the Asserts and this fixed 4.0+. 2.3 is still broken. The testNewTab failures may be intermittent issues (bug 1217484) and testAppMenuPathways is more likely to be my fault.
https://reviewboard.mozilla.org/r/25395/#review23365 > Shouldn't we keep this line? It's kept – it's just moved up a few lines.
Comment on attachment 8688739 [details] MozReview Request: Bug 1209967 - Toolbar: Always show the overflow menu. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25393/diff/1-2/
Comment on attachment 8688740 [details] MozReview Request: Bug 1209967 - Remove HardwareUtils.hasMenuButton. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25395/diff/1-2/
Comment on attachment 8688741 [details] MozReview Request: Bug 1209967 - Remove android:icons from GB menu. r=sebastian Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25397/diff/2-3/
Bug 1209967 - Remove Asserts from BrowserApp. r=me We added some Asserts but the robocop tests fail for an unknown reason. The error was: E/MessageQueue-JNI( 1972): java.lang.IllegalAccessError: Class ref in pre-verified class resolved to unexpected implementation I tried a few things but in the end just decided to remove them.
Bug 1209967 - Fix AppMenuComponent to work on GB. r=sebastian This required us to use the menu button to open the menu and check if the menu is open by looking at more menu items due to intermittent issues where the menu would not always be scrolled to the top (I think Robotium scrolled the menu before it finished appearing so we couldn't see the first item and failed the isOpen test).
Attachment #8692230 - Flags: review?(s.kaspari)
Comment on attachment 8692230 [details] MozReview Request: Bug 1209967 - Fix AppMenuComponent to work on GB. r=sebastian https://reviewboard.mozilla.org/r/26237/#review23683
Attachment #8692230 - Flags: review?(s.kaspari) → review+
So it looks like the testNewTab/testPrivateBrowsing failures may be related to my changes as well as the testAppMenuPathways failure. testDistribution appears to be unrelated, based on failure frequency (2/6).
In testAppMenuPathways, the failure is: 15:36:57 INFO - TEST-PASS | testAppMenuPathways | The menu item Page is visible - 0 should equal 0 15:36:57 WARNING - TEST-UNEXPECTED-FAIL | testAppMenuPathways | The page menu item is not null - didn't expect null, but got it 15:36:57 INFO - 0 ERROR Exception caught during test! - junit.framework.AssertionFailedError: TEST-UNEXPECTED-FAIL | testAppMenuPathways | The page menu item is not null - didn't expect null, but got it So the Menu opens, we confirm Page is there, and then we drop the ball from when Page is clicked and when "Save as PDF" is accessed, where it is null. The failure screenshot shows no menu is open. I think either: * The click misses and we dismiss the menu * There is a split second between when the page submenu appears and when page is clicked. Maybe the delay is so long on the emulator that we never have a chance to open the submenu. And it looks like we only wait one second for the submenu text to appear before trying to find the view [1]. Let's try increasing that. [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/components/AppMenuComponent.java#161
I'm working on testNewTab/testPrivateBrowsing in bug 1217484.
In the latest run, the intermittent (which I think I introduced), "testAppMenuPathways | The page menu item is not null - didn't expect null, but got it", came up 1/10 times on 2.3 (but there's no error screenshot so maybe this was a one-off issue). We might be able to make it go away by increasing the delay further but let's land this to see if it's actually a problem. Otherwise, everything is green – yay!
The test has the following error: 15:14:19 INFO - panda-0599: 15:14:19 INFO - Started Wed, 02 Dec 2015 15:14:19 15:14:19 INFO - Running test tcheck2: 15:14:19 INFO - Started Wed, 02 Dec 2015 15:14:19 command timed out: 3600 seconds without output running ['/tools/buildbot/bin/python', 'scripts/scripts/android_panda_talos.py', '--talos-suite', 'remote-trobocheck2', '--cfg', 'android/android_panda_talos_releng.py', '--branch-name', 'Fx-Team', '--blob-upload-branch', 'Fx-Team'], attempting to kill It seems that testCheck2 doesn't get to run at all before we hang. Geoff, any ideas? My patch here makes the 3-dot toolbar menu appear on all devices (i.e. devices with hardware menu buttons will also have a software menu button).
Flags: needinfo?(gbrown)
Sorry, I'm coming up empty. Your patches look unlikely to cause trouble for Talos -- I don't see anything amiss from code inspection. And there's not much to work with from the logs. I would love to see a post-run logcat. Since your patches affected all 3 Talos jobs but not any of the Android 2.3 or 4.3 tests, I suspect that the issue is 4.0-specific (or pandaboard specific?) and is causing Firefox to hang (maybe crash?) on startup or on page load. :jmaher -- Any ideas? Can you suggest a way to get better diagnostics?
Flags: needinfo?(gbrown) → needinfo?(jmaher)
I agree, a logcat would be nice here. Normally we do that, but in this case talos is timing out at the mozharness layer. I might be able to get this running on my local nexus s tomorrow.
I spoke with someone (from Mozilla Online?) who uses a Chinese-carrier phone and he mentioned that many phone manufacturers customize the menu button to show a carrier system menu, rather than the application menu. This should increase the priority of landing this fix. He mentioned FF45 seemed like a good release target. Joel, any word here?
I tried pushing sub-sets of these patches to try. Android talos passes with: https://hg.mozilla.org/integration/fx-team/rev/7942bfbde7f2ecefd4b6aecb675e76f3e1bf8022 Bug 1209967 - Toolbar: Always show the overflow menu. https://hg.mozilla.org/integration/fx-team/rev/035f6f5db4fe43f118c5d022cd87e93facd63849 Bug 1209967 - Remove HardwareUtils.hasMenuButton. r=sebastian Android talos hangs with: https://hg.mozilla.org/integration/fx-team/rev/7942bfbde7f2ecefd4b6aecb675e76f3e1bf8022 Bug 1209967 - Toolbar: Always show the overflow menu. https://hg.mozilla.org/integration/fx-team/rev/035f6f5db4fe43f118c5d022cd87e93facd63849 Bug 1209967 - Remove HardwareUtils.hasMenuButton. r=sebastian https://hg.mozilla.org/integration/fx-team/rev/5cea26659e0df7c4b6b1bd77dc7361877f8501d1 Bug 1209967 - Remove android:icons from GB menu. r=sebastian That's consistent with mcomella's conjecture from earlier today. Trying to narrow it down further...
:( we were not able to reproduce this on a device locally with the same build/robocop/tools. Ideally we will narrow this down on try, or reproduce with a loaner or locally early next week.
Flags: needinfo?(jmaher)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e8f581c170 has all the original patches, updated for bitrot (path changes for mobile/android/base), but with the "@android:color/transparent" resource changes reverted -- no more talos hangs! All unit tests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76c827a2ce73 Back to you mcomella...
(In reply to Geoff Brown [:gbrown] from comment #68) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e8f581c170 has all > the original patches, updated for bitrot (path changes for > mobile/android/base), but with the "@android:color/transparent" resource > changes reverted -- no more talos hangs! Weird – I can't explain it, but I think that's our solution. Thanks for your help Joel & Geoff!
I made the same changes as Geoff (using a rectangle shape with a transparent background for the null reference assets) but I folded them into the original commit. Will land and see how it fairs on fx-team.
Since this slipped to 46, I'm unclear if I should uplift – let's ask triage! There is no rush, except for: (In reply to Michael Comella (:mcomella) from comment #64) > I spoke with someone (from Mozilla Online?) who uses a Chinese-carrier phone > and he mentioned that many phone manufacturers customize the menu button to > show a carrier system menu, rather than the application menu. This should > increase the priority of landing this fix. He mentioned FF45 seemed like a > good release target. Perhaps we should check in with Mozilla Online here?
tracking-fennec: 45+ → ?
Let's nom these for uplift.
tracking-fennec: ? → 45+
Flags: needinfo?(michael.l.comella)
Comment on attachment 8688739 [details] MozReview Request: Bug 1209967 - Toolbar: Always show the overflow menu. Please uplift all of the patches landed in comment 71. Approval Request Comment [Feature/regressing bug #]: None [User impact if declined]: Users will have the current state of menu buttons (users w/ no hardware menu button will see a 3-dot menu button, users w/ hardware menu buttons will not see a 3-dot menu button). This uplift is important because in China, the hardware menu button opens the phone manufacturer's menu and the software menu button is needed to open the menu (see comment 64). [Describe test coverage new/current, TreeHerder]: Tested locally. [Risks and why]: It's a large changeset so honestly, there's a lot to go wrong. In the first commit, "Toolbar: Always show the overflow menu.", there is the potential for logic errors and things like NullPointerExceptions. Commit 2, "Remove HardwareUtils.hasMenuButton." has potential for logic errors, and the others are much lower risk. [String/UUID change made/needed]: None
Flags: needinfo?(michael.l.comella)
Attachment #8688739 - Flags: approval-mozilla-aurora?
Comment on attachment 8688739 [details] MozReview Request: Bug 1209967 - Toolbar: Always show the overflow menu. Taking it: * It is early in the aurora cycle * I am not sure that let this change rides the train from nightly will provide much more feedback * The android team is on top of this kind of things and addresses quickly the regressions * I am feeling lucky
Attachment #8688739 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Working great on nightly. Thanks everyone!
Blocks: 1236156
(In reply to KOLANICH from comment #80) > *FUUUUUU.... Reducing code complexity is not an argument for degrading UI so > badly. Could you reduce code complexity in another way than wasting screen > space with unneeded buttons and making it uncomfortable to reach tabs > counter button? Let's continue this discussion in bug 1236156.
(Moving NI to bug 1236156)
Flags: needinfo?(s.kaspari)
The problem with this change is that it removes space on my already narrow screen; right now the URL bar only shows "https://bugzilla.mozilla", not very useful (see attachment). It is also unnecessary for me at least since there's already a menu button on my phone, so this change is a regression for me.
Depends on: 1289101
No longer depends on: 1289101
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: