Spinoff from bug 626500 comment 9. A WIP patch can be found in attachment 504593 [details] [diff] [review].

We would very much like to have this.

Comment on attachment 505573 [details] [diff] [review] patch v1 Looks good to me, but it's outside my realm; can you please review, Dao?

Passed try.

Will this change the position of the panorama button if the user has added it from the toolbar customization palette? If so, I think we probably just want a pref to control if the button is displayed or not. Slightly more obscure consideration: if every user places the button in a different location, we're going to have some difficulty transitioning them all over to the correct location for the button in Firefox 5 (not that we can't still do that).

(In reply to comment #5) > Will this change the position of the panorama button if the user has added it > from the toolbar customization palette? If so, I think we probably just want a > pref to control if the button is displayed or not. This button will not be inserted if the button already exists in the target toolbar. We should also check that it does not exist in any other toolbar. > Slightly more obscure consideration: if every user places the button in a > different location, we're going to have some difficulty transitioning them all > over to the correct location for the button in Firefox 5 (not that we can't > still do that). We would have to search for the button in every toolbar and remove it if it's there. This should not be too difficult if we decide to do that.

The patch does now check if the toolbar button is already included in any of the available toolbars.

Passed try (but hit bug 626956).

Since I can't see any screenshot or description of where the button shows up here: We should make sure it doesn't show up in the same position as the List All Tabs button does right now, to not break people's habits. In other words, the rightmost (in LTR) button should be List All Tabs at all times. If the Panorama button is added, it should be to the left of List All Tabs. And I'd like to say that I presonally dislike that we add it automatically — just because a user visits Panorama once to see what it's for, doesn't mean (s)he necessarily want the button there afterwards. I'd rather we just leave it in the customization palette for the people that want it — or have something you can click in the Panorama screen itself to add the button. But I realize it's probably way too late to do something like that, so let's get this in, and then I can file a follow-up if needed. :)

(In reply to comment #9) > And I'd like to say that I presonally dislike that we add it automatically — > just because a user visits Panorama once to see what it's for, doesn't mean > (s)he necessarily want the button there afterwards. More precisely, what I'm worried about is that opening Panorama once doesn't mean you're going to use it (let alone that you want the button on the tab bar). You need to open it before you can even make that decision.

(In reply to comment #10) > More precisely, what I'm worried about is that opening Panorama once doesn't > mean you're going to use it (let alone that you want the button on the tab > bar). You need to open it before you can even make that decision. Exactly. It seems to violate the principle of least surprise, and veer into "adaptive UI" territory in a way that makes me uncomfortable.

>And I'd like to say that I presonally dislike that we add it automatically — >just because a user visits Panorama once to see what it's for, doesn't mean >(s)he necessarily want the button there afterwards. I'm not a big fan of adaptive UI either (at least when it comes to visual targets you hit with the mouse). However, I think this is the best approach, because then we can give the user clear instructions for how to start using panorama (menu command, keyboard shortcut), and the control will likely end up in a consistent position, since we are adding it instead of them. If the primary path is the user adding the control, every user is going to have the panorama button in a different location. And this location won't line up with support documents and videos, the control to exit panorama, or where the panorama button is going to be placed in future versions of Firefox. So proactively adding the control is a bit strange, but at least we get to place it in the correct location :)

>More precisely, what I'm worried about is that opening Panorama once doesn't >mean you're going to use it (let alone that you want the button on the tab >bar). You need to open it before you can even make that decision. Sure, but that also would have been true if we had fully shipped the feature and added the control by default. Also, 72% of users aren't going to access the List All Tabs menu (and even fewer, will accidentally hit accel-shift-e), so Panorama will fly under the radar a bit for our mainstream users.

Adding Frank Yan, who pointed out that we should only add the button once - if the user removes it, we shouldn't re-add it. Can use the "has Panorama been run at least once" flag to do this, he figures.

(In reply to comment #14) > Adding Frank Yan, who pointed out that we should only add the button once - if > the user removes it, we shouldn't re-add it. Can use the "has Panorama been run > at least once" flag to do this, he figures. The button is only inserted when panorama is run the first time. So when removed after the first panorama experience it does not get re-added. So what's the result of all this? Is this patch still going to be considered? If so, should the button be added right or left of the 'List all tabs' button?

(In reply to comment #14) > Adding Frank Yan, who pointed out that we should only add the button once - if > the user removes it, we shouldn't re-add it. Can use the "has Panorama been run > at least once" flag to do this, he figures. Just to clarify, the code to insert the toolbar button only when panorama is run for the first time at all, not the first time each browser session. If I understand the code correctly, the patch already does this, which is awesome. (In reply to comment #15) > So what's the result of all this? Is this patch still going to be considered? Yes, we have it as a hard blocker, meaning that we won't ship without it. Thanks for creating a patch. I'll give the patch a non-final feedback review if Dao doesn't get to it first. > If so, should the button be added right or left of the 'List all tabs' button? The panorama button should be added to the right of the 'List all tabs' button.

(In reply to comment #16) > Just to clarify, the code to insert the toolbar button only when panorama is > run for the first time at all, not the first time each browser session. If I > understand the code correctly, the patch already does this, which is awesome. Yep. > The panorama button should be added to the right of the 'List all tabs' button. Perfect, the patch does this already.

(In reply to comment #11) > (In reply to comment #10) > > More precisely, what I'm worried about is that opening Panorama once doesn't > > mean you're going to use it (let alone that you want the button on the tab > > bar). You need to open it before you can even make that decision. > > Exactly. It seems to violate the principle of least surprise, and veer into > "adaptive UI" territory in a way that makes me uncomfortable. Perhaps a better heuristic would be to add the button when you've created an orphan tab or a new tab group, not just gone into Panorama. That way, people who look at "tab groups" and it looks scary and close it, will not get the button.

(In reply to comment #18) (In reply to comment #9) > I'd rather we just leave it in the customization palette for the people that > want it — or have something you can click in the Panorama screen itself to add > the button. I actually concur with Limi; I was just making sure that, if we stick with the current approach of automatically adding in the toolbar button, we do it right.

>Perhaps a better heuristic would be to add the button when you've created an >orphan tab or a new tab group, not just gone into Panorama. That way, people >who look at "tab groups" and it looks scary and close it, will not get the >button. yeah, I think that is probably the best heuristic. Also, it makes sense in that the icon itself is an indicator of groups (so the first time it shows up it also indicates groups).

>The panorama button should be added to the right of the 'List all tabs' button. What Frank means to say here is that it should be to the left of the list all tabs button :) considerations: -we don't want list all tabs moving position (right most item, no longer rightmost item) -if this is viewed as a combo button (it kind of is), then the arrow needs to be on the right side of the main control, which in this case is the panorama button

The logical place is right of "List all tabs", as "List all tabs" correlates with the current set of tabs to its left, whereas the panorama button is the "zoom out" control.

They both technically mean "all", so that doesn't really help to differentiate which one should be on the outside more than the other. But with the first item in list all tabs the same as the panorama button (tab groups), it seems like the standard [main command] + [quick options including main command] makes sense.

(In reply to comment #21) > >The panorama button should be added to the right of the 'List all tabs' button. > > What Frank means to say here is that it should be to the left of the list all > tabs button :) I meant what I said, because as long as the panorama button in tab view is aligned to the right edge of the window, so the toolbar button should be there too. Ideally, the panorama button should not appear to move when toggling tab view. (It still moves a few pixels, depending on the platform and configuration.) Given the current behavior of the buttons, I also concur with Dao's comment 22.

Ah, I'm usually maximized on windows, so I've got the control moving so far right now that I hadn't considered that part.

(In reply to comment #25) > Ah, I'm usually maximized on windows, so I've got the control moving so far > right now that I hadn't considered that part. Yeah, we should keep the control from moving. :| Not easy to do that when it's in the titlebar though. I could file a bug for that, if one isn't filed already.

Is there a bug to make the control and icon like the screenshot here instead of just a toolbar icon or is that still future work? http://www.stephenhorlander.com/images/blog-posts/incontent-ui/win7-tabcandy-glass.jpg

(In reply to comment #27) > Is there a bug to make the control and icon like the screenshot here instead of > just a toolbar icon or is that still future work? > http://www.stephenhorlander.com/images/blog-posts/incontent-ui/win7-tabcandy-glass.jpg Once Panorama is a bit less experimental, we'll re-evaluate how central it'll be in the UI. Right now, our main concern is to make sure it's not something you'll accidentally activate, since it has the possibility of (perceived) data loss to the user.

Comment on attachment 506295 [details] [diff] [review] patch v2 I think there was consensus that we want a better heuristic.

Based on other UX comments, I'm moving this to a soft blocker. I still think we should do it, and I agree that if the user enters & exits, we shouldn't add the UI. If they do anything, then it should be added (just the once). Yes, this is somewhat adaptive UI, but I also think that due to the reliance of Panorama on being quickly activated by the button, it makes sense.

We should put the Panorama button in the UI if the user: * Named a group * Created a group (based on discussion in the UX meeting)

(In reply to comment #31) > We should put the Panorama button in the UI if the user: > > * Named a group > * Created a group It would also make sense to run when the user creates an orphan, as this is functionally equivalent to creating a group, i.e., the user is using Panorama and there's then a potential for tabs being hidden.

(In reply to comment #32) > (In reply to comment #31) > > We should put the Panorama button in the UI if the user: > > > > * Named a group > > * Created a group > > It would also make sense to run when the user creates an orphan, as this is > functionally equivalent to creating a group, i.e., the user is using Panorama > and there's then a potential for tabs being hidden. This patch has the following behavior: * When creating a group, add the button. * When naming a group, add the button. * When dragging a tab out of a group and creating an orphan, add the button. * When adding the button, store a pref so that the button is not re-added when explicitly removed by the user. A test is attached to ensure that everything works as intended.

Test corrected.

(In reply to comment #34) > Created attachment 508757 [details] [diff] [review] > patch v4 Passed try.

Comment on attachment 508757 [details] [diff] [review] patch v4 Looking good so far from the Panorama side (I don't know that much about tool bars). Comments: * It's also possible to create a group by dropping a tab onto an orphan tab; please cover that case as well. >+ addToolbarButton: function UI_addToolbarButton() { >+ let buttonId = "tabview-button"; >+ if (this._toolbarButtonExists(buttonId)) >+ return; >+ >+ if (gPrefBranch.prefHasUserValue("toolbar_button_added") && >+ gPrefBranch.getBoolPref("toolbar_button_added")) >+ return; I think the logic should instead be to check the pref (which you then cache), and then check the tool bar if appropriate. Then set the pref regardless of whether you added the button or it was already there.

(In reply to comment #36) > * It's also possible to create a group by dropping a tab onto an orphan tab; > please cover that case as well. I already implemented that an removed it because the precondition is that we have an orphan tab. That is covered, right? By talking about that I realize that I forgot to cover the case when creating an orphan via double click on the background. > I think the logic should instead be to check the pref (which you then cache), > and then check the tool bar if appropriate. Then set the pref regardless of > whether you added the button or it was already there. Yeah, that's better.

> Comment on attachment 508757 [details] [diff] [review] > patch v4 > >+ if (gPrefBranch.prefHasUserValue("toolbar_button_added") && > >+ gPrefBranch.getBoolPref("toolbar_button_added")) > >+ return; Perhaps this pref should just be called "used_panorama" or somesuch, and also be used in bug 626525?

(In reply to comment #37) > > * It's also possible to create a group by dropping a tab onto an orphan tab; > > please cover that case as well. > > I already implemented that an removed it because the precondition is that we > have an orphan tab. That is covered, right? By talking about that I realize > that I forgot to cover the case when creating an orphan via double click on the > background. Good points, both! Do that then. :)

(In reply to comment #38) > > Comment on attachment 508757 [details] [diff] [review] > > patch v4 > > >+ if (gPrefBranch.prefHasUserValue("toolbar_button_added") && > > >+ gPrefBranch.getBoolPref("toolbar_button_added")) > > >+ return; > > Perhaps this pref should just be called "used_panorama" or somesuch, and also > be used in bug 626525? Yes. If experienced_first_run isn't used for anything else, we can just keep using this.

(In reply to comment #40) > Yes. If experienced_first_run isn't used for anything else, we can just keep > using this. experienced_first_run is used for something else; it keeps track of whether we need to run the reset() code to create the initial group on startup. The new pref for this bug would have to be based on the heuristic from comment 33. Tim, I think the ball's in your court, yes?

> experienced_first_run is used for something else; it keeps track of whether we > need to run the reset() code to create the initial group on startup. Why would you need to treat a startup for a user having opened panorama once without doing anything differently from a startup for a user not having opened panorama?

Changes: 1.) The pref is now called "experienced_first_use". 2.) Handling now the case of orphans being created with double-click on the background. 3.) > I think the logic should instead be to check the pref (which you then cache), > and then check the tool bar if appropriate. Then set the pref regardless of > whether you added the button or it was already there. Fixed. > Why would you need to treat a startup for a user having opened panorama once > without doing anything differently from a startup for a user not having opened > panorama? In fact we don't need it at the moment because when there is no group information we create a default group anyway (regardless of whether panorama is launched for the first time). We need this again when bug 626926 is hopefully fixed for Firefox.Next.

(In reply to comment #43) > In fact we don't need it at the moment because when there is no group > information we create a default group anyway (regardless of whether panorama is > launched for the first time). We need this again when bug 626926 is hopefully > fixed for Firefox.Next. Sounds like this should be handled in that bug, then.

Comment on attachment 511013 [details] [diff] [review] patch v5 What ever the pref is called, we should use the same pref for this and the changes from bug 626525. This patch doesn't seem to do that.

(In reply to comment #45) > Comment on attachment 511013 [details] [diff] [review] > patch v5 > > What ever the pref is called, we should use the same pref for this and the > changes from bug 626525. This patch doesn't seem to do that. +1. Please fix this first. I would opt to change all instances of first_run, in its usage for bug 626525, to be called first_use. If first_run isn't going to be used at all for fx4, as is the plan, then we should just remove the code which sets its pref for the time being. We could reintroduce it in the future in bug 626926.

Comment on attachment 511879 [details] [diff] [review] patch v6 >+ _addToolbarButton: function TabView__addToolbarButton() { >+ let buttonId = "tabview-button"; >+ >+ if (this._toolbarButtonExists(buttonId)) >+ return; >+ // ---------- >+ // Function: _toolbarButtonExists >+ // Check if the panorama toolbar button is currently included in any of the >+ // available toolbars. >+ _toolbarButtonExists: function TabView__toolbarButtonExists(buttonId) { >+ let toolbars = document.getElementsByTagName("toolbar"); >+ >+ for (let i=0; i<toolbars.length; i++) { >+ let currentSet = toolbars[i].currentSet.split(","); >+ if (-1 < currentSet.indexOf(buttonId)) { >+ return true; >+ } >+ } >+ >+ return false; > } This can be simplified to: if (document.getElementById(buttonId) return; >+ try { >+ let toolbar = document.getElementById("TabsToolbar"); >+ let currentSet = toolbar.currentSet.split(","); >+ >+ currentSet.splice(-1, 0, buttonId); This seems to assume that the currentset equals the defaultset, but that's a bogus assumption, which means that -1 as a fixed insertion point will lead to random results. >+ toolbar.setAttribute("currentset", currentSet.join(",")); >+ toolbar.currentSet = currentSet.join(","); >+ document.persist(toolbar.id, "currentset"); >+ BrowserToolboxCustomizeDone(true); Calling BrowserToolboxCustomizeDone without BrowserCustomizeToolbar having been called can break stuff. >+ } >+ catch (e) {} Why would this throw? Don't try/catch arbitrarily.

(In reply to comment #49) > >+ try { > >+ let toolbar = document.getElementById("TabsToolbar"); > >+ let currentSet = toolbar.currentSet.split(","); > >+ > >+ currentSet.splice(-1, 0, buttonId); > > This seems to assume that the currentset equals the defaultset, but that's a > bogus assumption, which means that -1 as a fixed insertion point will lead to > random results. You mean because we want the panorama button right of the alltabs button? I thought we want the panorama button at roughly the same place when panorama is opened (comment #24)? So that would be at the right-most (LTR) / left-most (RTL) position?

(In reply to comment #50) > I thought we want the panorama button at roughly the same place when panorama is > opened (comment #24)? -1 won't necessarily do that, depending on what the last element in the currentset is. But then the idea of keeping the icon in the same spot when opening panorama is broken anyway... The simplest approach is probably to just move it next to the all-tabs button, and do nothing if the all-tabs button isn't in the currentset.

(In reply to comment #51) > The simplest approach is probably to just move it next to the all-tabs button, > and do nothing if the all-tabs button isn't in the currentset. Ok, will do that. Thanks!

(In reply to comment #49) > This can be simplified to: > > if (document.getElementById(buttonId) > return; OMG, this is so obvious, sorry :) Fixed. > This seems to assume that the currentset equals the defaultset, but that's a > bogus assumption, which means that -1 as a fixed insertion point will lead to > random results. Fixed. > Calling BrowserToolboxCustomizeDone without BrowserCustomizeToolbar having been > called can break stuff. Fixed. > >+ } > >+ catch (e) {} > > Why would this throw? Don't try/catch arbitrarily. You're right. I copied that from some MDC snippet - removed.

Comment on attachment 511940 [details] [diff] [review] patch v7 >+ if (-1 === alltabsPos) == >+ BrowserCustomizeToolbar().close(); Um. This is awful, you can't do that.

(In reply to comment #54) > >+ BrowserCustomizeToolbar().close(); > > Um. This is awful, you can't do that. Do you just want me to remove that call to BrowserToolboxCustomizeDone()? Seems to work fine without it. If that's really not needed we should correct the example I used to write this patch: https://developer.mozilla.org/en/Code_snippets/Toolbar#Example

Comment on attachment 511963 [details] [diff] [review] patch v8 Passed try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=2f497d6b048a

If this passes reviews, it has a=beltzner

Comment on attachment 511963 [details] [diff] [review] patch v8 >+ /* DISABLED BY BUG 626754. To be reenabled via bug 626926. > if (firstTime) { > gPrefBranch.setBoolPref("experienced_first_run", true); > // ensure that the first run pref is flushed to the file, in case a crash > // or force quit happens before the pref gets flushed automatically. > Services.prefs.savePrefFile(null); > >- /* DISABLED BY BUG 626754. To be reenabled via bug 626926. > let url = gPrefBranch.getCharPref("welcome_url"); > let newTab = gBrowser.loadOneTab(url, {inBackground: true}); > let newTabItem = newTab._tabViewTabItem; > let parent = newTabItem.parent; > Utils.assert(parent, "should have a parent"); > > newTabItem.parent.remove(newTabItem); > let aspect = TabItems.tabHeight / TabItems.tabWidth; > let welcomeBounds = new Rect(UI.rtl ? pageBounds.left : box.right, box.top, > welcomeWidth, welcomeWidth * aspect); > newTabItem.setBounds(welcomeBounds, true); > > // Remove the newly created welcome-tab from the tab bar > if (!this.isTabViewVisible()) > GroupItems._updateTabBar(); >- */ >- } >+ }*/ Please remove this whole block and this method's unused firstTime parameter.

(In reply to comment #58) > Please remove this whole block and this method's unused firstTime parameter. Removed.

Comment on attachment 512741 [details] [diff] [review] patch v9 >+ currentSet.splice(alltabsPos + 1, 0, buttonId); I think I'd prefer this: currentSet[alltabsPos] += "," + buttonId; >+ toolbar.setAttribute("currentset", currentSet.join(",")); >+ toolbar.currentSet = currentSet.join(","); You're joining twice, just do currentSet = currentSet.join(",") once. > XPCOMUtils.defineLazyGetter(this, "gWindow", function() { > return window.parent; > }); > > XPCOMUtils.defineLazyGetter(this, "gBrowser", function() gWindow.gBrowser); > >+XPCOMUtils.defineLazyGetter(this, "gTabView", function() { >+ return window.parent.TabView; >+}); >+ > XPCOMUtils.defineLazyGetter(this, "gTabViewDeck", function() { > return gWindow.document.getElementById("tab-view-deck"); > }); > > XPCOMUtils.defineLazyGetter(this, "gTabViewFrame", function() { > return gWindow.document.getElementById("tab-view"); > }); This stuff is cheap, so you can just do it this way: var gWindow = window.parent; var gBrowser = gWindow.gBrowser; var gTabView = gWindow.TabView; var gTabViewDeck = gWindow.document.getElementById("tab-view-deck"); var gTabViewFrame = gWindow.document.getElementById("tab-view"); >- reset: function UI_reset(firstTime) { >+ reset: function UI_reset() { There's still one call to this method with the argument passed in. Please drop it there as well.

(In reply to comment #60) > >+ currentSet.splice(alltabsPos + 1, 0, buttonId); > > I think I'd prefer this: > > currentSet[alltabsPos] += "," + buttonId; Fixed. > >+ toolbar.setAttribute("currentset", currentSet.join(",")); > >+ toolbar.currentSet = currentSet.join(","); > > You're joining twice, just do currentSet = currentSet.join(",") once. Fixed. > > XPCOMUtils.defineLazyGetter(this, "gWindow", function() { > > return window.parent; > > }); > > This stuff is cheap, so you can just do it this way: > > var gWindow = window.parent; Fixed. > >- reset: function UI_reset(firstTime) { > >+ reset: function UI_reset() { > > There's still one call to this method with the argument passed in. Please drop > it there as well. Oops. Sent to try again: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=ae3c2af38a8c

Comment on attachment 512801 [details] [diff] [review] patch for checkin this doesn't set the currentset attribute anymore

(In reply to comment #62) > this doesn't set the currentset attribute anymore Sorry, I misunderstood you.

Comment on attachment 512807 [details] [diff] [review] patch for checkin >+ toolbar.currentSet = currentSet.join(","); >+ toolbar.setAttribute("currentset", toolbar.currentSet); The toolbar.currentSet getter is expensive. You should do currentSet = currentSet.join(",").

(In reply to comment #64) > The toolbar.currentSet getter is expensive. You should do currentSet = > currentSet.join(","). Sorry :/

Passed try with some intermittent oranges: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=ae3c2af38a8c

Not seeing this behavior with today's Minefield nightly. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110217 Firefox/4.0b12pre New profile, hitting CMD-Shift-E twice, expected to see the toolbar button added but did not.

(In reply to comment #68) > Not seeing this behavior with today's Minefield nightly. You'll need to wait a little bit as this just landed about 5 hours ago. > New profile, hitting CMD-Shift-E twice, expected to see the toolbar button > added but did not. This will not add the button to the toolbar. Please use the behavior as described in comment #33 and comment #36.

(In reply to comment #69) > (In reply to comment #68) > > Not seeing this behavior with today's Minefield nightly. > > You'll need to wait a little bit as this just landed about 5 hours ago. > > > New profile, hitting CMD-Shift-E twice, expected to see the toolbar button > > added but did not. > > This will not add the button to the toolbar. Please use the behavior as > described in comment #33 and comment #36. Seeing this behaviour in the nightly now, was misled from simply the bug title.

We probably should have test(s) in Litmus to cover the scenarios indicated in comment 33 and 36. We should also create a Mozmill test for this. Tracy, can you add the Litmus test(s) and add rows to the Mozmill spreadsheet for any tests you add? Thanks.

Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110220 Firefox/4.0b12pre Verified issue with latest trunk.

After offline discussion with Tracy, we've identified that the existing Litmus tests cover this sufficiently; it doesn't need its own testcase.