Mockup: http://cl.ly/image/1O0b3k1b460T Pressing the close all tabs button in the tab menu should clear all the tabs in your current section. * If you are in private tabs, it clears all your private tabs. * If you are in normal tabs, it clears all your normal tabs. * This option should not be available to you in synced tabs, partly because they are not technically open, and partly because we probably don't want people to be able to remote wipe their tabs. This is probably something that should be undoable, so timing this release to match up with bug 701725 would be nice.

I'm confused, looks like Sriram landed 'Close all Tabs' in bug 806927. So what is this bug?

(In reply to Aaron Train [:aaronmt] from comment #1) > I'm confused, looks like Sriram landed 'Close all Tabs' in bug 806927. So > what is this bug? I think it's here to describe the behavior of Close All Tabs once private/normal/synced tabs are shown in separate lists, which we don't do yet.

So close-all-synch tabs if you're currently displaying the synch tabs tray, close-all-private tabs if you've selected the private tabs tray / "mustache", and close-all-"public" tabs if you've selected the public tabs tray / "hat" ... vs close-all anywhere it seems to do now.

Now that bug 843619 landed, I think we'll need new UX specs for this.

Req tracking for feature parity on already released product

We can always track this later if needed.

Maybe this could be shown in a context menu which is invoked by long-tapping any tab. Would that be accessible/visible enough?

Sriram and I talked about this a while back, perhaps we can actually hide it behind the title bar of the the tab list when more than one tab is open, and a user can pull down to expose a "close all tabs" command, like this http://cl.ly/image/0q3j342F1Z3j

This is probably not entirely trivial given that TwoWayView doesn't support things like header view. Removing from hackathon.

Re-noming, but we'll need some updated UX here.

Can I suggest long-press and a pop-up menu. That way it falls in line with desktop behaviour and doesn't clutter the UI. It'll also provide a platform for bug 701725, things like being able to share a tab without having to load it first (we already do this with links) and other parity-desktop features like Bookmark All Tabs should they wish to be pursued.

This is based on the UX to close all notifications in your notification tray (that 3 offset horizontal lines icon, although I just used an "x" icon here because I just wanted to use something we had in the tree). If we like this approach, we can get a real icon for it. This patch made me remember that our tabs logic is forever hairy, and it might be worth cleaning things up a bit before trying to land this. For one thing, I don't really like how TabsTray depends on GeckoApp to call various methods, but those methods are all implemented in BrowserApp. It seems like we could get rid of those GeckoApp stub methods and just have TabsPanel depend on BrowserApp instead.

(In reply to :Margaret Leibovic from comment #13) > Created attachment 8415680 [details] [diff] [review] > POC: Close all tabs button on right side of tabs tray > > This is based on the UX to close all notifications in your notification tray > (that 3 offset horizontal lines icon, although I just used an "x" icon here > because I just wanted to use something we had in the tree). If we like this > approach, we can get a real icon for it. > > This patch made me remember that our tabs logic is forever hairy, and it > might be worth cleaning things up a bit before trying to land this. For one > thing, I don't really like how TabsTray depends on GeckoApp to call various > methods, but those methods are all implemented in BrowserApp. It seems like > we could get rid of those GeckoApp stub methods and just have TabsPanel > depend on BrowserApp instead. Yeah, our tabs UI code has been in need for some love for ages. For the record, I'd rather make TabsPanel not depend on BrowserApp at all (through listeners or something else). Keep in mind through that we'll probably change the tabs UI code quite a bit as part of the upcoming tablet UI refresh. So, I'd probably avoid a big refactoring effort on our tabs code for now.

Comment on attachment 8415680 [details] [diff] [review] POC: Close all tabs button on right side of tabs tray Review of attachment 8415680 [details] [diff] [review]: ----------------------------------------------------------------- I agree that Tabs shouldn't have a handle to any Activity at all, but some kind of listener/controller interface instead. While the entity implementing that interface may very well be BrowserApp itself for now, abstracting this layer to a separate interface eases the pain for later if we decide we want more fine-grained controller logic. For now, you could create this interface implemented by BrowserApp to handle these events if you want to remove the GeckoApp/BrowserApp dependency, which by itself should be a pretty minimal change. ::: mobile/android/base/Tabs.java @@ +320,5 @@ > // Pass a message to Gecko to update tab state in BrowserApp > GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Tab:Closed", String.valueOf(tabId))); > } > > + public synchronized void closeAllTabs(boolean isPrivate) { This method doesn't need to be synchronized (it doesn't touch mSelected or mTabs). @@ +335,5 @@ > + } > + > + Tab newTab; > + if (isPrivate) { > + newTab = loadUrl(AboutPages.PRIVATEBROWSING, Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE); So closing all private tabs with this button will keep us in private browsing mode, whereas closing the last private tab individually switches us out of it? For consistency, maybe we should just leave private browsing like we do in the single close case.

Ian, what do you think about this idea to have a "close all" button in the tabs tray next to the "add tab" button? If you like this idea, can you make me an icon? :)

Margaret do you have a build I can try? I'm a little hesitant to put such a destructive command right next to a constructive one, but I'd like to try it and get a feel for it too before I give feedback one way or the other.

(In reply to Ian Barlow (:ibarlow) from comment #17) > Margaret do you have a build I can try? I'm a little hesitant to put such a > destructive command right next to a constructive one, but I'd like to try it > and get a feel for it too before I give feedback one way or the other. Here's a build: https://dl.dropboxusercontent.com/u/3358452/fennec-close-all.apk Thinking about this a bit more, I agree we should avoid putting this somewhere where people will accidentally hit it. Maybe we should make it a contextual action? Although we don't have any contextual actions on the tabs tray right now.

Thanks Margaret. Yeah, having tried it I think that control needs to live somewhere else. One approach might be to put it inside a menu in the action bar, like in the original mockup in this bug. That feels a little funky, though, so another approach might be something like this: http://cl.ly/image/3A1f2I2i1y10, where the close control lives at the bottom of the tab list (or the far right, for horizontal tab trays). Couple of other things I'd like to see: * only show this if 2 or more tabs are open * when clearing tabs, use a 'cascade' animation similar to the android notification tray, before closing the tray. That visual feedback of what's happening is something we need here.

In order to make the animations work, I think the best approach would be to do this directly in TabsTray, so that we can basically just make a bunch of successive calls to animate all the tabs closed. TwoWayView doesn't currently support footer views, but that seems like it would be the best way to implement this. If so, I think we should work on a patch for TwoWayView. Lucas, what do you think?

(In reply to :Margaret Leibovic from comment #20) > In order to make the animations work, I think the best approach would be to > do this directly in TabsTray, so that we can basically just make a bunch of > successive calls to animate all the tabs closed. > > TwoWayView doesn't currently support footer views, but that seems like it > would be the best way to implement this. If so, I think we should work on a > patch for TwoWayView. Lucas, what do you think? Implementing support for footer views in TwoWayView would be non-trivial to implement as header/footer views are kinda special i.e. they don't get recycled, they require the view to wrap the adapter, etc. We can do that. But if we're looking for a simpler/quicker solution, we can just change TabsAdapter to have multiple view types (say, TAB and CLOSE_ALL) and auto-append an item (with CLOSE_ALL type) to the end of the list when the list has more than one tab. Before we go ahead with this design though, it would be nice to think a bit about how the 'close all' item would look on the horizontal tabs tray.

(In reply to Lucas Rocha (:lucasr) from comment #21) > Before we go ahead with this design though, it would be nice to think a bit > about how the 'close all' item would look on the horizontal tabs tray. Good point, I feel like another item at the end of the horizontal tab strip might look weird. It would also be good to think about this bug in the context of some of the other tab redesigns we have in mind. However, I would love to find a way to land a patch for this in the current UI. Maybe a button in the toolbar wouldn't be so bad if we had an undo option? Although restoring long list of tabs might still be slow and annoying.

After skimming through the comments above, from my understanding the goal of supporting "Close all tabs" is to make closing lots of tabs convenient. The new horizontal tab strip has limited real estate and is optimized for quick switching and add tabs. In comparison, "Close all tabs" is not really a frequent action. So I think tab panel is a better place for this action. My idea for solving this on tablet: http://cl.ly/image/2a3r332j0Z2D. Based on the tablet proposal I presented last week, pressing down a tab thumbnail on tab panel triggers reordering a tab and also a trash can for closing tabs. The top bar basically serves as a nicer contextual menu for tabs. Taking advantage of this format, "Close all other tabs" can fit it quite well on the contextual top bar. As long as a tab is selected, the user can tap "Close all other tabs" and enjoy watching a nice transition of closing tabs. I personally think a contextual approach is better than putting the action in a menu and just waiting for our users to discover. This won't close ALL the tabs, but it should certainly make closing lots of tabs in a row easier.

The simplest and best solution is to add a context to the tab indicator (normal tabs, private tabs, sync'd tabs). If a user long presses on normal tabs or private tabs, they get a dialog offering to close all tabs. That's also feasible for smaller device screens plus it works both in portrait and landscape. It also falls in-line with desktop behaviour.

Thanks Yuan, this is some nice thinking -- let's hold onto this for our tabs redesign on tablets. I believe the idea with this bug is more about incremental improvements to the current UI. So in the mean time, do you have any thoughts on how this might be handled with our current tabs UI? In comment 19 I sketched out a rough idea for how a "close all" command might work in a vertical tab list, http://cl.ly/image/3A1f2I2i1y10, but didn't really think through how that might look with a horizontal tab list, which is where Lucas and Margeret seems to be stuck at the moment

I have a hunch that we'll not want to show the undo toast notification for closing all tabs, right?

Unlikely.

(In reply to Aaron Train [:aaronmt] from comment #26) > I have a hunch that we'll not want to show the undo toast notification for > closing all tabs, right? getting a good UX for bug 1017608 might make us want to show a toast

If we were to show a toast, we would have to do something different than what we currently do for the undo close tab toast, since we use session store to do that undoing, and right now we only hold onto one closed tab at a time. We will increase that number for bug 1004850, but it will probably never be more than 10, so we can't count on it. We could do something like not really close the tabs, but it might be tricky to get that logic right. FYI, Chrome doesn't show an undo toast for closing all tabs :) At this point, this bug isn't going to make it into Fx32, since it's hard to come up with a good place to put this control in the UI, balancing the desire to make it easy to access, yet hard to accidentally hit. I did make an add-on that adds a menuitem to the main menu, in case users really want this functionality until we figure this out: https://addons.mozilla.org/en-US/android/addon/close-all-tabs-mobile/

I wonder if we could just use an action mode here. Long press to select tabs. We show "Close" button "Select All" buttons in the actionbar. You can select whatever you want and close them all in one tap...

So uh, just to circle back... what happened to the idea in https://bugzilla.mozilla.org/show_bug.cgi?id=817716#c19 -- placing a control at the bottom of a vertical list, or the far right of a horizontal one?

(In reply to Ian Barlow (:ibarlow) from comment #31) > So uh, just to circle back... what happened to the idea in > https://bugzilla.mozilla.org/show_bug.cgi?id=817716#c19 -- placing a control > at the bottom of a vertical list, or the far right of a horizontal one? I thought that we were uncertain of how good that would look in the horizontal tab strip, but maybe I'm confused with the inline undo we were talking about in bug 701725. This would also be kinda tricky to implement with our current tabs tray, but I could try the idea lucasr suggested to make the adapter into a multi-type adapter like we use in other parts of the UI, such as the history list (where we have headers mixed with regular items).

Doing this in TabsTray is nice because we don't need to worry about private/regular tabs, since we're only closing tabs in the current view. One tricky thing here is that I'm not sure how we'll animate these tabs closing. In order to animate, we need to get the individual views for the tabs, but this is tricky because the adapter view recycles these views, so we would only have access to the ones available on the screen. I haven't played around with this yet, but maybe we can just try getting the second-to-last child in the view (the last child being the footer), then close it and repeat until there are no more tabs, simulating what a user would do.

Here are screenshots for tablet/phone in different orientations. I tried adding a close icon to the left of the text with a compound drawable, but it wouldn't center properly, so I would need to make a more complex view if we want to add that icon.

Comment on attachment 8433687 [details] [diff] [review] WIP - Add "Close All Tabs" item to the end of tab strip Review of attachment 8433687 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/TabsTray.java @@ +166,5 @@ > + > + mOnCloseAllClickListener = new View.OnClickListener() { > + @Override > + public void onClick(View v) { > + // XXX: Figure out a way to animate this. Since the items in the adapter aren't each necessarily associated with a view, I agree with your idea that you should be acting directly on the Views in the ViewGroup to do the animation. The animation for each TabRow closing doesn't have to directly correspond to that Tab being closed; you could animate each TabRow out, then actually iterate and close every Tab once the visible TabRows are gone. @@ +168,5 @@ > + @Override > + public void onClick(View v) { > + // XXX: Figure out a way to animate this. > + final int count = mTabs.size(); > + for (int i = 0; i < count; i++) { Nit: for (Tab tab : mTabs) @@ +237,5 @@ > * > * @param selected position of the selected tab > */ > private void updateSelectedStyle(int selected) { > + if (mTabs == null) { All of these null checks for mTabs in this file makes the code harder to read and more fragile. Instead of setting mTabs to null in clear(), can we just call mTabs.clear() and remove all this special handling?

Here's a patch with the animation. I decided not to try to repurpose the existing animation code, since it wasn't totally straightforward, and there are some things in the individual close animation that we don't need in this new animation (like animating the size of the view). Also this file could totally use some clean-up love, but I don't know how much I want to invest in that, if we're planning to redo our tabs in the near future. Here's a build for ibarlow to test: https://dl.dropboxusercontent.com/u/3358452/fennec-cascade.apk

This is looking pretty awesome, Margaret! Couple comments. ---- Styling I wonder if there is anything we can do to that "close all tabs" text to make it look more like a control. Maybe an icon? Make it look like a link? Needinfo-ing Anthony to take a moment to think about it. Screenshots from build: http://cl.ly/image/2d1W1I1Y2g2R ---- Animation Let's add a little more delay between each thumbnail movement. I was playing around in keynote, and it seems like waiting around 75-100ms between starting each thumbnail animation might give us a more pronounced 'cascade' look we're after. Right now they seem to move a little too much as one group. In landcscape mode, can we try animating the thumbnails up instead of down? Lastly, it feels like closing all tabs should also shut the tab tray at the end. It feels a little odd to just leave it open after you've closed all your tabs.

(In reply to Ian Barlow (:ibarlow) from comment #37) > This is looking pretty awesome, Margaret! Couple comments. Thanks for the feedback! > Animation > > Let's add a little more delay between each thumbnail movement. I was playing > around in keynote, and it seems like waiting around 75-100ms between > starting each thumbnail animation might give us a more pronounced 'cascade' > look we're after. Right now they seem to move a little too much as one > group. Okay, yeah, right now they do all start at once, but they take an increasingly long amount of time to finish. > In landcscape mode, can we try animating the thumbnails up instead of down? We can address this in a separate patch, but would you like that for individual close as well? I was just copying the behavior of the animation when you hit the close button, but I agree that seemed weird to me when I was testing (I expected them to fly up). > Lastly, it feels like closing all tabs should also shut the tab tray at the > end. It feels a little odd to just leave it open after you've closed all > your tabs. Good call, simple to fix.

Comment on attachment 8434604 [details] [diff] [review] Add "Close All Tabs" item to the end of tab strip Clearing review until I address UX concerns. Feel free to make drive-by comments, though! :)

(In reply to :Margaret Leibovic from comment #38) > We can address this in a separate patch, but would you like that for > individual close as well? I was just copying the behavior of the animation > when you hit the close button Oh wow, never noticed that. Yeah, that should be a universal change then.

(In reply to Ian Barlow (:ibarlow) from comment #37) > I wonder if there is anything we can do to that "close all tabs" text to > make it look more like a control. Maybe an icon? Make it look like a link? > Needinfo-ing Anthony to take a moment to think about it. Screenshots from > build: http://cl.ly/image/2d1W1I1Y2g2R This is my biggest issue. The "control" has no affordance. Can we get some ideas for making this look clickable, preferrably ideas that match other places in Fennec and/or other apps? I also think the landscape version will be an L10N problem. Imagine that text twice as long for some languages. I'd love a icon button with a long-press hint, like we have in the action bar. Mentioning action bar makes me wonder if this is better handled in the action bar or an overflow 3-dot menu. Yes, I seem to be a downer today.

My $0.02, why not add a 3 dot menu to the left of the '+' tab in the top bar and stick it in there? This also lets add-on authors utilize that for other things maybe. Closing all tabs I don't imagine is that important enough to have so highly visible anyways.

(In reply to Aaron Train [:aaronmt] from comment #42) > My $0.02, why not add a 3 dot menu to the left of the '+' tab in the top bar > and stick it in there? This also lets add-on authors utilize that for other > things maybe. Closing all tabs I don't imagine is that important enough to > have so highly visible anyways. That was the original idea in this bug. (see comment 0) I'm open to leaving that on the table as an option, but i'd still like Anthony to think about how it might look as an inline control as well.

(In reply to Mark Finkle (:mfinkle) from comment #41) > (In reply to Ian Barlow (:ibarlow) from comment #37) > > > I wonder if there is anything we can do to that "close all tabs" text to > > make it look more like a control. Maybe an icon? Make it look like a link? > > Needinfo-ing Anthony to take a moment to think about it. Screenshots from > > build: http://cl.ly/image/2d1W1I1Y2g2R > > This is my biggest issue. The "control" has no affordance. Can we get some > ideas for making this look clickable, preferrably ideas that match other > places in Fennec and/or other apps? > > I also think the landscape version will be an L10N problem. Imagine that > text twice as long for some languages. I'd love a icon button with a > long-press hint, like we have in the action bar. Mentioning action bar makes > me wonder if this is better handled in the action bar or an overflow 3-dot > menu. We could create a max with (width of thumbnail view) and wrap the text. I agree making this more button-y would be good. The overflow 3-dot menu is definitely an expected option (that's what Chrome does), but the easy to hit big + button is nice. Although another argument in favor of the 3-dot menu is that we could include both "New Tab" and "New Private Tab", regardless of what tab tray you're in. So you wouldn't need to select the private tabs before creating a new private tab.

(In reply to :Margaret Leibovic from comment #44) > Although another argument in favor of the 3-dot menu is that we could > include both "New Tab" and "New Private Tab", regardless of what tab tray > you're in. So you wouldn't need to select the private tabs before creating a > new private tab. I think you just convinced me back to using this approach. un-needinfo-ing anthony.

New approach! Back to using the menu. I was discussing the UX details with ibarlow on IRC today, so hopefully the behavior is pretty close to what we want. Here's a build: https://dl.dropboxusercontent.com/u/3358452/fennec-close-all.apk

This is feeling pretty good Margaret. Only thing I noticed was that the "close all tabs" control doesn't clear both your normal and private tabs like we discussed. Everything else is looking solid. Oh, I can't remember if we talked about this on IRC, but I might also shorten the animation delay between tabs from 100ms to around 75ms.

(In reply to Ian Barlow (:ibarlow) from comment #47) > This is feeling pretty good Margaret. Only thing I noticed was that the > "close all tabs" control doesn't clear both your normal and private tabs > like we discussed. Everything else is looking solid. Oh, okay, yeah, I think I forgot where we landed on that. > Oh, I can't remember if we talked about this on IRC, but I might also > shorten the animation delay between tabs from 100ms to around 75ms. Cool, I'll try that.

I want to bump this to 33.

Updated to address feedback.

It seems a tad awkward when I have the + next to the opened 3-dot menu cause there's essentially 3 "new tab" options at that point. What're your guys' thoughts on this?

(In reply to Anthony Lam (:antlam) from comment #51) > It seems a tad awkward when I have the + next to the opened 3-dot menu cause > there's essentially 3 "new tab" options at that point. > > What're your guys' thoughts on this? I kinda like still having quick access to creating a new tab, but I agree it is awkward to have 2 different options to do the same thing in almost the same place.

Comment on attachment 8436130 [details] [diff] [review] Add 3-dot menu to tabs panel, including a "close all tabs" option Review of attachment 8436130 [details] [diff] [review]: ----------------------------------------------------------------- Mostly looks good, but we should robustify the tabs tray a bit to prevent weird interactions during the animations. For example: - If I scroll around during the animations, tabs are animated out of order - If I touch any of the tabs right after tapping Close All Tabs, none of the tabs are closed - If I open the menu during the animation, none of the tabs are closed (and the tabs tray stays open) Disabling the menu and all touch events on the TabsTray might be a good way to prevent these. ::: mobile/android/base/tabspanel/TabsPanel.java @@ +34,5 @@ > import android.widget.ImageButton; > import android.widget.LinearLayout; > import android.widget.RelativeLayout; > > + Nit: Drop extra line @@ +223,5 @@ > + } > + return true; > + } > + > + if (itemId == R.id.new_tab || itemId == R.id.new_private_tab) { Since itemId is an int, these conditions might be better as a switch statement. ::: mobile/android/base/tabspanel/TabsTray.java @@ +342,5 @@ > + > + // Delay starting each successive animation to create a cascade effect. > + int cascadeDelay = 0; > + > + for (int i = getChildCount() - 1; i >= 0; i--) { s/getChildCount()/childCount/ @@ +358,5 @@ > + > + animator.addPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener() { > + @Override > + public void onPropertyAnimationStart() { } > + @Override Nit: New line before @Override

(In reply to Brian Nicholson (:bnicholson) from comment #53) > Comment on attachment 8436130 [details] [diff] [review] > Add 3-dot menu to tabs panel, including a "close all tabs" option > > Review of attachment 8436130 [details] [diff] [review]: > ----------------------------------------------------------------- > > Mostly looks good, but we should robustify the tabs tray a bit to prevent > weird interactions during the animations. For example: > - If I scroll around during the animations, tabs are animated out of order > - If I touch any of the tabs right after tapping Close All Tabs, none of the > tabs are closed > - If I open the menu during the animation, none of the tabs are closed (and > the tabs tray stays open) > > Disabling the menu and all touch events on the TabsTray might be a good way > to prevent these. Good catches! Good thing we didn't try to cram this in before the merge :) > @@ +223,5 @@ > > + } > > + return true; > > + } > > + > > + if (itemId == R.id.new_tab || itemId == R.id.new_private_tab) { > > Since itemId is an int, these conditions might be better as a switch > statement. I believe we can't do this because of bug 880118. If you look at our other menu logic, you'll see we need to do comparisons like this.

(In reply to :Margaret Leibovic from comment #54) > > @@ +223,5 @@ > > > + } > > > + return true; > > > + } > > > + > > > + if (itemId == R.id.new_tab || itemId == R.id.new_private_tab) { > > > > Since itemId is an int, these conditions might be better as a switch > > statement. > > I believe we can't do this because of bug 880118. If you look at our other > menu logic, you'll see we need to do comparisons like this. That is correct!

Addressed comments. I think we can address the "Should we get rid of the + button?" question in a follow-up bug.

(In reply to :Margaret Leibovic from comment #52) > (In reply to Anthony Lam (:antlam) from comment #51) > > It seems a tad awkward when I have the + next to the opened 3-dot menu cause > > there's essentially 3 "new tab" options at that point. > > > > What're your guys' thoughts on this? > > I kinda like still having quick access to creating a new tab, but I agree it > is awkward to have 2 different options to do the same thing in almost the > same place. If we want to get clever we can try switching around the menu contents and hiding duplicates depending on what kind of tab you're on, but it feels ... a little too clever perhaps. Worth trying maybe.

Comment on attachment 8437277 [details] [diff] [review] (v2) Add 3-dot menu to tabs panel, including a "close all tabs" option Review of attachment 8437277 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/tabspanel/TabsTray.java @@ +373,5 @@ > + // Hide the panel after the animation is done. > + autoHidePanel(); > + > + // Re-enable the view after the animation is done. > + TabsTray.this.setEnabled(true); I'm worried about possible edge cases where the onPropertyAnimationEnd doesn't get called, keeping the tabs tray disabled. Maybe it would be safer to do the setEnabled call right when the tabs tray is opened? I haven't been able to trigger any such case, though, so I guess we can wait and see if any regressions pop up first.

I had to back this out for (valid!) test failure: https://hg.mozilla.org/integration/fx-team/rev/08a1ee846b54 I forgot to add the menu button to the landscape tablet UI, so the build crashed, but this raises the bigger question: how are we going to do the on the landscape tablet UI? There's not really a good place for the 3-dot menu because the tabs are on the left :(

Talked about it in IRC, but we had plans a couple years back for how a menu might be placed, seems like a good place to start with: http://cl.ly/image/043t2b0k0W1u

(In reply to Ian Barlow (:ibarlow) from comment #61) > Talked about it in IRC, but we had plans a couple years back for how a menu > might be placed, seems like a good place to start with: > http://cl.ly/image/043t2b0k0W1u Sigh, this is annoying because the GeckoMenuPopup is displaying itself under the menu button, not accounting for the fact that there's no space beneath it. This will require some more work...

I added a new resource for the three-dot button in the tabs tray, since on tablets the one in the toolbar is a different color. Also, our MenuPopup extends PopupWindow, which is designed to display above the anchor if there's not enough room below it. However, currently, when we call showAsDropDown, the height of the popup is 0, and that's why it displays below the anchor. To fix this, I manually set a height before calling showAsDropdown. This feels like a bit of a hack, since this will really only work for this case where the menu button is at the bottom of the screen (not a generic solution), but I think that's fine, since this is a menu-specific popup, and we don't often add new menu buttons.

Comment on attachment 8441337 [details] [diff] [review] (Part 2) Add 3-dot menu for tabs tray on tablets Review of attachment 8441337 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good, though the menu looks a bit off to me. In the other places we open a menu, we align it directly under the action bar and flush with the edge of the screen. Could we position it a to a bit more eye-friendly place here? A couple of options I can think of: 1) Keep the menu in the same vertical position, but align the left edge to the left side of the screen. 2) Offset the X and Y positions by the size of the menu button, so that the menu is directly right of the button and flush with the bottom of the screen. Thoughts? ::: mobile/android/base/resources/layout-large-land-v11/tabs_panel_footer.xml @@ +13,5 @@ > android:contentDescription="@string/new_tab" > + android:background="@drawable/action_bar_button_inverse"/> > + > + <View android:layout_width="0dip" > + android:layout_height="fill_parent" s/fill_parent/match_parent/

Updated to center the popup with the anchor, as discussed on IRC. I verified this doesn't affect the behavior of the regular menu popup.

Comment on attachment 8442398 [details] [diff] [review] (Part 2 v2) Add 3-dot menu for tabs tray on tablets Review of attachment 8442398 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/menu/MenuPopup.java @@ +68,5 @@ > + // Set a height, so that the popup will not be displayed below the bottom of the screen. > + setHeight(mPopupMinHeight); > + > + // Attempt to align center the popup with the center of the anchor. If the anchor is > + // near the edge of the screen, the popup will just align with the edge of the screen. Oops, this should read: // Attempt to align the center of the popup with the center of the anchor. If the anchor is // near the edge of the screen, the popup will just align with the edge of the screen.

https://hg.mozilla.org/integration/fx-team/rev/164923fd51c6 https://hg.mozilla.org/integration/fx-team/rev/cb5d611cce01

https://hg.mozilla.org/mozilla-central/rev/164923fd51c6 https://hg.mozilla.org/mozilla-central/rev/cb5d611cce01

Added to the release notes with the wording: "Close all tabs at once"