Looking at https://bugzilla.mozilla.org/attachment.cgi?id=720728 I think we can iterate towards something prettier: rooting the dialog in the back menu, showing some kind of indication of what the list is for (for those poor suckers who accidentally held down the back button and ended up here), styling as something a little prettier than big black text on stark white with radio buttons. We probably have page thumbnails in the DB and cache at this point, for example. We'll need to tackle some kind of clear visual affordance to differentiate between going back to Twitter and going back to twitter.com, for Bug 833607, so marking as blocking.

(In reply to Ian Barlow (:ibarlow) from comment #1) > Created attachment 721668 [details] > mockup of menu! http://i2.kym-cdn.com/photos/images/newsfeed/000/103/740/c2c.png

Christian Vielma is interested in this bug, so I'll assign it to him. Christian, Sriram has experience with custom menus, so he'd be a good person to ask for any Android styling questions. One challenge for this bug is figuring out how we want to make these changes in the context of nsIPromptService (https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIPromptService). We're currently using an Android implementation of this service, and it's very generic. The mockup contains images, different font sizes, special separators, etc., so unfortunately, we'll probably have to reimplement as a native menu in Android. CC'ing Sriram for the menu styling, and Wes in case he has any ideas regarding the prompt service.

(In reply to Richard Newman [:rnewman] from comment #3) > (In reply to Ian Barlow (:ibarlow) from comment #1) > > Created attachment 721668 [details] > > mockup of menu! > > http://i2.kym-cdn.com/photos/images/newsfeed/000/103/740/c2c.png http://i.imgur.com/Gs5BJje.gif

Hi, finally I got the time to start working on this bug. Thank you for the info. I'll get in touch with Sriram in the next days when doubts arise. Any recommendation or extra docs/guidance is welcome!

Do we have an update here?

I'm working on it, but still I don't have any updates. I'll keep you posted.

This is a draft of what I plan to do.

Hello everyone. I've added a draft of the patch I plan to develop but I have some doubts. I think the main change has to be done in the Tab.java class. But the other two classes need to pass a context for Tab to be able to show the menu. Here are my doubts: 1- Should I test the Android version to show the GeckoPopupMenu if SDK>=11 and the current list if <11? 2- In Tab.java how can I access the history and go to a specific item in the history? 3- How can access the last application? (I know this is not specific of Fennec, but I'm not clear how to if there was an intent or application that started Fennec and how to access the Android application stack) 4.- I think that Android's Popup method doesn't support images or anything else than text on each menu item. Am I right? If I am, should we implement it using dialogs to show a small thumbnail of the history item? Thanks!

(In reply to Christian Vielma (IRC :cvielma) from comment #12) > I think the main change has to be done in the Tab.java class. But the other > two classes need to pass a context for Tab to be able to show the menu. I think we want to be moving in the opposite direction: Tab should be becoming *less* coupled, not more. Probably BrowserToolbar should own display of the menu. The 'ideal' way to do that is to have BrowserToolbar implement an interface -- displayHistoryStack(List<Page> pages), perhaps? -- and have Tab call back to it. But I'd like for us to move toward Tab being essentially an immutable blob, so perhaps we can just read the values right out of it, and treat it like a dumb record. > 3- How can access the last application? (I know this is not specific of > Fennec, but I'm not clear how to if there was an intent or application that > started Fennec and how to access the Android application stack) You can use getCallingActivity(), but bear in mind that it won't always work (it only works if the calling app is waiting for a result). Probably this is your best bet: http://developer.android.com/reference/android/app/ActivityManager.html#getRecentTasks%28int,%20int%29

(In reply to Christian Vielma (IRC :cvielma) from comment #12) > > 1- Should I test the Android version to show the GeckoPopupMenu if SDK>=11 > and the current list if <11? Why? Is GeckoPopupMenu tied to SDK v11 somehow? But the layout for the mockup in comment 1 couldn't be created using GeckoPopupMenu without a number of modifications. Rather than trying to hack around our existing custom menu code, it would probably be easiest to just create your own popup dialog/layout from scratch. > 2- In Tab.java how can I access the history and go to a specific item in the > history? The Java front end doesn't have direct access to the history, so this will still have to be done in Gecko. Rather than building a "Prompt:Show" message at http://hg.mozilla.org/mozilla-central/file/1150403342b2/mobile/android/chrome/content/browser.js#l1363, I think we'll instead want to just build a JSON array of page titles. showHistory() in browser.js can then send another message back to Java (Session:HistoryData or something similar), and Java can use this array to build the popup. To go to the specific history index, Java will then have to send another event to Gecko. This event, handled in browser.js, can then just call browser.gotoIndex(index). > 4.- I think that Android's Popup method doesn't support images or anything > else than text on each menu item. Am I right? If I am, should we implement > it using dialogs to show a small thumbnail of the history item? Which popup method are you referring to? But yes, I would implement this using Android's AlertDialog. AlertDialog#setView() can be used to show your custom view.

Thanks for your replies! I have a few more questions based on your responses: (In reply to Richard Newman [:rnewman] from comment #13) > Probably BrowserToolbar should own display of the menu. The 'ideal' way to > do that is to have BrowserToolbar implement an interface -- > displayHistoryStack(List<Page> pages), perhaps? -- and have Tab call back to > it. > > But I'd like for us to move toward Tab being essentially an immutable blob, > so perhaps we can just read the values right out of it, and treat it like a > dumb record. Currently the code that shows the history is in Tab.java. Should I move it to BrowserToolbar.java? Also, why is it recommended to implement the interface? Is there a future intention of implement this interface in other classes? (In reply to Brian Nicholson (:bnicholson) from comment #14) > Why? Is GeckoPopupMenu tied to SDK v11 somehow? I'm sorry, I thought GeckoPopupMenu extended Android's PopupMenu (which requires SDK v11). If I'm going to implement a new PopupMenu or Dialog, I'll have to check the version. > The Java front end doesn't have direct access to the history, so this will > still have to be done in Gecko. Rather than building a "Prompt:Show" message > at > http://hg.mozilla.org/mozilla-central/file/1150403342b2/mobile/android/ > chrome/content/browser.js#l1363, I think we'll instead want to just build a > JSON array of page titles. showHistory() in browser.js can then send another > message back to Java (Session:HistoryData or something similar), and Java > can use this array to build the popup. Can you point me a class or code that do this to get an idea? > > 4.- I think that Android's Popup method doesn't support images or anything > > else than text on each menu item. Am I right? If I am, should we implement > > it using dialogs to show a small thumbnail of the history item? > > Which popup method are you referring to? But yes, I would implement this > using Android's AlertDialog. AlertDialog#setView() can be used to show your > custom view. Well, I referred to Menu#add method. But is ok, I will look for a way to implement it using Dialogs. With the information in browser.js, I know I can get access to the history pages information, but where can I find the thumbnails?

(In reply to Christian Vielma (IRC :cvielma) from comment #15) > > But I'd like for us to move toward Tab being essentially an immutable blob, > > so perhaps we can just read the values right out of it, and treat it like a > > dumb record. > > Currently the code that shows the history is in Tab.java. Should I move it > to BrowserToolbar.java? The current code sends an event. If you're no longer delegating history display to Gecko through a sent message, then yes -- Tab.java is not the place for UI code. (Eventually I want to totally rip out Tab.java, turning it into tab-specific state (almost a token), and shuffle the tab handling logic around in Tabs.java.) > Also, why is it recommended to implement the > interface? Is there a future intention of implement this interface in other > classes? I said "ideal" in quotes, not "recommended" :) There is a decoupling benefit in introducing an interface like this: * It forces you to directly address roles and responsibilities. Rather than having Tab somehow know how to display a menu -- probably through an awful mess of singleton lookups -- you're forced to describe precisely what and how. Tab.java is pretty bad at this -- it has singleton references to GeckoApp, GeckoEvent, GeckoAppShell, Tabs, and the browser DB, with the orchestration of those public methods being the implicit interface surface for "what Tab needs to do stuff". Here lie subtle threading bugs. * It allows you to eliminate singletons, because references are passed around explicitly for callbacks. * It makes testing easier: construct a Tab, pass it a MockHistoryDisplayer, and make sure the right thing happens. My secret evil goal for Fennec is to decouple the display code from state logic as much as possible, so that it's more testable and more comprehensible. > > The Java front end doesn't have direct access to the history, so this will > > still have to be done in Gecko. Rather than building a "Prompt:Show" message > > at > > http://hg.mozilla.org/mozilla-central/file/1150403342b2/mobile/android/ > > chrome/content/browser.js#l1363, I think we'll instead want to just build a > > JSON array of page titles. showHistory() in browser.js can then send another > > message back to Java (Session:HistoryData or something similar), and Java > > can use this array to build the popup. This is an indicator, by the way, that Tab is not the place to be doing this work. It can send an event to Gecko saying "we want to show the back menu", and Gecko will gather data and ask some other part of Fennec to show the menu.

I'd be fine adding an attribute to the PromptService to style the menu differently as well if you want. i.e. we can add a "align" property to the message set to "bottom", or a style property to indicate its a "menu" and not a "dialog"? A bit more work, but you're creating something reusable in other parts of the UI instead of something special case. Longer term, I'd like to make the Prompt service message into more of a well defined API in JS. i.e. something like Android's AlertDialog.Builder: Prompt.create() .setTitle("Title") .setMessage("Message") .setButtons(["Button 1", "Button 2"]) .show();

(In reply to Wesley Johnston (:wesj) from comment #17) Wesley, I'm the newbie so I leave your question to the experts ;) It seemed to me from this thread history that doing it natively in Android would be better. (In reply to Richard Newman [:rnewman] from comment #16) Richard, from this I get the following: - I'll create an interface (say "HistoryDisplayerInterface", with method "ListHistoryStack" as said before). - I'll create a view based on Android's Dialog that is able to show the history (say "HistoryDisplayView") - Make BrowserToolbar implement "HistoryDisplayerInterface". - From Tab.java, call Gecko to obtain all the information about the pages to display (title, url, and thumbnail I think), and use it to call BrowserToolbar.ListHistoryStack that will show the history. - Optionally: create a container class, say "Page" that can hold in Java, the information from the items to be displayed in the HistoryDisplayView (title, url, thumbnail, current). Is that it? If so, I have a few questions just about the details (code organization, class names, design conventions, and other two that I made before: where to obtain the thumbnails and sample code to send info from Gecko or browser.js). Thanks!

Hi folks, we went back and had a look at this design and wanted to add in a couple of updates while this is still being worked on. The changes are all visual design based, none of the interactions should be affected. Changes: * Remove checkmark, as it is not really an appropriate use of that icon. * Add favicons * Add a "timeline" visual treatment that connects all the favicons in the list together, and communicates the idea of history more clearly * Use a thicker favicon border and bold text to indicate the current page

Perfect, Ian. I haven't been working on this :S I'm delayed. I'll be two weeks out of town so I'll continue with this in a couple of weeks. In comment #17 I showed the idea to implement this, can someone confirm this?

Friendly ping to see if someone can keep this work moving

Hi, I've been unable to work on this. If anybody want to take it please go ahead. If not, I might start working on it again not sooner than a month. I also have to learn a bit more about Android's UI in order to make some progress.

Unassigning; grab it again if nobody else does by the time you're back!

Non-trivial good first bug seems like an oxymoron :)

Hi, I would like to take a shot at this.

It's been over a year since these mockups were created. Are there any changes UX wants to make before we get started?

I'd like to keep this bug focused on the styling. Let's not add new functionality in here. We can do that in a new bug. By that I mean we should ignore the part of the mockup for "Menu behaviour when Firefox was entered from another app". Just focus on the "Normal menu behaviour" mockup.

(In reply to Brian Nicholson (:bnicholson) from comment #26) > It's been over a year since these mockups were created. Are there any > changes UX wants to make before we get started? I think we can start with what Ibarlow left in comment 19 - but let's keep the grey consistent with our new toolbar refinements and go with #EBEBF0

(In reply to vivek from comment #25) > Hi, > I would like to take a shot at this. Hi Vivek, Currently, BrowserApp#onKeyLongPress calls showAllHistory [1], which ends up sending a Session:ShowHistory event to Gecko [2]. The prompt service is then used to build the prompt at [3]. Since the prompt service is used to show the menu now, it can't easily be styled like the mockup. We'll probably want to replace the existing implementation with a new custom dialog view. Instead of invoking the prompt service at [3], we can send back a message containing the list of history entries to show, which can be used to populate the new custom view. Let me know if you have any questions! [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java?rev=c092f9e4310d#539 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/Tab.java?rev=c092f9e4310d#569 [3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=5e8369ca0489#1831

^ if you're looking for the 9 patch files for the menu background they can be found over in bug 1055536

Can we help move this along?

Sorry, I was unable to spend time on this. I'll take this up on priority.

A Partial patch that has following items still pending * Pop styling * Time line * handle item click

An updated partial patch that handles history navigation Pending tasks: * time line * View styling

Comment on attachment 8495589 [details] [diff] [review] Partial patch V2 Review of attachment 8495589 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good. I think this should be close to landing after you update the list style. ::: mobile/android/base/BrowserApp.java @@ +3195,5 @@ > + int historySize = tab.getHistorySize(); > + > + switch(action) { > + case BACK: > + if (!tab.canDoBack()) Nit: I know you just copied this code from Tab, but since you're touching it, please add braces around if statements (here and elsewhere). Our convention now is to use braces for all if statements, including single-line ones. @@ +3264,5 @@ > + historyPageList.add(new TabHistoryPage(title, url, selected)); > + } > + > + // Populate the alert dialog with the historic page items. > + final ArrayAdapter<TabHistoryPage> urlAdapter = new TabHistoryAdapter(getActivity(), historyPageList); Everything here and below needs to be done on the UI Thread since adapters and dialogs are UI. To clean up this method a bit, I'd recommend creating a separate helper method that takes a historyPageList argument, then call it from here like this: runOnUiThread(new Runnable() { @Override public void run() { showHistory(historyPageList); } }); Also, just to be explicit, please add a "ThreadUtils.assertOnUiThread()" as the first line of this helper. @@ +3266,5 @@ > + > + // Populate the alert dialog with the historic page items. > + final ArrayAdapter<TabHistoryPage> urlAdapter = new TabHistoryAdapter(getActivity(), historyPageList); > + final AlertDialog dialog = new AlertDialog.Builder(getActivity()) > + .setAdapter(urlAdapter, new Dialog.OnClickListener() { Overflow lines are normally indented, but since that would mean further indenting everything below, you can just prevent the overflow here by calling setAdapter on the dialog variable: final AlertDialog dialog = ...; dialog.setAdapter(...); @@ +3271,5 @@ > + @Override > + public void onClick(DialogInterface dialog, int which) { > + final JSONObject json = new JSONObject(); > + try { > + json.put("index", toIndex - which); If this message is only going to have one key, you can just pass the index directly below in "Session:Navigate" instead of creating this JSON object. @@ +3291,5 @@ > + dialog.show(); > + } > + > + @Override > + public void onError() { You can just drop this override and let the default handler throw if anything happens. ::: mobile/android/base/TabHistoryAdapter.java @@ +10,5 @@ > +import android.widget.ArrayAdapter; > + > +public class TabHistoryAdapter extends ArrayAdapter<TabHistoryPage> { > + private List<TabHistoryPage> pages; > + private Context context; Both of these can be final. @@ +20,5 @@ > + } > + > + @Override > + public View getView(int position, View convertView, ViewGroup parent) { > + TabHistoryItemRow row; You can restructure this block to simplify it a bit: TabHistoryItemRow row = (TabHistoryItemRow) convertView; if (row == null) { row = new TabHistoryItemRow(context, null); } ::: mobile/android/base/TabHistoryItemRow.java @@ +16,5 @@ > + > +public class TabHistoryItemRow extends RelativeLayout { > + private final FaviconView mFavicon; > + private final TextView mTitle; > + private int mLoadFaviconJobId = Favicons.NOT_LOADING; For new code, we've chosen not to use any prefixes for class members. Please remove the "m" prefixes (mFavicon -> favicon, etc). @@ +56,5 @@ > + } > + } > + > + // Listener for handling Favicon loads. > + private final OnFaviconLoadedListener mFaviconListener; Move this to the top of the class with the other variable declarations. ::: mobile/android/chrome/content/browser.js @@ +343,5 @@ > Services.obs.addObserver(this, "Tab:Load", false); > Services.obs.addObserver(this, "Tab:Selected", false); > Services.obs.addObserver(this, "Tab:Closed", false); > Services.obs.addObserver(this, "Session:Back", false); > + Messaging.addListener(this.showHistory.bind(this), "Session:ShowHistory"); Nit: Move this after all of the Services.obs.addObserver calls. Also, Gecko is no longer showing the history, but instead responding to a request for the history. "Session:GetHistory" would be a more appropriate name. @@ +1525,5 @@ > break; > > + case "Session:Navigate": > + let data = JSON.parse(aData); > + let index = data.index; You should just be able to do "let index = JSON.parse(aData)" directly if you remove the object as suggested above. @@ +1839,5 @@ > this._prefObservers = newPrefObservers; > }, > > // This method will print a list from fromIndex to toIndex, optionally > // selecting selIndex(if fromIndex<=selIndex<=toIndex) Please update the comments describing the new behavior (this will return the list, but not show it). @@ +1840,5 @@ > }, > > // This method will print a list from fromIndex to toIndex, optionally > // selecting selIndex(if fromIndex<=selIndex<=toIndex) > + showHistory: function(data) { showHistory -> getHistory @@ +1857,5 @@ > }; > listitems.push(item); > } > > + return { "urls" : listitems }; Nit: Rename "urls" -> "historyItems" since we're sending more than just URLs.

(In reply to Brian Nicholson (:bnicholson) from comment #35) > This is looking good. I think this should be close to landing after you > update the list style. Which reminds me -- how should this look on tablets? At minimum, I assume we'll want the list to be shifted to the top where the buttons are instead of anchored at the bottom. Should the design be mostly the same for tablets with some width added constraints?

Related question: In addition to the back/forward buttons in the tablet toolbar bar, tablets still have the hardware back button at the bottom. Do we want to shift the list's position based on which back button was pressed?

(In reply to Brian Nicholson (:bnicholson) from comment #36) > Which reminds me -- how should this look on tablets? At minimum, I assume > we'll want the list to be shifted to the top where the buttons are instead > of anchored at the bottom. Should the design be mostly the same for tablets > with some width added constraints? I think that's a good starting point. In my mind right now, this would be triggered via a long press, then *shake*, and menu pops up. (In reply to Brian Nicholson (:bnicholson) from comment #37) > Related question: In addition to the back/forward buttons in the tablet > toolbar bar, tablets still have the hardware back button at the bottom. Do > we want to shift the list's position based on which back button was pressed? I'd like to avoid that if possible just because two places for the same information that are so easily accessible at any time seems kind of confusing. Especially if one is organized top > down, while the other might be organized from the bottom > top. Ideally I'd like to keep it both anchored to the back button in the top (under the back button) for now. Maybe we could get a build to test this?

A new partial patch with Item list updates. Items pending: * Remove default view padding with alert dialog

UI Mock for the patch

Comment on attachment 8498502 [details] mock.png Could I get an .apk to test this? The magnifying glass looks too big there. But I'd like to get an entire build to test it out more thoroughly. Also, how come Google is bolder and highlighted with a fuller stroke here? + for a good start in the right direction!

^ quick nit picky thing, the font color looks like it might be too light. Could we set it at #363B40 in the build too? :)

(In reply to Anthony Lam (:antlam) from comment #41) > The magnifying glass looks too big there. But I'd like to get an entire > build to test it out more thoroughly. Might be a problem with the PNG asset itself. I saw a bug about the favicon being to big. > Also, how come Google is bolder and highlighted with a fuller stroke here? Looks like Google is the "current" page in the history session, so it's shown bold because it's where you currently are. > + for a good start in the right direction! Agreed. Looks great.

@antlam, Please download the .apk file from https://drive.google.com/file/d/0B34Nogao7VfpNUR4eUZ4WHRPaEE/edit?usp=sharing The apk build has the font color changes

font color updated in this patch

Hey Vivek, I'm taking a look at this more extensively and trying to pick up Ian's older designs. Stay tuned!

Comment on attachment 8498935 [details] [diff] [review] Partial patch V3.1 Review of attachment 8498935 [details] [diff] [review]: ----------------------------------------------------------------- Very nice, this is looking excellent. Just a few comments below -- thanks for all the hard work! Would you mind posting a screenshot or a build so that Anthony can try this out and give some UX feedback? ::: mobile/android/base/BrowserApp.java @@ +3205,5 @@ > appLocale, > previousSession); > } > + > + public boolean showTabHistory(Tab tab, HistoryAction action) { The code in BrowserApp is starting to grow pretty large. Perhaps this code would be best in its own separate TabHistoryController.java. Then it would look like this: TabHistoryController mTabHistoryController = new TabHistoryController(context); ... mBrowserToolbar.setTabHistoryController(mTabHistoryController); BrowserApp and BrowserToolbar would then call showTabHistory() on the controller. @@ +3294,5 @@ > + > + return true; > + } > + > + Nit: remove extra line ::: mobile/android/base/TabHistoryItemRow.java @@ +25,5 @@ > + private final OnFaviconLoadedListener faviconListener; > + > + private int loadFaviconJobId = Favicons.NOT_LOADING; > + > + Nit: remove extra line @@ +81,5 @@ > + title.setTypeface(null, Typeface.NORMAL); > + } > + > + if (isFirstElement) { > + timeLineTop.setVisibility(INVISIBLE); Rather than setting the visibility to VISIBLE at the top of this method and then changing it here, I would do this in one line: timeLineTop.setVisibility(isFirstElement ? View.INVISIBLE : View.VISIBLE); @@ +85,5 @@ > + timeLineTop.setVisibility(INVISIBLE); > + } > + > + if (isLastElement) { > + timeLineBottom.setVisibility(INVISIBLE); Same here. ::: mobile/android/base/widget/FaviconView.java @@ +57,5 @@ > // Size of the background rectangle. > private final RectF mBackgroundRect; > > + // Type of the border whose value is defined in attrs.xml . > + private final boolean isDomaintBorderEnabled; Domaint -> Dominant

(In reply to Brian Nicholson (:bnicholson) from comment #47) > Would you mind posting a screenshot or a build so that Anthony can try this > out and give some UX feedback? Oops, I wasn't paying attention. Looks like you did this already!

A new patch with code refractored to a separate controller. DialogFragment used to handle activity lifecycle callbacks

Implemented custom layout to support full width tab history. Some Minor details yet to iron out * Back button doesn't clear the fragment. * Tab history stays open on switching to new tab Mocks https://drive.google.com/file/d/0B34Nogao7VfpNUs3SmZnNV81LUk/view?usp=sharing https://drive.google.com/file/d/0B34Nogao7VfpcnRzbGE4czZja2c/view?usp=sharing

Hey, Vivek! How have you been? :) I just want to note that :antlam will be on PTO until some time next week - you may not hear from him until then!

One thing that jumps out at me with the new mocks is the lack of a "shadowed" edge. The fragment seems to be part of the web content, not floating over it.

Comment on attachment 8500725 [details] [diff] [review] Partial patch ver 5 Review of attachment 8500725 [details] [diff] [review]: ----------------------------------------------------------------- Lots of new Tab* files here. Can you move these to the org.mozilla.gecko.tabs package (mobile/android/base/tabs) from the top level? (In reply to Mark Finkle (:mfinkle) from comment #52) > One thing that jumps out at me with the new mocks is the lack of a > "shadowed" edge. The fragment seems to be part of the web content, not > floating over it. Agreed. Ian's mockup has this fairly thick border at the top that's a bit darker than the background color. Vivek, just in case you didn't notice this already, could you update the layouts to include that border? Something else that sticks out is the oversized search icon. At minimum, we'll probably want to shrink it so it matches the other icons in the view, though we may end up wanting another icon entirely. The search icon is good on about:home since it's in the top input box, but it doesn't make as much sense for this list. ::: mobile/android/base/TabHistoryContainerPanel.java @@ +8,5 @@ > +import android.util.AttributeSet; > +import android.view.View; > +import android.widget.RelativeLayout; > + > +public class TabHistoryContainerPanel extends RelativeLayout implements TabHistoryPanelListener { Rather than creating a separate class for this, I would create an OnShowTabHistory listener inside of TabHistoryController. BrowserApp can then initialize this listener with the fragment code below, then pass it to the TabHistoryController constructor. ::: mobile/android/base/TabHistoryController.java @@ +101,5 @@ > + final boolean selected = obj.getBoolean("selected"); > + historyPageList.add(new TabHistoryPage(title, url, selected)); > + } > + > + activity.runOnUiThread(new Runnable() { With the interface changes mentioned above, you can simply do mShowHistoryListener.onShowHistory(historyPageList, toIndex). That way, TabHistoryController doesn't even need a reference to the Activity, meaning it doesn't touch UI at all, so it remains a true "controller". @@ +113,5 @@ > + > + return true; > + } > + > + private void showHistory(final List<TabHistoryPage> historyPageList, final int toIndex) { And then you can remove this method. ::: mobile/android/base/TabHistoryDialogFragment.java @@ +39,5 @@ > + > + @Override > + public void onActivityCreated(Bundle savedInstanceState) { > + super.onActivityCreated(savedInstanceState); > + final ArrayAdapter<TabHistoryPage> urlAdapter = new TabHistoryAdapter(getActivity(), historyPageList); TabHistoryAdapter is only used internally here, so it can stay in this file. Please move TabHistoryAdapter to the bottom of this file as a private static class. ::: mobile/android/base/TabHistoryPanelListener.java @@ +1,3 @@ > +package org.mozilla.gecko; > + > +public interface TabHistoryPanelListener { Please rename this to OnHistoryDismissListener and move it inside of TabHistoryDialogFragment. ::: mobile/android/base/widget/FaviconView.java @@ +57,5 @@ > // Size of the background rectangle. > private final RectF mBackgroundRect; > > + // Type of the border whose value is defined in attrs.xml . > + private final boolean isDDominantBorderEnabled; DD -> D

A new patch that addresses the review comments. Mock can be found at https://drive.google.com/file/d/0B34Nogao7VfpeUJkYzQzbW03M28/view

Comment on attachment 8502639 [details] [diff] [review] Patch ver 6 Review of attachment 8502639 [details] [diff] [review]: ----------------------------------------------------------------- Hi Vivek, I've been busy recently, so I'm sorry for taking so long this time around. I think there's still a bit of work that needs to be done for proper Fragment use, as I've commented below. Aside from these refactoring changes, I think we'll also need a couple other fixes before this can land: * Having the back button dismiss the fragment instead of going back in history. * Reducing the size of the home and generic icons. By generic icon, I mean the one that appears for pages with no favicons (e.g., go to http://foo.bar and notice how the globe icon is also too big). Thanks again for spending so much time on this. ::: mobile/android/base/BrowserApp.java @@ +553,5 @@ > mProgressView = (ToolbarProgressView) findViewById(R.id.progress); > mBrowserToolbar.setProgressBar(mProgressView); > > + // Initialize Tab History Controller. > + tabHistoryController = new TabHistoryController(this); Sorry, I wasn't very clear with my comments last time. Where you initialize tabHistoryController above, I would do this: tabHistoryController = new TabHistoryController(new OnShowTabHistory() { @Override public void onShowHistory(final ArrayList<TabHistoryPage> historyPageList, final int toIndex) { runOnUiThread(new Runnable() { @Override public void run() { tabHistoryContainer.setVisibility(View.VISIBLE); final TabHistoryDialogFragment fragment = TabHistoryDialogFragment.newInstance(historyPageList, toIndex); final FragmentManager fragmentManager = getSupportFragmentManager(); fragmentManager.popBackStackImmediate(null, FragmentManager.POP_BACK_STACK_INCLUSIVE); fragmentManager.beginTransaction() .add(R.id.tab_history_panel, fragment, TAB_HISTORY_FRAGMENT_TAG) .commitAllowingStateLoss(); } }); } }); That is, have the activity own the fragment creation. Then you don't need a special TabHistoryContainerPanel class (tabHistoryContainer can just be a regular RelativeLayout). @@ +3000,5 @@ > Tab tab = Tabs.getInstance().getSelectedTab(); > if (tab != null) { > + // Hide History if shown already. > + tabHistoryContainer.clearHistory(); > + return tabHistoryController.showTabHistory(tab, TabHistoryController.HistoryAction.ALL); I don't like having to handle this in two separate places -- hopefully we can figure out a way for this to be contained entirely in tabHistoryController and the fragment. ::: mobile/android/base/tabs/TabHistoryContainerPanel.java @@ +12,5 @@ > +import android.util.AttributeSet; > +import android.view.View; > +import android.widget.RelativeLayout; > + > +public class TabHistoryContainerPanel extends RelativeLayout implements OnHistoryDismissListener { We shouldn't need this class at all -- please replace it with a regular RelativeLayout in gecko_app.xml (see other comments below). ::: mobile/android/base/tabs/TabHistoryDialogFragment.java @@ +23,5 @@ > + > +public class TabHistoryDialogFragment extends DialogFragment implements OnItemClickListener, OnClickListener { > + private final List<TabHistoryPage> historyPageList; > + private final int toIndex; > + private final OnHistoryDismissListener panelListener; panelListener -> dismissListener @@ +28,5 @@ > + > + private ListView dialogList; > + > + public interface OnHistoryDismissListener { > + void onHidePanel(); onHidePanel -> onHistoryDismiss @@ +32,5 @@ > + void onHidePanel(); > + } > + > + public TabHistoryDialogFragment(OnHistoryDismissListener panelListener, List<TabHistoryPage> historyPageList, > + int toIndex) { Apologize for missing this the first time around, but Fragments should not take args in their constructors. Instead, they need a static newInstance() getter that constructs the Fragment and initializes it with a Bundle. So this constructor should be replaced with this: public static TabHistoryDialogFragment newInstance(ArrayList<TabHistoryPage> historyPageList, int toIndex) { final TabHistoryDialogFragment fragment = new TabHistoryDialogFragment(); final Bundle args = new Bundle(); args.putParcelableArrayList(ARG_LIST, historyPageList); args.putInt(ARG_INDEX, toIndex); fragment.setArguments(args); return fragment; } and then you'll need to load these args in onCreate: public void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); final Bundle args = getArguments(); historyPageList = args.<TabHistoryPage>getParcelableArrayList(ARG_LIST); toIndex = args.getInt(ARG_INDEX); } Now, the slight complication here is that TabHistoryPage isn't Parcelable. But that's easy to fix if you add these to TabHistoryPage: public void writeToParcel(Parcel dest, int flags) { dest.writeString(title); dest.writeString(url); dest.writeInt(selected ? 1 : 0); } public static final Parcelable.Creator<TabHistoryPage> CREATOR = new Parcelable.Creator<TabHistoryPage>() { @Override public TabHistoryPage createFromParcel(Parcel source) { return new TabHistoryPage(source); } @Override public TabHistoryPage[] newArray(int size) { return new TabHistoryPage[size]; } }; You'll also need a TabHistoryPage constructor that accepts a source Parcel, then initializes the page from that Parcel. @@ +70,5 @@ > + > + @Override > + public void onDismiss(DialogInterface dialog) { > + super.onDismiss(dialog); > + panelListener.onHidePanel(); I'm not even sure we need to expose this listener at all. Note that we get the ViewGroup container in onCreateView -- how about we hold onto that and then use it directly?

Hey Vivek, Just got back (as Michael pointed out)! To help move this along, I'm going to take a quick clean up pass at the old mocks that we have in comment 19. Note: Not sure about the white overlay over the website right now, just had that there because it was a bit distracting when I was designing. We could ignore that for now. Hope these specs help! The favicon size should be 32dp square, and the strokes are #D7D9DB. There's also a 2 dp line along the top of this menu that acts as a "shadow" that is #DADADF. Let me know if you got any questions!

Comment on attachment 8502639 [details] [diff] [review] Patch ver 6 + for progress! :) Is there a build I can test somewhere? I'd like to test the interactions and vibrations. Namely, I'd like to see if we could get it to: 1) Vibrate when the user initiates the long press of the back button (already does) 2) Vibrate when this menu pops up 3) Allow users to drag up through this list of items without lifting their same finger (right now I think it triggers the swipe up intent on Nexus) I realize 3 might be a bigger ask so we might want to file a follow up bug for that? Thoughts?

Patch with following changes: * colors updated as per antlam's comment * Favicon size reduced for generic icons. (@bnicholson : The problem was not with padding but was with the scaletype. I've introduced an attribute to disable the scaletpye which overridden in constructor. This way it doesn't break the FaviconView used for ReadingList bookmark etc) * code refractor as per bnicholson comment. Things to do: * Restrict the height of the dialog to 1/2 screen height at max when the history list is lengthy. @antlam: Mocks and APK file are available at https://drive.google.com/folderview?id=0B34Nogao7VfpVGJTVi1XNi1HNkE&usp=sharing

Comment on attachment 8507296 [details] [diff] [review] Patch ver 7 I'm getting downloading issues with the .APK file. Could you attach that here instead?

^nevermind - works now! Some feedback about the visuals: 1) The background color needs to align with the spec: #EBEBF0 2) The padding on top and bottom of each list item needs to match the spec: 14dp upper and lower 3) Padding to the right and left of the icon should be 15dp 4) ^ The above should make each list item 60dp tall 5) Let's make the "timeline" visual a bit thicker. I have it as 3 dp in my designs :) 6) I forgot to mention this in the spec but could we get text to be padded 15 dp away from the right hand side as well? (I'll attach a mock) and let's keep it consistent and use fading to cover text overflow (if needed) 7) Could we get a second vibrate when the menu pops up?

Adding the spacing spec for the right hand side to handle text overflow

(In reply to Anthony Lam (:antlam) from comment #60) > 7) Could we get a second vibrate when the menu pops up? Do any other apps do this? The vibration on back press is automatically handled by the OS. We could add a manual vibration (which would require detecting the phone's haptic feedback setting and setting a specific vibration time), but this seems like something best left to the device.

(In reply to Brian Nicholson (:bnicholson) from comment #62) > (In reply to Anthony Lam (:antlam) from comment #60) > > 7) Could we get a second vibrate when the menu pops up? > > Do any other apps do this? The vibration on back press is automatically > handled by the OS. We could add a manual vibration (which would require > detecting the phone's haptic feedback setting and setting a specific > vibration time), but this seems like something best left to the device. Yeah, for the first action it makes sense. But usually if it's a long-press interaction, there's physical feedback that comes when you've "pressed long enough" basically. Essentially what I'm suggesting is what you get when you long press on a link right now in Android. Not sure how hard that might be though :)

(In reply to Anthony Lam (:antlam) from comment #63) > Essentially what I'm suggesting is what you get when you long press on a > link right now in Android. Not sure how hard that might be though :) Yeah, that should definitely be possible, though probably not something else we want to tack onto this bug. I filed bug 1087673 as a mentor bug to handle this.

(In reply to Brian Nicholson (:bnicholson) from comment #64) > (In reply to Anthony Lam (:antlam) from comment #63) > > Essentially what I'm suggesting is what you get when you long press on a > > link right now in Android. Not sure how hard that might be though :) > > Yeah, that should definitely be possible, though probably not something else > we want to tack onto this bug. I filed bug 1087673 as a mentor bug to handle > this. Thanks Brian!

A new patch that address review comment from antlam Further, from discussion over irc with antlam following are done: * list selection color * Fading edge The mocks and apk are available at https://www.dropbox.com/sh/az1sp9jsjqgsten/AACdlbgXgrsAHUa5WQ4KboHza?dl=0

Comment on attachment 8512870 [details] [diff] [review] Patch ver 8 Review of attachment 8512870 [details] [diff] [review]: ----------------------------------------------------------------- Looks good for this bug, Vivek! Thanks for doing this.

Comment on attachment 8512870 [details] [diff] [review] Patch ver 8 Review of attachment 8512870 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Just a few (minor) questions and things to clean up, and this will be ready to land. ::: mobile/android/base/BrowserApp.java @@ +480,5 @@ > + public void run() { > + final TabHistoryDialogFragment fragment = TabHistoryDialogFragment.newInstance(historyPageList, > + toIndex); > + final FragmentManager fragmentManager = getSupportFragmentManager(); > + fragmentManager.popBackStackImmediate(TabHistoryDialogFragment.TAB_HISTORY_FRAGMENT_TAG, Can you put this inside the same transaction you use below? That is: fragmentManager.beginTransaction() .popBackStack(...) .add(...) ... @@ +686,5 @@ > > > @Override > public void onBackPressed() { > + Nit: Remove empty space @@ +697,5 @@ > + // Remove the tab history fragment still on the back stack even though the container is no longer visible. > + // This case occurs when the fragment dialog was dismissed by click event. > + Fragment frag = getSupportFragmentManager().findFragmentByTag(TabHistoryDialogFragment.TAB_HISTORY_FRAGMENT_TAG); > + if (frag != null) { > + super.onBackPressed(); I don't quite understand your comment about why this is needed. Some questions: * What happens if you don't call super.onBackPressed() here? * Can you give me an exact set of steps to follow where this case occurs? ::: mobile/android/base/resources/layout/tab_history_item_row.xml @@ +4,5 @@ > + - file, You can obtain one at http://mozilla.org/MPL/2.0/. --> > + > +<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" > + xmlns:gecko= "http://schemas.android.com/apk/res-auto" > + android:layout_width="fill_parent" Nit: Align the android: tags at the same indentation level as the xmlns: tags. @@ +8,5 @@ > + android:layout_width="fill_parent" > + android:layout_height="wrap_content" > + android:background="@drawable/action_bar_button" > > + > + <RelativeLayout android:id="@+id/tab_history_timeline_combo" I think you should be able to remove this RelativeLayout... @@ +41,5 @@ > + </RelativeLayout> > + > + <org.mozilla.gecko.widget.FadedTextView > + android:id="@+id/tab_history_title" > + android:layout_toRightOf="@id/tab_history_timeline_combo" ...and use layout_toRightOf="@id/tab_history_icon" instead here. ::: mobile/android/base/resources/layout/tab_history_layout.xml @@ +2,5 @@ > +<RelativeLayout > + xmlns:android="http://schemas.android.com/apk/res/android" > + android:layout_width="match_parent" > + android:layout_height="match_parent" > + android:clickable="true" > Why does this RelativeLayout need to have clickable="true"? @@ +10,5 @@ > + android:layout_height="wrap_content" > + android:layout_alignParentBottom="true" > + android:background="@drawable/tab_history_bg" > + android:divider="@null" /> > +</RelativeLayout> \ No newline at end of file Nit: newline above this tag ::: mobile/android/base/resources/values/attrs.xml @@ +182,5 @@ > <attr name="floatingHintEditTextStyle" format="reference" /> > </declare-styleable> > > + <declare-styleable name="FaviconView"> > + <attr name="domaintBorderEnabled" format="boolean" /> domaint -> dominant ::: mobile/android/base/resources/values/colors.xml @@ +142,5 @@ > <color name="toast_button_text">#FFFFFFFF</color> > > + <!-- Tab History colors. --> > + <color name="tab_history_background">#EBEBF0</color> > + <color name="tab_history_timeline_seperator">#D7D9DB</color> seperator -> separator ::: mobile/android/base/tabs/TabHistoryDialogFragment.java @@ +23,5 @@ > + > +public class TabHistoryDialogFragment extends DialogFragment implements OnItemClickListener, OnClickListener { > + private static final String ARG_LIST = "historyPageList"; > + private static final String ARG_INDEX = "index"; > + public static final String TAB_HISTORY_FRAGMENT_TAG = "tabHistoryFragment"; This tag should actually be part of BrowserApp instead of the fragment since fragments don't own their tags (it would be possible for someone else to create this fragment using a different tag, for example). @@ +30,5 @@ > + private int toIndex; > + private ViewGroup parent; > + private ListView dialogList; > + > + public TabHistoryDialogFragment() { Change this constructor to private to prevent people from doing "new TabHistoryDialogFragment()" accidentally. ::: mobile/android/base/tabs/TabHistoryItemRow.java @@ +39,5 @@ > + } > + > + // Only holds a reference to the FaviconView itself, so if the row gets > + // discarded while a task is outstanding, we'll leak less memory. > + private static class UpdateViewFaviconLoadedListener implements OnFaviconLoadedListener { Nit: Move this class to the bottom of the file, after all methods. @@ +68,5 @@ > + } > + > + // Update the views with historic page detail. > + public void update(final TabHistoryPage historyPage, boolean isFirstElement, boolean isLastElement) { > + timeLineTop.setVisibility(isFirstElement ? View.INVISIBLE : View.VISIBLE); setVisibility/setText/etc are all methods on View, which means this method must be called on the UI thread. Please add ThreadUtils.assertOnUiThread() as the first line in this method. @@ +82,5 @@ > + title.setTypeface(null, Typeface.NORMAL); > + } > + > + favicon.setEnabled(historyPage.isSelected()); > + Nit: remove newline @@ +87,5 @@ > + favicon.clearImage(); > + Favicons.cancelFaviconLoad(loadFaviconJobId); > + > + // Favicon needs to be loaded from main thread. > + ThreadUtils.postToUiThread(new Runnable() { You shouldn't need to post a Runnable here -- update() is called from the adapter getView(), which is already on the UI thread.

A couple more fixes: (In reply to Brian Nicholson (:bnicholson) from comment #68) > @@ +10,5 @@ > > + android:layout_height="wrap_content" > > + android:layout_alignParentBottom="true" > > + android:background="@drawable/tab_history_bg" > > + android:divider="@null" /> > > +</RelativeLayout> > \ No newline at end of file Well that's strange...I'm not sure how this became part of my comment. But this is one more thing that should be fixed: please make sure all files end in a new line! That is, looking at the raw patch file, you don't want to see "\ No newline at end of file" anywhere. Also, please make sure all new files include the Mozilla license header. Several new files (tab_history_bg.xml, tab_history_layout.xml, TabHistoryController.java, etc.) are missing it.

New patch with review comments corrected. Patch also includes: * changes to list item row layout * Fragment Transaction backstack handling. The APk files are uploaded at https://www.dropbox.com/s/xfuqk8n2ye7zkiv/fennec-36.0a1.en-US.android-arm.apk?dl=0

^ Visual and UX side seems under control here :) again, great work Vivek

A new patch with cleaner the tab fragment logic @Antlam: The apk are available at https://www.dropbox.com/s/xfuqk8n2ye7zkiv/fennec-36.0a1.en-US.android-arm.apk?dl=0

Comment on attachment 8516896 [details] [diff] [review] Tab History Patch Review of attachment 8516896 [details] [diff] [review]: ----------------------------------------------------------------- Looks great -- thanks for all the hard work! I'm looking forward to this feature :) To get this checked in, please do the following: 1) Fix the newline comment mentioned below. 2) Update the patch with a description with r=bnicholson at the end (e.g., "Bug 847435 - Redesign tab history menu. r=bnicholson"). 3) Do an Android try push with this patch. Once the try push is green, we can set the "checkin-needed" flag in the bug so a tree sheriff can check it in. I think you've contributed before and are familiar with the steps above, but please let me know if anything is unclear! ::: mobile/android/base/BrowserApp.java @@ +3288,5 @@ > osLocale, > appLocale, > previousSession); > } > +} \ No newline at end of file > -} > +} > \ No newline at end of file Please make sure you add the newline back to the end of BrowserApp.java.

A new patch re-based against tree with nits corrected. The try server run logs are available at: https://tbpl.mozilla.org/?tree=Try&rev=7be394027ef3

Comment on attachment 8517565 [details] [diff] [review] Show Tab History Patch Review of attachment 8517565 [details] [diff] [review]: ----------------------------------------------------------------- I see BrowserApp.java is still showing the last line as modified, so I just went ahead and fixed that and landed. Thanks for all the great work, and I'll be happy to help with the follow-ups! https://hg.mozilla.org/integration/fx-team/rev/086bd7b12b92