Closed Bug 563730 Opened 14 years ago Closed 14 years ago

Create "tab-like" sticky buttons for frequently used web applications

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b1

People

(Reporter: mak, Assigned: dao)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

Attachments

(2 files, 4 obsolete files)

App tabs are special tabs, usually without a label, that are sticky and allow quick access to commonly accessed web applications.

While these will look like tab they don't necessarily need to be real tabs, nor to have the full behaviors of common tabs (detach, bookmark, scroll and so on), they could also have new behaviors.

A simple approach adds just a contextual "make app tab" menuitem to common tabs.
Drag and drop could be a plus.

since Tabbar is a toolbar, we could preserver customizability by allowing the user to put app tabs where he wants. This (and easier different behaviors and drag&drop) could maybe be obtained by creating a special app-tabs-container element, moveable across the toolbars (and the new tab toolbar).
Blocks: 563734
So, looks like no current addon does this in a way that could be (or we would want to be) uplifted.

The best way forward (which I've started) seems to be to make app-tabs real tabs, make the home tab an app-tab, have them held within their own container so they're pinned (this also gives us toolbar customization for free), and teach tabbrowser about them. 

The hard bit, of course, is that last part. I have a hacky proof-of-concept thing almost running, that I'll fix up properly tomorrow. I doubt it'll be review-ready by then though. But thankfully, it seems that once tabbrowser knows the basics, anything else can be put in/fixed in separate bugs.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Attached patch WiP v1 (obsolete) — Splinter Review
Still a work in progress. Works pretty well, but there's at least one bug in sessionstore (selected tab isn't correctly restored), only winstripe has the necessary theme changes, it's not syncing the page title, and it may need some general cleanup too and other things done too.
I took a look at the WIP v1 patch, and it looks good.

Are we following the design found here?
https://wiki.mozilla.org/Firefox/Projects/Home_Tab/Design#App_Tabs
(That design is far superior to my hacked-together App Tabs extension.)
(In reply to comment #3)
> Are we following the design found here?
> https://wiki.mozilla.org/Firefox/Projects/Home_Tab/Design#App_Tabs

Yea. I'll be meeting with Limi sometime this week to flesh out more of the details of the interactions - will update that page accordingly.
Attached patch WiP v2 (obsolete) — Splinter Review
This is basically ready for review, except for an annoying error in nsSessionStore.
Attachment #445935 - Attachment is obsolete: true
Attachment #447974 - Flags: feedback?(mak77)
Comment on attachment 447974 [details] [diff] [review]
WiP v2

Not sure the separate app tabs container is the right approach. There's some weird stuff going on:

- Attempting to close a window with open app tabs triggers the "You are about to close X tabs" warning.

- If you open enough tabs to cause overflow, selecting an app tab scrolls the tab strip to the beginning.

- Focusing a tab and switching tabs with the arrow keys stops working at app tabs.

- Clicking a selected tab doesn't focus it but clicking a selected app tab does.

- Middle clicking a link in the last app tab opens the new tab at the beginning, middle clicking a link in another app tab opens the new tab at the end.

Fixing up all these edge cases is going to be nasty.

Accessibility needs to be considered too.

>+  <binding id="tabbrowser-apptabs"
>+           extends="chrome://global/content/bindings/tabbox.xml#tabs">
>+    <resources>
>+      <stylesheet src="chrome://browser/content/tabbrowser.css"/>
>+    </resources>

What's the stylesheet used for?

>+
>+    <content>
>+      <children/>
>+    </content>

Remove?

>+  <binding id="tabbrowser-apptab" display="xul:hbox"
>+           extends="chrome://global/content/bindings/tabbox.xml#tab">
>+    <resources>
>+      <stylesheet src="chrome://browser/content/tabbrowser.css"/>
>+    </resources>

Same question as above.

>   onTabClose: function sss_onTabClose(aWindow, aTab) {
>+    // skip AppTabs, as these are managed seperately
>+    if (aTab.appTab)
>+      return;

"separately"

Where are they managed?

>-.tabbrowser-tab {
>+.tabbrowser-tab,
>+.tabbrowser-apptab {
>   padding: 0 2px 2px;
>   margin-bottom: 1px;
>   min-height: 25px; /* reserve space for the sometimes hidden close button */
> }
> 
>-.tabbrowser-tab[selected="true"] {
>+.tabbrowser-tab[selected="true"],
>+.tabbrowser-apptab[selected="true"] {
>   margin-bottom: 0;
>   padding-top: 2px; /* compensates the top margin of background tabs */
>   padding-bottom: 3px; /* compensates the bottom margin of background tabs */
>   min-height: 28px;
> }

This shouldn't be needed. App tabs should reuse the tabbrowser-tab class and have a second class if needed.
Attachment #447974 - Flags: feedback-
(In reply to comment #6)
> (From update of attachment 447974 [details] [diff] [review])
> Not sure the separate app tabs container is the right approach.

They need to be in their own container one way or another to ensure they're not scrolled with normal tabs (they need to be visible in the same position, no matter what). Another approach I tried was to try to make tabbrowser manage two containers independently, but that turned out to have far more downsides and edge-cases (and code changes). The other alternative of being completely separate from tabbrowser would mean re-implementing a lot of stuff, and still needing to deal with the various special-cases (and more).

> Fixing up all these edge cases is going to be nasty.

The cases you listed didn't seem too bad.

> Where are they managed?

That will be a separate bug, but I'm thinking preferences. Don't want it stored in sessionstore, as AppTabs are meant to be more permanent than a session. Will make that more clear in the comment.
(In reply to comment #7)
> > Fixing up all these edge cases is going to be nasty.
> 
> The cases you listed didn't seem too bad.

There are likely more, I spent only a few minutes on that list.
(In reply to comment #6)
> (From update of attachment 447974 [details] [diff] [review])
> Not sure the separate app tabs container is the right approach. There's some
> weird stuff going on:

(In reply to comment #8)
> > The cases you listed didn't seem too bad.
> 
> There are likely more, I spent only a few minutes on that list.

Is there an alternative approach you think Blair should be considering?
I don't know offhand.
usually trying to reuse the same thing for 2 much different use cases causes far more headaches in future, I think having a separate apptabs container should cause less, both cases have their edge cases, but touching a new container is less likely to cause regression in what we have, and that should be a good point at this advanced stage (beta is not so far).
If i'd have to vote, I'd not even want a binding for it, we have seen from past experience that bindings used just ones are not cheap at all, just removing the bookmarks toolbar binding we gained 1-2% on Ts, and that time was only spent in getting and applying it (so adding new ones is going toward worse Ts), thus if we cdould manage this special container with just listeners and eventually a global object I'd be completely in favor of that.
i meant "used just once"... sigh.
Comment on attachment 447974 [details] [diff] [review]
WiP v2

Another reason for them to be in a separate container is that drag&drop could have to be handled completely different, dropping in tabs container is just creating a tab, here we could need to set something more in future, esp when these will have to be synced across all browser windows.

I don't have enough time to check all the code and learn secrets of tabbrowser today, so I'll try to give some generic thought on the approach.


diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul

>+      <tabs id="tabbrowser-apptabs"
>+            class="tabbrowser-apptabs"
>+            tabbrowser="content"
>+            setfocus="false">

Going on you will need a new context menu with app-tabs options (and at that point would be cool to integrate something like prism functionality in the menu), and tooltips clearly, most likely you could use the tabbrowser-tab-tooltip for now it should give you the title of the page no?
would not be easier if these would have both tabbrowser-tabs and tabbrowser-apptabs classes and apptabs class would be defined later in the stylesheets to override just what you care about? I see that many css definitions are just repeats, and these are still tabs (with some special behavior). And same for -tab with -apptab. or they could have an "apptab" attribute.


>+      <method name="addAppTab">
>+        <parameter name="aURL"/>
>+        <body>
>+          <![CDATA[
>+            var tab = this.addTab(aURL);
>+            tab.className = "tabbrowser-apptab-placeholder";
>+            tab.linkedBrowser.isAppTab = true;
>+            this.moveTabTo(tab, this.appTabs.length - 1);

hm I'd bet addTab is doing a lot of work we don't need like touching scrollbuttons or checking relationship, you maybe want to
pass an optional param to it to avoid non related work


>+            var appTab = document.createElementNS(
>+              "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul",
>+                                                  "tab");
>+            appTab.className = "tabbrowser-apptab";
>+            appTab.realTab = tab;
>+            appTab.linkedBrowser = tab.linkedBrowser;
>+            tab.appTab = appTab;

I'm not sure why we can't just add/expando/override what we need to the tab object itself instead of having a tab child of a tab,
would not be easier to just set tab.isAppTab = true (and an "apptab" attribute) and append tab to the appTabContainer? Imo this thing should be directly handled in addTab with the above param


>+            // this can be set before addTab() returns, so re-set to sync
>+            if (tab.getAttribute("busy") == "true")
>+              this.setTabTitleLoading(tab);
>+            else
>+              this.setTabTitle(tab);

this looks a strange need


Does drag&drop work fine already? if I drag a tab to the apptabs container, I want that to become an apptab and vice versa, this does not look handled.
Also, context menus of tabs should have "make app tab", as well as app tabs context menu should have "make tab".


>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js
>--- a/browser/components/sessionstore/src/nsSessionStore.js
>+++ b/browser/components/sessionstore/src/nsSessionStore.js
>@@ -744,16 +744,19 @@ SessionStoreService.prototype = {
>    * @param aWindow
>    *        Window reference
>    * @param aBrowser
>    *        Browser reference
>    * @param aNoNotification
>    *        bool Do not save state if we're updating an existing tab
>    */
>   onTabAdd: function sss_onTabAdd(aWindow, aBrowser, aNoNotification) {
>+    if (aBrowser.isAppTab)
>+      return;
>+    

there are various trailing spaces in sessionstore changes (And various in the code itself ;()


>     var tabbrowser = aWindow.gBrowser;
>-    var openTabCount = aOverwriteTabs ? tabbrowser.browsers.length : -1;
>+    var numAppTabs = tabbrowser.appTabs.length - 1;
>+    var openTabCount = aOverwriteTabs ? tabbrowser.browsers.length - numAppTabs : -1;

hm, I would tell there could be lots of extensions using browsers.length, and this is going to give headaches... should maybe add appBrowsers and manage them separately?
which kind of problems could that give us?

Ideally, apptabs should reuse tabs code, but they should not interleave with tabs if possible. The more separation we can have, the less compatibility issues we have and less internal code needs to be adapted, the separate container should help achieving this, but then looks like the separation ends up to be only visual.


>diff --git a/browser/themes/pinstripe/browser/browser.css b/browser/themes/pinstripe/browser/browser.css

> 
>+.tabbrowser-apptab > .tab-icon-image {
>+  -moz-margin-end: 0px;
>+}

looks like this could be managed in the title code, if there's a title there should be a margin around the title, otherwise not, having it on the icon makes it less general (and winstripe has a similar margin definition but i don't see it overridden here).
Attachment #447974 - Flags: feedback?(mak77)
Ok, so, I feel I should give some background information to justify the implementation so far, as many of the the reasons behind this aren't clear. I really do think this implementation is the best way forward. Its not perfect, but all the alternatives I've explored have even more downsides.

* Having AppTabs completely separate from tabbrowser would mean breaking the assumption that tabbrowser controls what is displayed, and that tabbrowser always has a selected tab. Pretty much all code that uses tabbrowser in some way makes this assumption.

* Having the actual tabs for AppTabs be only in their own container (ie, not having placeholders in the normal tab container) also breaks a lot of assumptions in tabbrowser - mostly to do with handling the selected tab. There's also the issue that tabbox doesn't support not having a tab selected - I get away with using a hidden tab that can be selected in the AppTab tabbox only because its a facade. I tried for awhile to make this method work - but it ended up breaking too many things and needing a *lot* of changes.

* Having hidden tabs for AppTabs in the normal tab container means most of the existing tabbrowser code Just Works. There are a few hooks and guards for special management of AppTabs, but for the most part very little needed changing. It also means any other code interacting with tabs doesn't have to know about AppTabs unless it wants to handle them in a special way.

* Having the browsers of AppTabs be in the same container makes sense to me. In nearly all cases, the actual content in the browser should be treated just like a normal tab.

* I'm using the tabbrowser-apptab class (instead of tabbrowser-tab) because the visual styling will change. The idea is to allow AppTabs to be either loaded or not - similar to BarTab functionality. When an AppTab is loaded, it will display similar to a small normal tab. However, when its not loaded, it will look more like a button or a floating icon (in the same position).
(In reply to comment #14)
> * I'm using the tabbrowser-apptab class (instead of tabbrowser-tab)

It's not a question of either / or. You can have multiple classes, as I tried to say. You're definitely doing it wrong when duplicating .tabbrowser-tab selectors with s/tabbrowser-tab/tabbrowser-apptab/. Use tabbrowser-tab to pick up the similarities and tabbrowser-apptab for the differences.
I would just like to say this, regarding chromeless tabs... I already commented on Blair's Brain but I'm saying this to make sure it's not missed.

Chromeless tabs? In a nutshell: don't do it. Reasons? Here's a few:

# There is no logic behind chromeless tabs, and no benefit (that I can see);

# Home pages are usually a whole site, not a single page, and the same goes for app tabs! You will definitely break functionality, and scare away people from the home page and app tabs, creating all kinds of support problems. I'm all for opening a new tab when opening links on the home tab, but make that domain dependent, and not page dependent. Imagine people who have to navigate inside their home page to access content. Nightmare! And implementing domain-dependent new tab behavior will mean you will have to include some way to go back to base on the home tab, because people will want a substitute for the home button inside the home tab (because some sites still are not easy to navigate, and the browser should rely on the sites: the sites should rely on the browser!). The address bar is probably where you need to put it;

# As it stands now, there is no reason for someone to use the new tab button: you can just type an address and press alt+ENTER, same with search, or middle-click links or bookmarks. Don't go and create a reason, because, again, that will break usability for many people;

# They are hideous;

# There's the redundancy you would need to create in the default home page (which would be nothing else that covering a huge design flaw with a transparent sheet of A5 paper, because as soon as someone changes the home page, there goes the redundancy out the window). You would need to include a search field (that would open on a new tab), a location bar (that would open on a new tab), and of course all the buttons the user would have put in the toolbar to have easy access to, INSIDE the home tab. As you can see, this is a nightmare.

Conclusion: DO NOT USE CHROMELESS TABS! As an option, sure, but if you make this the default behavior, it will be a nightmare for many people, and I think you'll just be wasting time because you'll eventually get them out of Firefox in beta stage...

If you need few study cases that will tell you, right away, that this will never work, think about these:

# A user who has Google (or whatever search engine that navigates away from the main page to display results) as his homepage. His normal routine is to open Firefox, see his homepage, type in some stuff, open the results in the same tab, middle-click a few links, press the home button, go back to the home page in the same tab, type in some stuff, and so on. Then when he's full of it, he'll go check the tabs he opened in the background to see the results. Imagine how much loose would there be for this user with chromeless tabs, that open a new tab as soon as you navigate away from them (in this case, display search results);

# A user who uses a forum as his homepage, where he checks on using a single tab while he does the rest of his browser in the other tabs. Imagine how impossibly irrelevant (as with the previous example) the homepage would be if it would open a new tab on each clicked link;

# A user who doesn't care about his homepage and only types in an address to go somewhere. And just to make things more interesting, he doesn't have the default home page, but some ad-page that some random application fooled him in set as his homepage;

# A user who doesn't care about his homepage and opens Firefox (on this occasion) just to access some extension he has installed, for which he has a button on the toolbar;

# A newb going «FIREBOX AS NO TOOLBATTONS!!!!!1»;

# A geek going «WHERE HAS MY INTERFACE GONE!!! I WANT AN EXTENSION FOR THIS!»;

# A user has a homepage that he uses for a while after he launches Firefox, and then navigates away from when he doesn't want it anymore, inside the same tab, because he's all «oh, my son says I should have more than one tab open at a time, but I'm a self-righteous pedantic moron so I'll keep using one tab just to show him that "you can only read one thing at a time!».

I really hope I've made my point. And if this is not the right place to post this, please tell me where.

Thanks :)
Let's just wait until this has landed; then we can see how this affects usability and whatnot. Maybe we could add options to customise the feature?
(In reply to comment #16)
> I would just like to say this, regarding chromeless tabs... I already commented
> on Blair's Brain but I'm saying this to make sure it's not missed.
> 
> Chromeless tabs? In a nutshell: don't do it. Reasons? Here's a few:
> 
> # There is no logic behind chromeless tabs, and no benefit (that I can see);
> 
> # Home pages are usually a whole site, not a single page, and the same goes for
> app tabs! You will definitely break functionality, and scare away people from
> the home page and app tabs, creating all kinds of support problems. I'm all for
> opening a new tab when opening links on the home tab, but make that domain
> dependent, and not page dependent. Imagine people who have to navigate inside
> their home page to access content. Nightmare! And implementing domain-dependent
> new tab behavior will mean you will have to include some way to go back to base
> on the home tab, because people will want a substitute for the home button
> inside the home tab (because some sites still are not easy to navigate, and the
> browser should rely on the sites: the sites should rely on the browser!). The
> address bar is probably where you need to put it;
> 
> # As it stands now, there is no reason for someone to use the new tab button:
> you can just type an address and press alt+ENTER, same with search, or
> middle-click links or bookmarks. Don't go and create a reason, because, again,
> that will break usability for many people;
> 
> # They are hideous;
> 
> # There's the redundancy you would need to create in the default home page
> (which would be nothing else that covering a huge design flaw with a
> transparent sheet of A5 paper, because as soon as someone changes the home
> page, there goes the redundancy out the window). You would need to include a
> search field (that would open on a new tab), a location bar (that would open on
> a new tab), and of course all the buttons the user would have put in the
> toolbar to have easy access to, INSIDE the home tab. As you can see, this is a
> nightmare.
> 
> Conclusion: DO NOT USE CHROMELESS TABS! As an option, sure, but if you make
> this the default behavior, it will be a nightmare for many people, and I think
> you'll just be wasting time because you'll eventually get them out of Firefox
> in beta stage...
> 
> If you need few study cases that will tell you, right away, that this will
> never work, think about these:
> 
> # A user who has Google (or whatever search engine that navigates away from the
> main page to display results) as his homepage. His normal routine is to open
> Firefox, see his homepage, type in some stuff, open the results in the same
> tab, middle-click a few links, press the home button, go back to the home page
> in the same tab, type in some stuff, and so on. Then when he's full of it,
> he'll go check the tabs he opened in the background to see the results. Imagine
> how much loose would there be for this user with chromeless tabs, that open a
> new tab as soon as you navigate away from them (in this case, display search
> results);
> 
> # A user who uses a forum as his homepage, where he checks on using a single
> tab while he does the rest of his browser in the other tabs. Imagine how
> impossibly irrelevant (as with the previous example) the homepage would be if
> it would open a new tab on each clicked link;
> 
> # A user who doesn't care about his homepage and only types in an address to go
> somewhere. And just to make things more interesting, he doesn't have the
> default home page, but some ad-page that some random application fooled him in
> set as his homepage;
> 
> # A user who doesn't care about his homepage and opens Firefox (on this
> occasion) just to access some extension he has installed, for which he has a
> button on the toolbar;
> 
> # A newb going «FIREBOX AS NO TOOLBATTONS!!!!!1»;
> 
> # A geek going «WHERE HAS MY INTERFACE GONE!!! I WANT AN EXTENSION FOR THIS!»;
> 
> # A user has a homepage that he uses for a while after he launches Firefox, and
> then navigates away from when he doesn't want it anymore, inside the same tab,
> because he's all «oh, my son says I should have more than one tab open at a
> time, but I'm a self-righteous pedantic moron so I'll keep using one tab just
> to show him that "you can only read one thing at a time!».
> 
> I really hope I've made my point. And if this is not the right place to post
> this, please tell me where.
> 
> Thanks :)

Let me ask you a simple question: why on Earth you need chrome for applications?
Anyway, I was using Gmail in chromeless app tab for quite some time and I can tell from my own experience there is absolutely no need for any toolbar in chrome. Every single action is happening inside site.
Think of app tabs as integrated Prism (more or less).
(In reply to comment #17)
> Let's just wait until this has landed; then we can see how this affects
> usability and whatnot. Maybe we could add options to customise the feature?

Yes, that's probably a good idea: let the user *enable* chromeless tabs in the options menu :)

Seriously, you didn't even read my comment! Why do UX professionals always discard whatever a single user puts forth, no matter of convincing or eloquent it is, and always just listen to statistics and inexperienced users complaining over at SUMO?

I think you're just wasting time with this, but whatever. I don't know if it is just my innate talent to design or something, but this is very clear to me: this is a usability nightmare you DON'T want to get yourself into. Even in the nightlies. Just imagine how long it will take you to realize it's not going to do it, and then figure out a way to emulate the logic behind chromeless tabs in some other way, and THEN figure out it's still not going to do it and all that...

If you do indeed choose to go on with development and land this feature, be my guest, but don't say I didn't warn you when you disable it by default or even altogether. Or when you release the final version with this and you get a flood at SUMO. Again.



(In reply to comment #18)
> Let me ask you a simple question: why on Earth you need chrome for
> applications?

Thanks a lot for the reply Peter.

I will go past the point I made that I (the anonymous non-singular user) need chrome in the home tab, and answer your question: I need chrome in an apptab for multiple reasons: A, I need some way to keep track of where I am inside the appsite (I assume we will be able to navigate inside the appsite, because that's the logic thing to do), B, I need to be able to open a new tab when looking at an appsite (because, yes, I don't like the new tab button on the tab bar, and I keep it where the home button is on default 3.6), C, I need easy access to extensions controls in my toolbar (again, I assume extensions won't be disabled on appsites like they are - were? - in Prism, because that would be very logical - or very doable), and D, I don't want to be force to open a new tab, when looking at an appsite, to open a new site (using the address bar plus ALT+ENTER, or just ENTER in the apptab, since it would open a new site, so it would just open in a new tab) or use a search engine (using the search bar plus ALT+ENTER, or just ENTER, for the same reasons as before).

I think that pretty much sums it up.

(In reply to comment #18)
> Anyway, I was using Gmail in chromeless app tab for quite some time and I can
> tell from my own experience there is absolutely no need for any toolbar in
> chrome. Every single action is happening inside site.
> Think of app tabs as integrated Prism (more or less).
I understand your point very well, but I will venture a guess that you are not a UX designer... If you were, you'd know that just because *you* work ok with it, doesn't mean other people will. Furthermore, just because it works ok with Gmail, doesn't mean it works ok with all the sites (because any site can be an appsite). As I said, let the user decide, please! But don't make it the default, because the users who have the most problems with chromeless tabs, are usually those who are afraid of the options menu... Experienced users like you and I can go there and change the setting.
Compromise then: make it default and soon as apptabs is opened, show notification if user want to keep it that way, or change it.
Yeah? And who are the most likely to miss a notification? Experienced users who want chromeless tabs, or unexperienced users who don't want or are confused by them?

I appreciate that you are quick to change your position (no matter how slightly), but I really think Firefox should be set up, by default, to be friendly to the unexperienced users first, and allow the experienced ones, who do know what they want, to configure it as they please. After all, this is the main reason why "find as you type" isn't enabled by default: it would confuse people. And yet my head explodes every time I use a browser without it.

A notification is nice, but make it ask if the user wants to *disable* chrome, rather than enable it, because it's supposed to be there.
Same notification as is now being used for remembering passwords should be enough. Please, don't presume users are stupid who can't for themeselfs. Furthermore, this approach is bad. And you know why? Because user will not know that there is option to change behaviour. Standard user will NOT be poking in prefs just to check if there is something new. Therefore, user should be notified that he has the option to change it.
(In reply to comment #22)
> Same notification as is now being used for remembering passwords should be
> enough.
I remind you that those notifications will not be present in Firefox 4. Notifications will work in some need ballon popup way.

(In reply to comment #22)
> Please, don't presume users are stupid who can't for themeselfs.
Well... I really don't know how to reply to that. Have you been to SUMO? Have you seen the overwhelming hoard of people who can't fix simple problems?

(In reply to comment #22)
> Furthermore, this approach is bad. And you know why? Because user will not know
> that there is option to change behaviour. Standard user will NOT be poking in
> prefs just to check if there is something new. Therefore, user should be
> notified that he has the option to change it.
Oh, you tell me not to presume they're stupid, but you do? That's not fair! The standard user doesn't want chromeless tabs, and you're right, the standard user will not be poking in the prefs. So make standard Firefox for the standard user.

There's pretty much it. Get the notification, sure, but a notification that tells the user that he can disable chrome in apptabs.
(In reply to comment #23)
> (In reply to comment #22)
> There's pretty much it. Get the notification, sure, but a notification that
> tells the user that he can disable chrome in apptabs.
That is pretty much what I was trying to tell you.
The question here is what defines and AppTab. It's a tab which is stuck to the
left of the tab strip and it's a tab that is used solely for the page it's been
created for. I have no idea why anyone would *require* a location bar or
bookmarks toolbar from this page, as with desired implementation, opening a
bookmark from an AppTab would create a new tab at the end of the tab bar.

Either way, let's get this implemented first and then tweak it after. At the
moment, it's a question of getting the usability of the AppTabs, rather than
the feel (whether to display the navigation bar or not).
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > There's pretty much it. Get the notification, sure, but a notification that
> > tells the user that he can disable chrome in apptabs.
> That is pretty much what I was trying to tell you.

Sorry, I misunderstood. I thought you were saying to make chromeless apptabs the default.

(In reply to comment #25)
> The question here is what defines and AppTab. It's a tab which is stuck to the
> left of the tab strip and it's a tab that is used solely for the page it's been
> created for. I have no idea why anyone would *require* a location bar or
> bookmarks toolbar from this page, as with desired implementation, opening a
> bookmark from an AppTab would create a new tab at the end of the tab bar.

Paul, pleas read comment #19, my reply to Peter.
(In reply to comment #26)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > (In reply to comment #22)
> > > There's pretty much it. Get the notification, sure, but a notification that
> > > tells the user that he can disable chrome in apptabs.
> > That is pretty much what I was trying to tell you.
> 
> Sorry, I misunderstood. I thought you were saying to make chromeless apptabs
> the default.
Well, yes, I want, but notifie user he can change that. Something like: "If you want display chrome, press this button".
Harumph... That's what I was trying to say! Chromeless tabs are "for" non-standard users. Pref poking is "for" non-standard users. We should have that in mind.
I have feeling like I'm telling with wall... Again, make it default, BUT display notification when opening app tab for the first time that it's behaviour can be reverted be pressing ONE SINGLE BUTTON. It can't be any simplier than that.
It can: show chrome by default. Users tend to miss notifications, and if you don't show a notification each time, there will be people left wondering what happened. That's my biggest grip, to be honest. Not matter how many notifications you should, there will always be people missing them, and they will have problems, so, as a solution, make chrome visible by default, and show a notification (for advanced users, who don't miss notifications) telling them they can hide the Chrome if they want.

And if I were to guess, I'd guess there are more people who WANT chrome than those who DON'T want it. There is no other major browser with chromeless tabs, and there are all those disadvantages I mentioned before.
Tiago, Peter - take it to the newsgroups. App Tabs are part of the product plan, and while they are certainly subject to iteration and review (and even removal if they fail completely), bugzilla is not the place for it. Bugzilla is for tracking specific technical work - newsgroups are for broader design discussion.

And please no grief about how no one's listening to you - I'm trying to steer you to the location where listening happens.
Sorry about that Johnathan.
Dao: Based on comment 14, are you fine with me going ahead with this approach? I do think its the best of the alternatives, but I don't want to put additional time into it if its never going to get review.
With adjustments to the scrollbox code, could position:fixed be used to prevent app tabs from scrolling?
Then app tabs would still be between the scroll arrows, no? That would be very confusing...
They would, unless you move things around with css margins.
According to the design doc, the same set of app tabs are also going to be shown on all windows, so it makes less sense to put them in the same container.
(In reply to comment #37)
> According to the design doc, the same set of app tabs are also going to be
> shown on all windows, so it makes less sense to put them in the same container.

I don't think this necessarily follows. Separate windows will need to have separate app-tab instances for the foreseeable future, so I don't think the multiple windows case is relevant to how we decide to store them in a given window.

My initial reaction was that we should keep the two containers completely separate, but comment 14 does a good job of listing why that isn't really feasible. Dao's suggestion from the other end of the spectrum (comment comment 34/comment 36 - keeping them in the same container, but with styling and scrollbox hacks to keep them in the right position) sounds like the simplest solution in terms of implementation, but it's only feasible if the styling hacks can reliably get us the interaction/look that we want, and I'm not sure that's the case. Worth investigating, I think.

If the same-container-with-styling-hacks approach doesn't work, I wonder whether we can use a different compromise: keep the app tabs in a separate anonymous container within the existing tabbrowser-tabs binding (outside of the scrollbox), but have the tabbrowser's "tabs" getter return the union of both containers' childNodes, with the tabbrowser otherwise treating them as a single collection? I haven't thought through all the implications of that - some scrollbox-related code would need fixes (selection, avoiding scrollTo() for app tabs, etc.), and I'm sure there would be other issues - but it may be easier to deal with than maintaining separate mirrored dummy tabs in the main container?
(In reply to comment #38)
> Dao's suggestion from the other end of the spectrum (comment comment
> 34/comment 36 - keeping them in the same container, but with styling and
> scrollbox hacks to keep them in the right position) sounds like the simplest
> solution in terms of implementation, but it's only feasible if the styling
> hacks can reliably get us the interaction/look that we want, and I'm not sure
> that's the case. Worth investigating, I think.

So, I've been playing with absolute positioning - and it just doesn't work in a scrollbox (as in, it has no visible effect). No idea why though - maybe I'm missing something. But I do know absolute positioning in XUL can be a bit funky in general - last time I tried using absolute positioning I gave up and just switched to using HTML's box-models. Regardless, this would require manually maintaining the pixel position of all AppTabs and the normal tab container - through adding, removing, moving, and loading AppTabs.


> If the same-container-with-styling-hacks approach doesn't work, I wonder
> whether we can use a different compromise: keep the app tabs in a separate
> anonymous container within the existing tabbrowser-tabs binding (outside of the
> scrollbox), but have the tabbrowser's "tabs" getter return the union of both
> containers' childNodes, with the tabbrowser otherwise treating them as a single
> collection? I haven't thought through all the implications of that - some
> scrollbox-related code would need fixes (selection, avoiding scrollTo() for app
> tabs, etc.), and I'm sure there would be other issues - but it may be easier to
> deal with than maintaining separate mirrored dummy tabs in the main container?

I've also been playing around with this - definitely not easier. Its taking a lot more work and ends up being more complex, in part because a lot of tabbrowser just doesn't use the tabs getter (for various reasons). And it still has the issue with tabbox not supporting having nothing selected. Together with needing to alter the tab-switching behaviour, a decent amount of the tabbox code would also need to be overridden.

We could always explore this further some time in the future, but I've already spent too much time exploring alternate aproaches that haven't yeilded any satisfactory results. For now, I'll continue with my existing approach, as that's the only one thats currently working.
What about modeling it after the new tab button (with some tweaks of course)?
(In reply to comment #39)
> So, I've been playing with absolute positioning - and it just doesn't work in a
> scrollbox (as in, it has no visible effect). No idea why though - maybe I'm
> missing something.

Please attach what you have. My quick test of adding position:fixed to tabbrowser tabs with DOMi worked just fine.

> Regardless, this would require manually
> maintaining the pixel position of all AppTabs and the normal tab container -
> through adding, removing, moving, and loading AppTabs.

Sure.
(In reply to comment #41)
> > Regardless, this would require manually
> > maintaining the pixel position of all AppTabs and the normal tab container -
> > through adding, removing, moving, and loading AppTabs.
> 
> Sure.

Maybe I should be more explicit: Yes, it's unfortunate that we'd have to do this, but I think it's going to be a less involved problem compared to the separate container.
(In reply to comment #41)
> Please attach what you have. My quick test of adding position:fixed to
> tabbrowser tabs with DOMi worked just fine.

I would if I hadn't already deleted it. Could you describe what styles you applied? I'm still not convinced this would be a better solution (especially since the other solution already works) - and I have no idea how dragging tabs would fit together with absolutely positioned tabs.
(In reply to comment #40)
> What about modeling it after the new tab button (with some tweaks of course)?

What's kind-of what Gavin's solution and my solution involves (and what Dao is against) - adding an additional toolbar item to make AppTabs stay in the same position. However, integrating with tabbrowser is the other part of the problem.
I set style="position:fixed", and that's it. So theoretically, you should move the tab to the beginning of the tab strip, do tab.classList.add("tabbrowser-apptab"), and pick up the positioning from browser.css:

.tabbrowser-apptab {
  position: fixed;
}
(In reply to comment #45)
> I set style="position:fixed", and that's it.

Ah - I stand corrected.
Attached patch Patch v1 (obsolete) — Splinter Review
This continues with the approach of using a separate container of AppTabs that is synced to hidden tabs in the main tab container. It fixes review comments so far from the previous patch - including using the tabbrowser-tab class, avoiding an XBL binding for the container, refactoring addTab() to not do unnessisary work, focus issues.

Things this patch does not do that will be in separate bugs: UI for adding/removing/moving AppTabs, persisting AppTabs, context menu for AppTabs, BarTab-like functionality for AppTabs.

Note: I have a couple of tests failing. I believe they're unrelated, but haven't proven that yet (browser_bug521216.js and browser_bug500328.js). Additionally, there's a non-fatal error in nsSessionStore which needs fixed. I expect any changes from this to be minor - so I'd like to fix them in parallel to getting review.
Attachment #447974 - Attachment is obsolete: true
Attachment #450449 - Flags: review?(gavin.sharp)
Attachment #450449 - Flags: review?(mak77) → feedback+
Comment on attachment 450449 [details] [diff] [review]
Patch v1

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+            if (!aAppTab) {
>+

>+
>+            } else {
>+

>+
>+            }

newlines around if/else do not look like a common style in this file


>@@ -1294,26 +1345,35 @@
>             if ((aRelatedToCurrent == null ? aReferrerURI : aRelatedToCurrent) &&
>                 Services.prefs.getBoolPref("browser.tabs.insertRelatedAfterCurrent")) {
>               let newTabPos = (this._lastRelatedTab ||
>                                this.selectedTab)._tPos + 1;
>               this.moveTabTo(t, newTabPos);
>               this._lastRelatedTab = t;
>             }
> 
>-            return t;
>+            return aAppTab ? appTab : t;

should the previous relatedTo code be handled for appTabs too?

>+      <method name="addAppTab">
>+        <parameter name="aURL"/>
>+        <body>
>+          <![CDATA[
>+            return this.addTab(aURL, {appTab: true});

imho, you use appTab name too often for vars, in this code appTab can be a bool, in the above one is a tab... what about calling this param isAppTab for better readability?


>             // Remove the tab ...

nit: while there, space before dots

>+      <property name="normalTabs" readonly="true">
>+        <getter>
>+          <![CDATA[
>+            return this._normalTabs ||
>+                   (this._normalTabs = Array.filter(this.tabs, function (tab) !tab.appTab));
>+          ]]>
>+        </getter>
>+      </property>

I don't like the "normal" prefix, normal regarding to what? maybe visibleTabs, pageTabs? what is this for? I can only find one usage in browserGlue and that sounds like being achievable in other ways (.tabs.length - .appTabs.length).
Actually I think .tabs should return "normal" tabs and there should be a .allTabs, but I guess that would break compatibility :(
It is also easy and short to implement for callers (and you could do the same in your code) using array comprehensions:

let commonTabs = [ t for each (t in browser.tabs) if (!t.appTab) ];
or
let commonTabs = [ t for each (t in browser.tabs) if (browser.appTabs.indexOf(t) != -1) ];

imo you could even avoid the method and add a comment above "tabs" with one of these examples.

>       <method name="moveTabTo">
>         <parameter name="aTab"/>
>         <parameter name="aIndex"/>
>         <body>
>         <![CDATA[
>+          // first make sure we're working with a normal tab, not an AppTab
>+          if (aTab.realTab)
>+            aTab = aTab.realTab;

.realTab exists only for appTabs, would not this fire a js warning? should it be ("realTab" in aTab) instead?

>+          // don't allow mixing normal tabs and app tabs

nit: if possible make comments proper phrases, uppercase, use periods...


>+      <method name="_selectNewTab">
>+        <parameter name="aNewTab"/>
>+        <parameter name="aFallbackDir"/>
>+        <parameter name="aWrap"/>
>+        <body>
>+        <![CDATA[
>+          var requestedTab = aNewTab;

new code in this file uses let, btw ask gavin what to use and be consistent.

>+          while (aNewTab.hidden || aNewTab.disabled) {
>+            aNewTab = aFallbackDir == -1 ? aNewTab.previousSibling : aNewTab.nextSibling;
>+            if (!aNewTab && aWrap)
>+              aNewTab = aFallbackDir == -1 ? this.childNodes[this.childNodes.length - 1] :
>+                                             this.childNodes[0];
>+            if (aNewTab == requestedTab)
>+              return;
>+          }

can you add a small logic comment to explain this code path and meanings of wrap and fallback so people does not have to mentally run the loop?

>+          var isTabFocused = false;
>+          try {
>+            isTabFocused =
>+              (document.commandDispatcher.focusedElement == this.selectedItem);

why the parenthesis, doesn't look like they are helping readability

>+          this.selectedItem = aNewTab;
>+          if (isTabFocused) {
>+            (aNewTab.appTab || aNewTab).focus();
>+          }
>+          else if (this.getAttribute("setfocus") != "false") {

!= "false" looks strange... should be == "true" ? this is a bool and a wongly set setfocus="" does not look like a positive value.

>+            document.commandDispatcher.advanceFocusIntoSubtree(aNewTab);
>+            

trrrrailing spaces

>+            // Make sure that the focus doesn't move outside the tabbox
>+            if (this.tabbox) {
>+              try {
>+                let el = document.commandDispatcher.focusedElement;
>+                while (el && el != this.tabbox)
>+                  el = el.parentNode;
>+                if (el != this.tabbox)
>+                  (aNewTab.appTab || aNewTab).focus();
>+              } catch(e) {
>+              }

no reason to  put the catch brace in a new line if you don't handle it.

again, a small focus logic comment before this code, for lazy devs like me, would be appreciated.


>+    <handlers>
>+      <handler event="dragstart" phase="capturing">
>+        this.style.MozUserFocus = '';
>+      </handler>

is capturing really needed?

>+        gBrowser.tabContainer.advanceSelectedTab(direction == 'ltr' ? -1 : 1, gBrowser.arrowKeysShouldWrap);

srly, would be cool if -1 and 1 would be named like DIR.BEFORE, DIR.AFTER

>+        var newTab = gBrowser.selectedTab;
>+        (newTab.appTab || newTab).focus();

you use this pretty commonly, an helper like this._focusTab(newTab)?

>+      <handler event="keypress" keycode="VK_HOME">
>+      <![CDATA[
>+        gBrowser.selectedTab = gBrowser.tabs[0];
>+        gBrowser.selectedTab.appTab.focus();
>+      ]]>
>+      </handler>

is it always guaranteed first tab is an appTab (you had a dummy tab right?)?
Add a comment please.

>+
>+      <handler event="keypress" keycode="VK_END">
>+      <![CDATA[
>+        var tabs = gBrowser.tabs;
>+        gBrowser.selectedTab = tabs[tabs.length - 1];
>+        gBrowser.selectedTab.focus();

And I guess it's impossible to have only appTabs but no normal tabs


>diff --git a/browser/base/content/test/browser_appTabs.js b/browser/base/content/test/browser_appTabs.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/test/browser_appTabs.js
>@@ -0,0 +1,190 @@
>+function test() {

add a pd license (see http://www.mozilla.org/MPL/license-policy.html), it is shorter and still gives your test a license:
/*
 * Any copyright is dedicated to the Public Domain.
 * http://creativecommons.org/publicdomain/zero/1.0/
 */

>+  waitForExplicitFinish();
>+  registerCleanupFunction(cleanupTabs);
>+  
>+  var gen = testSequences();
>+  gen.next();
>+  

trailing spaces in both empty lines

This test should not grow more than what it is, oranges are behind the corner, I'd already prefer it being splitted into 2 based on some logic.

>diff --git a/browser/components/sessionstore/src/nsSessionStore.js b/browser/components/sessionstore/src/nsSessionStore.js

>   onTabClose: function sss_onTabClose(aWindow, aTab) {
>+    // skip AppTabs, as these are managed separately in preferences

this comment is still unclear about where those are managed (in preferences?), and it should also be in onTabAdd.

>     for (var t = 0; t < newTabCount; t++) {
>-      tabs.push(t < openTabCount ? tabbrowser.tabs[t] : tabbrowser.addTab());
>+      tabs.push(t < openTabCount ? tabbrowser.tabs[t + numAppTabs] : tabbrowser.addTab());
>       // when resuming at startup: add additionally requested pages to the end
>       if (!aOverwriteTabs && root._firstTabs) {
>         tabbrowser.moveTabTo(tabs[t], t);

Hm, I don't recall how you manage indices in moveTabTo, looks like t here should be t + numAppTabs?

Gobally it is better and smaller scoped than previous iteration, I have an hard time to comment about this being the best approach for browser (gavin is the guru there), but sounds a quite reasoneable and pondered approach. f+ for me, will keep looking at next versions if you wish.
Something is missing between "Ah - I stand corrected" and "This continues with the approach of using a separate container of AppTabs that is synced to hidden tabs in the main tab container"...
The missing part is in comment 39:

"Regardless, this would require manually maintaining the pixel position of all AppTabs and the normal tab container - through adding, removing, moving, and loading AppTabs."
Sounds like that would be one rather simple method that you'd call when adding, removing and moving app tabs. What's missing, then, is the part that explains why that's a problem.
Also from comment 39:

"We could always explore this further some time in the future, but I've already
spent too much time exploring alternate approaches that haven't yielded any
satisfactory results. For now, I'll continue with my existing approach, as
that's the only one thats currently working."

In other words: We've already spent too much time bikeshedding, and we're running out of time before beta 1. The current approach is known to work, solves all issues brought up, and has code - which can't be said for the absolutely-positioned approach.
When making a change like this to tabbrowser, "let's take this for now, it kinda works" is inappropriate, especially before beta 1, as this is going to have significant add-on impact.
(In reply to comment #53)
> When making a change like this to tabbrowser, "let's take this for now, it
> kinda works" is inappropriate, especially before beta 1, as this is going to
> have significant add-on impact.

It doesn't kinda work - it does work. I wouldn't be putting it up for review if I thought it was a half-measure.
Attached patch Patch v2Splinter Review
Resolves Marco's comments, and fixes the exception thrown in nsSessionStore.

I still have those 2 tests giving me trouble (browser_bug521216.js and browser_bug500328.js - the failure of which causes other tests to fail). But they also show up on my local build without this patch. Doing another Try build to see what happens.


(In reply to comment #48)
> should the previous relatedTo code be handled for appTabs too?
 
Do you mean when adding an AppTab? No, because an AppTab can't be moved to directly after a normal tab (relatedness just means the new tab will be moved to be directly after its related tab). And it doesn't make sense to add an AppTab from an AppTab (which is what would make an AppTab related to an AppTab). But looking at that, the current patch allows normal tabs to be related to an AppTab (ie, moved to directly after an AppTab) - which in practice can only happen when its the last AppTab, so I've removed that possibility (for consistancy).


> imho, you use appTab name too often for vars, in this code appTab can be a
> bool, in the above one is a tab... what about calling this param isAppTab for
> better readability?

Agreed.

> >+      <property name="normalTabs" readonly="true">
> 
> I don't like the "normal" prefix, normal regarding to what? maybe visibleTabs,
> pageTabs?

"normal" as in tabs that are not AppTabs. visibleTabs/pageTabs makes less sense to me. Open to other suggestions.

> what is this for? I can only find one usage in browserGlue and that
> sounds like being achievable in other ways (.tabs.length - .appTabs.length).

It's just a convenience API. Its doable via other means, but its annoying to do so.


> Actually I think .tabs should return "normal" tabs and there should be a
> .allTabs, but I guess that would break compatibility :(

Yea, that's on purpose to keep compatibility, and because in most cases consumers won't (or shouldn't) care about the difference.

> It is also easy and short to implement for callers (and you could do the same
> in your code) using array comprehensions:
> 
> let commonTabs = [ t for each (t in browser.tabs) if (!t.appTab) ];
> or
> let commonTabs = [ t for each (t in browser.tabs) if
> (browser.appTabs.indexOf(t) != -1) ];
> 
> imo you could even avoid the method and add a comment above "tabs" with one of
> these examples.

I could, but in addition to being inconvient, there would be a performance penalty every time some consumer wanted just normal tabs (FWIW, tabbrowser.browsers does basically the same, for the same reasons).


> .realTab exists only for appTabs, would not this fire a js warning? should it
> be ("realTab" in aTab) instead?

Good catch.


> new code in this file uses let, btw ask gavin what to use and be consistent.

I only saw a couple of instances of new code using let - and they were in blocks. Gavin?


> >+          while (aNewTab.hidden || aNewTab.disabled) {
> >+            aNewTab = aFallbackDir == -1 ? aNewTab.previousSibling : aNewTab.nextSibling;
> >+            if (!aNewTab && aWrap)
> >+              aNewTab = aFallbackDir == -1 ? this.childNodes[this.childNodes.length - 1] :
> >+                                             this.childNodes[0];
> >+            if (aNewTab == requestedTab)
> >+              return;
> >+          }
> 
> can you add a small logic comment to explain this code path and meanings of
> wrap and fallback so people does not have to mentally run the loop?

Most of this code was actually copied from tabbox.xml - which I am loath to do. I've since figured out a much cleaner way to solve the focus issue, so this method override has been removed (and various new code relating to focus has changed).
 
> >+    <handlers>
> >+      <handler event="dragstart" phase="capturing">
> >+        this.style.MozUserFocus = '';
> >+      </handler>
> 
> is capturing really needed?

I have no idea. Its what other code in tabbrowser uses, so I kept it.


> >+        gBrowser.tabContainer.advanceSelectedTab(direction == 'ltr' ? -1 : 1, gBrowser.arrowKeysShouldWrap);
> 
> srly, would be cool if -1 and 1 would be named like DIR.BEFORE, DIR.AFTER

Agreed - but this is what all code in tabbrowser and tabbox uses.


> >+      <handler event="keypress" keycode="VK_HOME">
> >+      <![CDATA[
> >+        gBrowser.selectedTab = gBrowser.tabs[0];
> >+        gBrowser.selectedTab.appTab.focus();
> >+      ]]>
> >+      </handler>
> 
> is it always guaranteed first tab is an appTab (you had a dummy tab right?)?
> Add a comment please.

It is in this case - because this is in the AppTab binding. So for that code to be triggered, there must be at least one AppTab, so its guarenteed the first tab is an AppTab. Previous patch had a dummy tab in the AppTab container, but I've change that so its no longer needed. 


> And I guess it's impossible to have only appTabs but no normal tabs

Correct.



> This test should not grow more than what it is, oranges are behind the corner,
> I'd already prefer it being splitted into 2 based on some logic.

Done - I've moved the focus-related parts into their own test.
Attachment #450449 - Attachment is obsolete: true
Attachment #451245 - Flags: review?(gavin.sharp)
Attachment #451245 - Flags: feedback?(mak77)
Attachment #450449 - Flags: review?(gavin.sharp)
(In reply to comment #55)
> > I don't like the "normal" prefix, normal regarding to what? maybe visibleTabs,
> > pageTabs?
> 
> "normal" as in tabs that are not AppTabs. visibleTabs/pageTabs makes less sense
> to me. Open to other suggestions.

"default", "standard", "basic"?
(In reply to comment #54)
> (In reply to comment #53)
> > When making a change like this to tabbrowser, "let's take this for now, it
> > kinda works" is inappropriate, especially before beta 1, as this is going to
> > have significant add-on impact.
> 
> It doesn't kinda work - it does work. I wouldn't be putting it up for review if
> I thought it was a half-measure.

The question never was whether you managed to produce something usable, but whether it's the implementation we want going forward.
Comment on attachment 451245 [details] [diff] [review]
Patch v2

We need to seriously look into the position:fixed approach at least. It's not clear to me how far you got, more comments in this regard or even a WIP patch would have been helpful. I'll try to put up a basic patch later today.
Attachment #451245 - Flags: review-
Okay - let's get this sorted, and let's agree as a first principle that everyone wants the best thing for the code and our users. The discussion here needn't get personal (it isn't, yet, but these things can go that way.)

I absolutely agree with Blair that we want to get this *feature* in front of users ASAP, because we think that there will be interaction and usability feedback to come from it that we want to have time to respond to. Having said that, Dao's right that changes to the tab model will likely impact a good number of addons, so we don't need it to be perfect, but we shouldn't do something if we know we're going to change it.

Is it clearly the case that a separate container is wrong? Dao, you raised concerns up above around potential problem areas that Blair says he's addressed. If it's architecturally the wrong play, then we're going to have to change the patch, but if you're just worried that it might lead to unforseen bugs down the road, without feeling that it is fundamentally a weaker approach, then I think we can keep going as is, and understand that we'll need to expect fallout.

Blair - you put a lot of time into getting the container to work, so there will be a certain natural reticence to gutting the patch. Understanding that, what do you think the impact would be if you *did* have to rebuild around a single tab container, with some tabs getting classed to have different styling and positioning, as well as some different behaviours. Is this an hour's rewrite, a day's, a month's?

Gavin - I don't need you to break ties, but I would welcome your input here as a browser peer, even if it's just to say "either could work, here's the questions/concerns I have..."
(In reply to comment #59)
> Is it clearly the case that a separate container is wrong? Dao, you raised
> concerns up above around potential problem areas that Blair says he's
> addressed. If it's architecturally the wrong play, then we're going to have to
> change the patch, but if you're just worried that it might lead to unforseen
> bugs down the road, without feeling that it is fundamentally a weaker approach,
> then I think we can keep going as is, and understand that we'll need to expect
> fallout.

Blair addressed some of them(*) by reimplementing features and duplicating a bunch of code -- I was concerned about the issues exactly because I foresaw this solution. It's architecturally wrong and fundamentally weaker if we can avoid it. I think we can avoid it.

*: I haven't reviewed the patch closely enough to speak about all the raised issues.
Attached patch early patch (obsolete) — Splinter Review
This is unlikely to be complete, although I don't see serious issues right now.

Note that I'm intentionally not touching session restore. I don't think it makes sense to prevent these tabs from being restored as long as they aren't stored separately. Incidentally, I'm not sure storing them separately makes sense.
> >+          if (aTab.realTab)
> >+            aTab = aTab.realTab;
> 
> .realTab exists only for appTabs, would not this fire a js warning? should it
> be ("realTab" in aTab) instead?

Not that it matters now, but no, JS wouldn't warn there.
(In reply to comment #59)
> Blair - you put a lot of time into getting the container to work, so there will
> be a certain natural reticence to gutting the patch. 

I had been worried about this too - so about half my time has been spent looking into alternative approaches, such as the ones brought up by Dao, Gavin, and Marco, but also before coming to my current approach. And I still believe my current approach is the lesser of the various evils.

> Understanding that, what
> do you think the impact would be if you *did* have to rebuild around a single
> tab container, with some tabs getting classed to have different styling and
> positioning, as well as some different behaviours. Is this an hour's rewrite, a
> day's, a month's?

The basic foundation probably wouldn't take that long (and Dao has an initial patch that does this, now). My main problem is that I think its fragile, and the issues that I think it was cause in the future (such as drag&drop). But on top of that, I just have a really bad gut feeling about that specific approach.

(In reply to comment #60)
> Blair addressed some of them(*) by reimplementing features and duplicating a
> bunch of code

The only code duplication in my latest patch are the event handlers on the AppTab XBL binding. Not sure what you mean by reimplementing features - various methods have small changes, but nothing has been rewritten or reimplemented.

(In reply to comment #61)
> Note that I'm intentionally not touching session restore. I don't think it
> makes sense to prevent these tabs from being restored as long as they aren't
> stored separately. Incidentally, I'm not sure storing them separately makes
> sense.

I wanted to explicitly keep them out of session restore because AppTabs aren't meant to be related to any session - they're permanent, not transitive like normal tabs. If you start a new session, lose your old one, or have session restore disabled, AppTabs should still persist. Similarly, AppTabs should be persisted across windows (ie, all windows have the same AppTabs, always) - session store works on a per-window basis.
(In reply to comment #63)
> I wanted to explicitly keep them out of session restore because AppTabs aren't
> meant to be related to any session - they're permanent, not transitive like
> normal tabs. If you start a new session, lose your old one, or have session
> restore disabled, AppTabs should still persist. Similarly, AppTabs should be
> persisted across windows (ie, all windows have the same AppTabs, always) -
> session store works on a per-window basis.

This seems to ignore that users will actually browse in these tabs and would lose data without session restore.
(In reply to comment #64)
> This seems to ignore that users will actually browse in these tabs and would
> lose data without session restore.

Surely that defeats the point of an application tab? After all, wasn't there a discussion about removing navigation bar et al within application tabs*.

* Over in the UX group IIRC
(In reply to comment #65)
> (In reply to comment #64)
> > This seems to ignore that users will actually browse in these tabs and would
> > lose data without session restore.
> 
> Surely that defeats the point of an application tab? After all, wasn't there a
> discussion about removing navigation bar et al within application tabs*.

Exactly - the whole point is that you *don't* browse in these tabs. If you try to navigate away from the site (eg, by clicking a link), it will open a new tab. If you browse within the site (ie, within the same subdomain), it should always reset to the original URL when the AppTab is loaded again.
This is the groundwork I think we need in tabbrowser. Anything else can (or should, where it's not fleshed out yet) be taken care of subsequently.

To play around with this, execute these commands in the error console:

top.opener.gBrowser.pinTab(top.opener.gBrowser.tabContainer.lastChild)
top.opener.gBrowser.unpinTab(top.opener.gBrowser.tabContainer.firstChild)
Attachment #451287 - Attachment is obsolete: true
Attachment #451526 - Flags: review?(gavin.sharp)
(In reply to comment #66)
> (In reply to comment #65)
> > (In reply to comment #64)
> > > This seems to ignore that users will actually browse in these tabs and would
> > > lose data without session restore.
> > 
> > Surely that defeats the point of an application tab? After all, wasn't there a
> > discussion about removing navigation bar et al within application tabs*.
> 
> Exactly - the whole point is that you *don't* browse in these tabs. If you try
> to navigate away from the site (eg, by clicking a link), it will open a new
> tab. If you browse within the site (ie, within the same subdomain), it should
> always reset to the original URL when the AppTab is loaded again.

So that's the idea, but it still doesn't mitigate the dataloss concern. If you crash with gmail in an app tab, you very likely want the text you just typed to be restored.
I think it's important to differentiate between the handling and usage of AppTabs compared to normal tabs. As such hacking a few tabs so as that they remain small and behave as intended is a far cry from building a long promoted feature. At one point there was in issue raised about AppTabs breaking extensions, the question here is so what? AppTabs are a new feature upon which new extensibility will be built upon. As such, at this moment in time, based upon the comments in here and the comments that lead up this feature, I find Blair's approach to be the more elegant thus making a better foundation for the future of AppTabs.
Paul, I don't think you understand the approaches and their implications. Blair's app tabs are fake tabs in a separate container but normal tabs behind the scene. If by "handling and usage" you mean points such as in comment 6, then no, we don't want to differentiate all the way, but only in some cases, which is why it makes sense to implement the differences based on a common foundation, rather than building the common foundation twice. If you're referring to session restore, then that's a separate question entirely.
Comment on attachment 451526 [details] [diff] [review]
patch using position:fixed

>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml

>+      <property name="_numPinnedTabs" readonly="true">

Could use: onget="return this.tabs.reduce(function (a,b) a+(b.pinned?1:0), 0);"

Some other issues:
- Closing a pinned tab with this patch results in an empty space (either removeTab needs to call _positionPinnedTabs if aTab is pinned, or we should disallow closing pinned tabs?).
- session restore doesn't restore the "pinned" state - it probably should, right?
Attachment #451526 - Flags: review?(gavin.sharp) → review+
It would be good to get some tests for the moveTabTo clamping as well (e.g. for new tabs from _blank-targeted links in a pinned tab).
- tab drag indicator should take into account the clamping (it doesn't currently)
Comment on attachment 451245 [details] [diff] [review]
Patch v2

I'm hopeful that we can get by with attachment 451526 [details] [diff] [review]'s simpler approach, but we may have to revisit if it ends up not being sufficient.
Attachment #451245 - Flags: review?(gavin.sharp)
Attachment #451245 - Flags: feedback?(mak77)
(In reply to comment #71)
> >+      <property name="_numPinnedTabs" readonly="true">
> 
> Could use: onget="return this.tabs.reduce(function (a,b) a+(b.pinned?1:0), 0);"

This would do more work, as it wouldn't abort when b.pinned is false.

> - Closing a pinned tab with this patch results in an empty space (either
> removeTab needs to call _positionPinnedTabs if aTab is pinned, or we should
> disallow closing pinned tabs?).

I'll go with _positionPinnedTabs until the interaction model is more refined. I think the primary motivation for disallowing closing was the home tab, but that could be special-cased.

> - session restore doesn't restore the "pinned" state - it probably should,
> right?

I'll leave this for a follow-up, since Blair believes session restore should ignore these tabs entirely.

(In reply to comment #72)
> It would be good to get some tests for the moveTabTo clamping as well (e.g. for
> new tabs from _blank-targeted links in a pinned tab).

I'll write a basic test for tab indices after pinTab, unpinTab and moveTabTo.

(In reply to comment #73)
> - tab drag indicator should take into account the clamping (it doesn't
> currently)

I think we'll actually want this later on in order to allow unpinning tabs by dragging them away.
http://hg.mozilla.org/mozilla-central/rev/8d0ea220e3cb
Assignee: bmcbride → nobody
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Component: General → Tabbed Browser
Flags: in-testsuite+
QA Contact: general → tabbed.browser
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a6
Assignee: nobody → dao
So, if I may ask, this is AppTabs implementation?
(In reply to comment #77)
> So, if I may ask, this is AppTabs implementation?

Not yet, more work is needed.
If it's not complete why is it marked "resolved" ?
Subsequent work will happen in separate bugs.
Depends on: 574487
Depends on: 575560
For which milestone are AppTabs targeted?
Er, I thought ApépTabs will be chromeless and persistent across sessions.
(In reply to comment #82)
> Er, I thought ApépTabs will be chromeless and persistent across sessions.

Are you talking to yourself? In any case, yes, that is the plan.
Depends on: 577039
Depends on: 577042
Blocks: 577092
Depends on: 577410
Depends on: 578061
Blocks: 578796
Blocks: 578806
No longer blocks: 578796
No longer blocks: 578806
Depends on: 578806
Depends on: 579629
Depends on: 579683
Depends on: 579373
Depends on: 579776
Depends on: 579852
Depends on: 580244
No longer depends on: 580244
Depends on: 580257
Depends on: 579869
Depends on: 582954
No longer depends on: 582954
Depends on: 579482
Can someone please add a dependency for bug #583299 ?

Thanks in advance.
Depends on: 583299
No longer depends on: 583299
Depends on: 588011
Depends on: 577820
No longer depends on: 577039
Depends on: 594201
Depends on: 594378
No longer depends on: 579482
Verified fixed on Mozilla/5.0 (Windows NT 6.1; rv:6.0a1) Gecko/20110504 Firefox/6.0a1
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.