Closed Bug 347930 Opened 13 years ago Closed 10 years ago

Tab strip should be a toolbar instead

Categories

(Firefox :: Tabbed Browser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.7a4

People

(Reporter: pkasting, Assigned: dao)

References

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

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 24 obsolete files)

109.40 KB, patch
Details | Diff | Splinter Review
From bug 347844 comment 7:

"The tab strip should become the 'Tab Toolbar' and be toggleable on and off, and customizable, in the same way other toolbars are.  Currently it would have a large flexible 'Tabs' section, and an 'All Tabs' button at the end.  Maybe 'New Tab', 'Close Tab', etc. buttons qould be other choices in the menu o' buttons of what could go here."

This allows a lot of things:
(1) Addition of a 1.0/1.5-style "close tab" button without a pref
(2) Placement of the New Tab button (or an "undo close tab" button) on the tab strip itself
(3) Removal of the All Tabs menu
(4) Permanent hiding of the tab strip
...all without touching preferences, in a way that very much matches user expectations from the other toolbars.  We can still style the tab strip the same way, and we can still auto-hide it (under a pref) the same way.

*** This bug has been marked as a duplicate of 150379 ***
No longer blocks: 347844
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
In the interest of not getting pulled in to tangential topics, I'm unduping this from bug 150379.

Reasons:
* That bug is mainly about collapsing/hiding the tab bar, and when and how that should happen.  I perceive the WONTFIX on that bug to be centered around this issue.  I'm not proposing that a tab bar MUST show or hide permanently the same way other toolbars do; I'm not proposing to change our behavior at all.
* The screenshots and dates in that bug make it clear that it is a Seamonkey bug that was morphed to be on the "Firefox" component.  This is a true Firefox bug.
* That bug fails to deal with the idea of other buttons, such as "new tab", "All Tabs", etc. on the bar, whose presence in the current product provide a much better motivation for being able to customize the tab bar.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to comment #2)
> such as "new tab",

Heh. You said "new tab." Bug 186865 rides again!
Blocks: 186865
I disagree, bug 150379 comment 15 shows that that bug was reopened to request customization of the tab bar and was moved to Firefox (then Phoenix). The next comment shows that the module owners for Phoenix said no to it.
Anyway, bug 150379 is old/dead and doesn't really provide relevant information. FWIW, it could be duped against this bug.
Component: Toolbars → Tabbed Browser
This is interesting; I'm also mulling ways that we could do more with gestures such as drags, clicks and drops (perhaps with modifier keys) to make the tabstrip a little more dynamic and responsive to different interactions. Definitely something to think about / experiment with for Firefox 3.
Keywords: uiwanted
Target Milestone: --- → Firefox 3 alpha1
QA Contact: toolbars → tabbed.browser
Hardware: PC → All
Target Milestone: Firefox 3 alpha1 → ---
Version: 2.0 Branch → Trunk
I would like to see the tab strip implemented much like the current <Bookmarks Toolbar Item> - an auto-expanding widget that can be positioned anywhere on any toolbar.  This <Tab Strip Item> would be comprised of everything between the left and right overflow buttons inclusively leaving <All Tabs> to be implemented as a separate toolbar button.

With this, the current default interface could be reimplemented with little if any visual or functional changes.  However, for those that prefer, the <Tab Strip Item> could move above the <Location Bar> and navigation buttons.
Duplicate of this bug: 439786
I'll take a look at how feasible this is.
Assignee: nobody → dao
Blocks: 457187
Status: REOPENED → ASSIGNED
Depends on: 436304
No longer blocks: 186865
Blocks: 130061
Keywords: uiwanted
Blocks: 456984
Attached patch WIP (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #342180 - Attachment is obsolete: true
Flags: blocking-firefox3.1?
Attached patch patch v2 (obsolete) — Splinter Review
Event listeners for Tab* events added to tabbrowser (rather than tabContainer) need special treatment, as the tabbrowser doesn't wrap the tabs anymore.
Attachment #342352 - Attachment is obsolete: true
Attached patch patch v2.1 (obsolete) — Splinter Review
some gnomestripe issues fixed
Attachment #342369 - Attachment is obsolete: true
Attached patch patch v2.2 (obsolete) — Splinter Review
Attachment #342475 - Attachment is obsolete: true
Attached patch patch v2.3 - ready for review (obsolete) — Splinter Review
updated to trunk (but still depends on bug 436304), more theme fixes
Attachment #342580 - Attachment is obsolete: true
Bugs I've found with the try server build:

* Dragging and dropping the tab overflow menu in to the customize toolbar window seems to generate an empty space in the window and it can never properly be dragged back on to the window

* Dragging and dropping "flexible space" after the tabs and then putting the new tab button on the left side of the flexible space when there are only 1 or 2 tabs open, there ends up being a large distance between the last tab and the newtab button, it sort of is just somewhere in the middle of the tab toolbar on its down

* A separate bug, but it appears the newtab graphic needs updating as the right side of it looks weird when it's placed elsewhere, perhaps the tab overflow button as well but I can't access that right now

* Oh also also the tab toolbar doesn't reset itself when you click "resotre defaults" in the toolbar customize window.

Otherwise looking awesome :D, good work!
Blocks: 457651
(In reply to comment #17)
> * Dragging and dropping the tab overflow menu in to the customize toolbar
> window seems to generate an empty space in the window and it can never properly
> be dragged back on to the window

works for me

> * A separate bug, but it appears the newtab graphic needs updating as the right
> side of it looks weird when it's placed elsewhere, perhaps the tab overflow
> button as well but I can't access that right now

By "elsewhere" you mean with the flexible space added, right? That's a use case I haven't considered yet.

> * Oh also also the tab toolbar doesn't reset itself when you click "resotre
> defaults" in the toolbar customize window.

works for me
Ahh, sorry, just tried again with a clean profile, I got confused because the tab overflow seems to have been replaced with the "all tabs" button. Yes, the only issue that seems normal is inputting flexible space to try and get the new tab button to be right of the the last tab, causing the 2 bugs I've already mentioned.

There are some 'odd' testcases as well though, if you drag the bookmark toolbar items down to the tab bar right of the tabs, and open lots of tabs, eventually the whole bookmark toolbar items gets pushed off the screen. 

Also if you drag the address bar or the tabs on to any bar, so they are on the same toolbar and the address bar is right of the tabs and keep opening lots of tabs, the address bar probably gets squeezed too small, but I imagine this is a different bug.
The removal of some anonid attributes could make it awkward for extension developers; For instance I use tabContextMenu when modifying the context menu and will no longer be able to do

var tabContextMenu = document.getAnonymousElementByAttribute(gBrowser, "anonid", "tabContextMenu");

> -          <xul:menupopup anonid="tabContextMenu" onpopupshowing="this.parentNode.parentNode.parentNode.updatePopupMenu(this);">
> +          <xul:menupopup onpopupshowing="gBrowser.updatePopupMenu(this);">
(In reply to comment #20)
> The removal of some anonid attributes could make it awkward for extension
> developers; For instance I use tabContextMenu when modifying the context menu
> and will no longer be able to do
> 
> var tabContextMenu = document.getAnonymousElementByAttribute(gBrowser,
> "anonid", "tabContextMenu");

Leaving the anonid there wouldn't help, as the context menu is neither a child of gBrowser nor anonymous when the tab strip is moved to a toolbar.
Attachment #342721 - Attachment is obsolete: true
Attachment #342908 - Attachment description: patch v2.4 - vista tabstrip styling fixed → patch v2.4 - vista tabstrip styling and fullscreen autohiding fixed
(In reply to comment #21)
> (In reply to comment #20)
> > The removal of some anonid attributes could make it awkward for extension
> > developers; For instance I use tabContextMenu when modifying the context menu
> > and will no longer be able to do
> > 
> > var tabContextMenu = document.getAnonymousElementByAttribute(gBrowser,
> > "anonid", "tabContextMenu");
> 
> Leaving the anonid there wouldn't help, as the context menu is neither a child
> of gBrowser nor anonymous when the tab strip is moved to a toolbar.

So from the latest patch the context menu is remaining as an anonymous node, maybe that is changing but regardless I agree there should be a fairly simple way for extensions to get an easy reference to it (even if that way is changing for 3.1, but if it is it needs to be documented).

I also really dislike the repeated use of |gBrowser| throughout this. Relying on what is in the window scope just seems wrong to me, is there some big perf benefit of it at all?
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > The removal of some anonid attributes could make it awkward for extension
> > > developers; For instance I use tabContextMenu when modifying the context menu
> > > and will no longer be able to do
> > > 
> > > var tabContextMenu = document.getAnonymousElementByAttribute(gBrowser,
> > > "anonid", "tabContextMenu");
> > 
> > Leaving the anonid there wouldn't help, as the context menu is neither a child
> > of gBrowser nor anonymous when the tab strip is moved to a toolbar.
> 
> So from the latest patch the context menu is remaining as an anonymous node,

It becomes non-anonymous when the strip is moved to a toolbar.

> maybe that is changing but regardless I agree there should be a fairly simple
> way for extensions to get an easy reference to it (even if that way is changing
> for 3.1, but if it is it needs to be documented).

Absolutely, I'm thinking of tabbrowser.tabContextmenu.

> I also really dislike the repeated use of |gBrowser| throughout this. Relying
> on what is in the window scope just seems wrong to me, is there some big perf
> benefit of it at all?

It's not about perf, stuff like this.parentNode.parentNode.parentNode just doesn't work anymore when "this" is somewhere else in the document.
Attachment #342908 - Attachment is obsolete: true
(In reply to comment #24)
> > I also really dislike the repeated use of |gBrowser| throughout this. Relying
> > on what is in the window scope just seems wrong to me, is there some big perf
> > benefit of it at all?
> 
> It's not about perf, stuff like this.parentNode.parentNode.parentNode just
> doesn't work anymore when "this" is somewhere else in the document.

Doesn't work for anything outside of tabbrowser and tabbrowser-tabbox sure, but the majority of uses of gBrowser seem to be in those. For the other cases it would seem to be logical to use a single field pointing to the tabbrowser.
(In reply to comment #26)
> (In reply to comment #24)
> > > I also really dislike the repeated use of |gBrowser| throughout this. Relying
> > > on what is in the window scope just seems wrong to me, is there some big perf
> > > benefit of it at all?
> > 
> > It's not about perf, stuff like this.parentNode.parentNode.parentNode just
> > doesn't work anymore when "this" is somewhere else in the document.
> 
> Doesn't work for anything outside of tabbrowser and tabbrowser-tabbox sure, but
> the majority of uses of gBrowser seem to be in those.

Which majority are you referring to? I'm referring to tabbrowser-strip / mStrip and everything inside of it that gets moved to the toolbaritem.

What we could do is let the tabbrowser add a self-reference to mStrip, which everything inside of it could use. But trying to avoid the use of gBrowser in browser code really seems moot to me.
Attached patch patch v2.6 (obsolete) — Splinter Review
There was one use of gBrowser that was obviously unnecessary... fixed that.
Attachment #343024 - Attachment is obsolete: true
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #24)
> > > > I also really dislike the repeated use of |gBrowser| throughout this. Relying
> > > > on what is in the window scope just seems wrong to me, is there some big perf
> > > > benefit of it at all?
> > > 
> > > It's not about perf, stuff like this.parentNode.parentNode.parentNode just
> > > doesn't work anymore when "this" is somewhere else in the document.
> > 
> > Doesn't work for anything outside of tabbrowser and tabbrowser-tabbox sure, but
> > the majority of uses of gBrowser seem to be in those.
> 
> Which majority are you referring to? I'm referring to tabbrowser-strip / mStrip
> and everything inside of it that gets moved to the toolbaritem.

Essentially all the changes inside the content section of tabbrowser seem to be about switching to using gBrowser for instance.
Yes, and it's done because this.parentNode.parentNode[...] doesn't work anymore for these nodes (as soon as they are moved to the toolbaritem).
My apologies, I hadn't fully appreciated what this patch was doing because I didn't think it was either possible or sensible to detach anonymous content and insert it into the normal DOM tree. I spoke with bz over IRC and he agrees and said that we shouldn't be doing this, while it may work currently they've considered making it not work and may still do so in a future security release.
Well, "shouldn't be doing this" is an unsatisfying response. It's possible that I've missed something, but so far it seems to me that exploiting this "security hole", which I've clearly seen and used as a feature, is the only way to fix this bug in a backwards-compatible way and without ripping apart tabbrowser.
I haven't thought about it too deeply, but my first guess would be just to make the tab strip its own binding or plain XUL within tabs-toolbaritem. It appears to be essentially what you already have but saves you having to use startup code to rip the tab strip out of tabbrowser and put it into the tabs-toolbaritem.
The tab strip can't just live within a toolbaritem, because 1) tabbrowser needs access to it even if the toolbaritem is removed, 2) tabbrowser is constructed earlier than the toolbars and 3) because of the way toolbaritems are attached to toolbars (they are cloned). These are only the problems that I've already ran into.
updated tryserver build:

https://build.mozilla.org/tryserver-builds/2008-10-15_08:09-dgottwald@mozilla.com-try-224473da0bc/

I changed the id of the toolbar item, so if you used a previous build, it's possible that you need to move the tabstrip back from the customization palette to the toolbar, or simply use "restore default set".
Few comments. If you move the show all tabs button to the bookmarks toolbar, it becomes too long and makes the bookmarks toolbar unnecessarily high. Is this button designed to adapt to the to the toolbar height? If so, why is it making the bookmarks menu even bigger. (Maybe it's just the size of the icon) and this isn't an issue. It happens with the new tab button also.

Also bug for you. This isn't really practical, but it breaks nonetheless. If you put the tabstrip on the bookmarks toolbar after the bookmarks toolbar item, there is some strange behavior that occurs when they all do not fit. The tabstrip behaves fine if a lot of tabs are opened, with the left and right arrows. However, some bookmarks are missing. Other times, there is the dropdown arrow, which can either not show up, or be misaligned.

I absolutely love how customizable this is. This is perfect.
(In reply to comment #36)
> Few comments. If you move the show all tabs button to the bookmarks toolbar, it
> becomes too long and makes the bookmarks toolbar unnecessarily high. Is this
> button designed to adapt to the to the toolbar height? If so, why is it making
> the bookmarks menu even bigger. (Maybe it's just the size of the icon) and this
> isn't an issue. It happens with the new tab button also.

The bookmarks toolbar has a small icon size by default. To get that while keeping the big icon size in the navigation toolbar, use "restore default set".
Blocks: 460228
Attached patch patch v2.7 (obsolete) — Splinter Review
Attachment #343036 - Attachment is obsolete: true
Attachment #343394 - Flags: review?(mconnor)
Will the patch allow to position the new-tab button near back/forward buttons as in firefox <=3.0?
What about theme's compatibility then?
(In reply to comment #39)
> Will the patch allow to position the new-tab button near back/forward buttons
> as in firefox <=3.0?

Yes.

> What about theme's compatibility then?

The new tab button has the same id as in Firefox 3.
I'm not sure if allowing any little (or big) thing to be placed on the tabbar is a good idea at all.

Anyway, this will change the UI so deep that I'm thinking it's a little late for 3.1, or?
(In reply to comment #41)
> I'm not sure if allowing any little (or big) thing to be placed on the tabbar
> is a good idea at all.

Toolbar customization always used to allow things that may not make sense. I don't see a problem with that, as long as these things don't break the browser and can be reverted easily.

> Anyway, this will change the UI so deep that I'm thinking it's a little late
> for 3.1, or?

It would probably have to be done for beta 2, which means that there are less than two weeks left to get this reviewed and landed.
(In reply to comment #35)
> updated tryserver build:
> 
> https://build.mozilla.org/tryserver-builds/2008-10-15_08:09-dgottwald@mozilla.com-try-224473da0bc/
> 
This build seems to break the tabs on every single third party theme that I tried it on, i.e. themes that do not have the tabbar and tabs coded identically to the default theme - that is a lot of themes. A number of extensions immediately broke as well.

If it is really necessary to go down this path, rather than trying to reinvent the wheel here, would it not be easier to just check this out for coding? http://totaltoolbar.mozdev.org/index.html This extension successfully allows customisation of the tabbar (and many other areas) and does so without, it would appear, breaking third party themes or major extensions.
(In reply to comment #34)
> The tab strip can't just live within a toolbaritem, because 1) tabbrowser needs
> access to it even if the toolbaritem is removed, 2) tabbrowser is constructed
> earlier than the toolbars and 3) because of the way toolbaritems are attached
> to toolbars (they are cloned). These are only the problems that I've already
> ran into.

Your code already handles all of the above by adding and removing the anonymous node to the toolbaritem. Can you not do the same with a real XUL <tabs> element created during the tabbrowser constructor? Still not exactly what I'd think of as ideal but it gets round the problem us abusing anonymous nodes.
With respect if indeed Frank Lion is right about this breaking a large number of 3rd party themes and extensions is this a wise course to take?
I just tried the 2008-10-15 trial build with one of my themes. No tab appears on the tabbar. When I create a new tab, the browser page becomes blank. If I middle-click a bookmark to create a new tab, the target replaces the previous page: in essence, the tab function is lost. 

I suspect a lot of extensions will be broken along with themes.

This amounts to a major change in the browser architecture. Such major changes should wait for a version change - i.e., Firefox 4. This is just too big a change for Firefox 3.1.

In the interim, if the devs want a customizable UI on the tabbar, the extension Total Toolbar - available at Mozdev - creates that without breaking themes. The TT code could easily be incorporated into Firefox.
(In reply to comment #44)
> Your code already handles all of the above by adding and removing the anonymous
> node to the toolbaritem. Can you not do the same with a real XUL <tabs> element
> created during the tabbrowser constructor? Still not exactly what I'd think of
> as ideal but it gets round the problem us abusing anonymous nodes.

I tried that, but the tabs element's binding was constructed asynchronously and seemingly too late for some other (tab)browser code.

(In reply to comment #45)
> With respect if indeed Frank Lion is right about this breaking a large number
> of 3rd party themes and extensions is this a wise course to take?

I don't think this would be a huge deal for extensions, except maybe Tab Mix Plus. The fact that I hardly had to touch browser code outside of tabbrowser.xml shows it: The tabbrowser API basically stays in place. In some few cases, we might have to introduce new and better extension hooks, like the proposed tabContextMenu property.

(In reply to comment #46)
> I just tried the 2008-10-15 trial build with one of my themes. No tab appears
> on the tabbar. When I create a new tab, the browser page becomes blank. If I
> middle-click a bookmark to create a new tab, the target replaces the previous
> page: in essence, the tab function is lost. 

I guess your theme is using custom bindings?

If it is, it might be a good idea to change this, as it's bound to be fragile. Gecko is now powerful enough that you shouldn't need custom markup for styling purposes. (We're now successfully removing the remaining XBL hacks for pinstripe.) If you do it now, you'll have less trouble next time we touch the tab bar.

If it isn't, things might not look right but shouldn't break. Fixing this should be trivial.

> This amounts to a major change in the browser architecture. Such major changes
> should wait for a version change - i.e., Firefox 4. This is just too big a
> change for Firefox 3.1.

I think 3.1 is the right target for this, since bug 455756 was added as a 3.1 feature. And I don't think deferring this till Firefox 4 would make extension and theme authors' lives easier. It would just shift the work that's needed anyway (and it would likely affect more authors).

The only problem is that we've made promises like <http://starkravingfinkle.org/blog/2008/07/extension-developers-dont-fear-firefox-31/>. I think there have been some wrong assumptions about what kind of features 3.1 would offer (even at that time, it wasn't _only_ about web-content facing features), and it's getting worse now that we've added a second beta with more user-facing changes.

> In the interim, if the devs want a customizable UI on the tabbar, the extension
> Total Toolbar - available at Mozdev - creates that without breaking themes. The
> TT code could easily be incorporated into Firefox.

The fact that the extension exists and works doesn't immediately make it ready to be incorporated into Firefox.
(In reply to comment #46)
> . . . in essence, the tab function is lost. 

I guess your theme is using custom bindings?

Nice guess, but like the default theme, my tab code has no xml binding. I am not sure that discovering the problem will be trivial. 

Worse, since this change will be coming during the beta part of the cycle, I and others will have little time to change our themes and extensions to adapt to your final code.
(In reply to comment #48)
> (In reply to comment #46)
> > . . . in essence, the tab function is lost. 
> 
> I guess your theme is using custom bindings?
> 
> Nice guess, but like the default theme, my tab code has no xml binding. I am
> not sure that discovering the problem will be trivial. 
> 
> Worse, since this change will be coming during the beta part of the cycle, I
> and others will have little time to change our themes and extensions to adapt
> to your final code.

If I may add...

After changing the new tab button the way it was, there will be complaining anyhow if we don't fix this. This is crucial, imo.
This patch should be landed in 3.1. There will be plenty of time before 3.1 final is released for extension and theme developers to tweak their code. I have been using the tryserver build and don't want to keep updating nightlys until this lands.

Because the new tab button on the toolbar was removed and forcefully relocated to the very right of the tabbar (which is out of the way and inconvenient) along with other major changes that happened to the tabbar (forcing it to be visible, closing the last tab closes the browser, etc) this needs to land so Firefox remains fully customizable. That's the point, right?
I tested the last test builds. I hoped it would be possible to do like Chrome and IE:
 _______  ___                                  ___
/ Tab 1 \/new\ <-      flexible space      -> /all\
|                                                 | 
but the result is:
 _______            ___                        ___
/ Tab 1 \          /new\ <- flexible space -> /all\
|                                                 | 

Is it possible to make the tab bar reduced to the minimum necessary space?
(In reply to comment #48)
> Nice guess, but like the default theme, my tab code has no xml binding. I am
> not sure that discovering the problem will be trivial. 

Would you send me (a link to) your theme via e-mail?

(In reply to comment #51)
> I tested the last test builds. I hoped it would be possible to do like Chrome
> and IE:
>  _______  ___                                  ___
> / Tab 1 \/new\ <-      flexible space      -> /all\
> |                                                 | 
> but the result is:
>  _______            ___                        ___
> / Tab 1 \          /new\ <- flexible space -> /all\
> |                                                 | 
> 
> Is it possible to make the tab bar reduced to the minimum necessary space?

Yes, that's possible, but the "minimum necessary space" would be determined quite aggressively. I.e. it would cause tab 1 to shrink to its minimum width (browser.tabs.tabMinWidth).
(In reply to comment #51)
> I tested the last test builds. I hoped it would be possible to do like Chrome
> and IE:
>  _______  ___                                  ___
> / Tab 1 \/new\ <-      flexible space      -> /all\
> |                                                 | 
> but the result is:
>  _______            ___                        ___
> / Tab 1 \          /new\ <- flexible space -> /all\
> |                                                 | 
> 
> Is it possible to make the tab bar reduced to the minimum necessary space?

A work around with this build seems to be to put many flexible spaces there (about 6 or 7) and you get the behavior your looking for.
In reply to comment #50
Obviously you have little idea or do you care in how much time it would take to "tweak"
a theme if this patch is landed and breaks 3rd party themes in a big way. As it appears it will and does.
(In reply to comment #54)
> In reply to comment #50
> Obviously you have little idea or do you care in how much time it would take to
> "tweak"
> a theme

The given patch updates all three default themes and should give a good idea of what's needed.
(In reply to comment #47)

> 
> I guess your theme is using custom bindings?
> 

Wrong guess. I'm using bindings for tab (and everywhere I can), and I don't have any issue with the tab functionality (I have other problems)...

> If it is, it might be a good idea to change this, as it's bound to be fragile.
> Gecko is now powerful enough that you shouldn't need custom markup for styling
> purposes. (We're now successfully removing the remaining XBL hacks for
> pinstripe.) If you do it now, you'll have less trouble next time we touch the
> tab bar.
> 

No it doesn't. I use bindings for the tab (since FF 0.9) because I have better performance, better control and my experience is that I'm running into less problems if the code changes. 
The binding I'm using is just to add a startcap and an endcap to the tab, to work with background images and have rounded tabs with gradients for the tab and shadows. If I'm not wrong the same approach used by pinstripe a time ago...
Let's imagine the follow situation: I take pinstripe with those boxes as basis for my theme. I use the boxes to style my tabs. Some day you decide you don't want more to style the tab with background images, or don't want rounded tabs anymore, or whatever. I'm pretty sure you would "clean up" the code removing those "useless" boxes...

Maybe Gecko is powerful now for styling like I want, but not powerful enough for my proposes and only now. We have problems you don't have, like compatibility. I have been working really hard to have one single file for Firefox 1.5 - 3.0 and Thunderbird 1.5 - 2.0 which works on all platforms, another problem you don't have.

And, as mentioned above, we don't have access to change the markup to fit our style proposes as you have. Not having these problems, with compatibility to several Firefox versions, cross platforms issues, being able to change the mark up, makes your job easier, but actually should make your job really more difficult, because you have to think about us ;-) (please??)

> If it isn't, things might not look right but shouldn't break. Fixing this
> should be trivial.
> 

I'm very happy to see you're willing to help us to fix the issues we could have because of this patch. Maybe we could open a thread on MZ for discuss the issues, after this lands?

> I think 3.1 is the right target for this, since bug 455756 was added as a 3.1
> feature. And I don't think deferring this till Firefox 4 would make extension
> and theme authors' lives easier. It would just shift the work that's needed
> anyway (and it would likely affect more authors).
> 

The problem is that the new tab button was treated as a toolbar button... The complains about this were more because the old "new tab" button disappears. IMHO a better approach could be offering this button as a "tab bar feature", with options on the pref pane. We could offer this and the others (close button, all tabs, and whatever) as preferences for the tab bar.
(In reply to comment #56)
As of 1.9.1 you could use border-image, and I doubt that custom bindings would give you a better performance. Anyway, you said this isn't a problem for you, so this discussion doesn't really belong here.

> I'm very happy to see you're willing to help us to fix the issues we could have
> because of this patch. Maybe we could open a thread on MZ for discuss the
> issues, after this lands?

Sure.
It turns out that my theme was not terribly incompatible with the try build. The problem was with the Total Toolbar extension. I did have to "tweak" the theme to make it function properly- several hours worth - and it would take a fair amount of work to make it backward compatible with Firefox 3.0.

I am hoping that when I start modifying my other themes, they will take less time. I think I will approach the modification process differently. I will need to, since I have ten more themes to work on.

Which of course is the problem: I retired four themes to give myself time to work on the others. I get lots of requests to make holiday themes like Halloween. I shudder, though, at the thought of updating more themes for this build and Firefox 4.
(In reply to comment #58)
> and it would take a
> fair amount of work to make it backward compatible with Firefox 3.0.

Have you considered using chrome overrides?
http://developer.mozilla.org/en/Chrome_Registration#appversion
Flags: in-litmus?
(In reply to comment #57)
> (In reply to comment #56)
> As of 1.9.1 you could use border-image, and I doubt that custom bindings would
> give you a better performance. Anyway, you said this isn't a problem for you,
> so this discussion doesn't really belong here.
> 

You can compare the "smooth tab scrolling" using my theme against the default theme. Maybe because, although the improvements now, border rendering stills slower as using simple background images. But, you're right, this discussion doesn't belong here.

I'm not against the bug at all. I find it interesting and "exquisit" (maybe too "exquisit"). But I just think it's too late for it. This will open a lot of bug possibilities, that we don't have time to treat properly. 

Just one example:

1. Open Firefox
2. Double click the tab bar to open a new tab
3. Close this tab using the "close tab" button inside the tab
4. Double click the tab bar to open a new tab

Nothing happens.

And there are also the appearance bugs, like everything odd it can happen if, for example, someone is using "large icons" and "icons and text" options for toolbar... We can't say that "We don't see a problem with that, as long as these things don't break the browser and can be reverted easily" to people complaining that Firefox is looking horrible, because they are doing something we've said they can do.

I'm thinking this bug will need more polish, not only for functionality and appearance, but as concept too. And I'm not talking here about third party themes and extensions issues.

For third party themes and extensions this will be a big headache, since authors are not expecting such significant changes on the UI at this moment.

In my case, I will maybe have to change the whole concept for the tabbar buttons I came for FF 3.0. You can see it here: 
http://img370.imageshack.us/my.php?image=tabbarbuttonsyn1.png

To do this I'm using the overflow property from tabbrowser-tabs. Now I can't know if the scrollbuttons are appearing or not, to skin the other buttons according to it. Do you have a suggestion?

(In reply to comment #59)
> Have you considered using chrome overrides?
> http://developer.mozilla.org/en/Chrome_Registration#appversion

Apropos those manifest flags, for people wanting to work with it, I've figured out a way to use them without having to duplicate any folder or file. Check it out: 
http://forums.mozillazine.org/viewtopic.php?f=18&t=906535
Attached patch patch v2.8 (obsolete) — Splinter Review
Attachment #343394 - Attachment is obsolete: true
Attachment #344377 - Flags: review?(mconnor)
Attachment #343394 - Flags: review?(mconnor)
(In reply to comment #60)
> I'm not against the bug at all. I find it interesting and "exquisit" (maybe too
> "exquisit"). But I just think it's too late for it. This will open a lot of bug
> possibilities, that we don't have time to treat properly. 
> 
> Just one example:
> 
> 1. Open Firefox
> 2. Double click the tab bar to open a new tab
> 3. Close this tab using the "close tab" button inside the tab
> 4. Double click the tab bar to open a new tab
> 
> Nothing happens.

fixed

> And there are also the appearance bugs, like everything odd it can happen if,
> for example, someone is using "large icons" and "icons and text" options for
> toolbar... We can't say that "We don't see a problem with that, as long as
> these things don't break the browser and can be reverted easily" to people
> complaining that Firefox is looking horrible, because they are doing something
> we've said they can do.

You could see that as a feature, and leave it to themes to support or not support a big tab toolbar. Or we could lock the tab toolbar in small icon mode from the start. No big deal either way.

> For third party themes and extensions this will be a big headache, since
> authors are not expecting such significant changes on the UI at this moment.
> 
> In my case, I will maybe have to change the whole concept for the tabbar
> buttons I came for FF 3.0. You can see it here: 
> http://img370.imageshack.us/my.php?image=tabbarbuttonsyn1.png

This is a very special case and probably not suited to prove that this will be a big headache for themes and extensions in general.

> To do this I'm using the overflow property from tabbrowser-tabs. Now I can't
> know if the scrollbuttons are appearing or not, to skin the other buttons
> according to it. Do you have a suggestion?

The overflow attribute could be added to the toolbar or the toolbar item.
(In reply to comment #53)
> (In reply to comment #51)
> > I tested the last test builds. I hoped it would be possible to do like Chrome
> > and IE:
> >  _______  ___                                  ___
> > / Tab 1 \/new\ <-      flexible space      -> /all\
> > |                                                 | 
> > but the result is:
> >  _______            ___                        ___
> > / Tab 1 \          /new\ <- flexible space -> /all\
> > |                                                 | 
> > 
> > Is it possible to make the tab bar reduced to the minimum necessary space?
> 
> A work around with this build seems to be to put many flexible spaces there
> (about 6 or 7) and you get the behavior your looking for.
This is not a very good workaround since tab are shrunk even if empty space is available. The best thing may be to set the tab list sized to the preferred sizeof the tabs but without empty space.

Other reports:
- If you add a normal or flexible space between "+" button and "all" button, the right border of the "+" button is not drawn.

- A big icon should be provided for "all tabs" since it can be moved to other toolbars.

- Since it is now possible to add/remove buttons from the tabbar, I think an optionnal "recently closed tabs button" would be a nice addition since it's a feature available in the history menu and related to tab browsing
Attached patch patch v2.9 (obsolete) — Splinter Review
popup handling and manual hiding of the toolbar fixed
Attachment #344377 - Attachment is obsolete: true
Attachment #344586 - Flags: review?(mconnor)
Attachment #344377 - Flags: review?(mconnor)
Depends on: 380960
> 
> The overflow attribute could be added to the toolbar or the toolbar item.

This can help. I guess the best could be the toolbar. Thank you.
new tryserver builds will follow
Attachment #344586 - Attachment is obsolete: true
Attachment #344643 - Flags: review?(mconnor)
Attachment #344586 - Flags: review?(mconnor)
I wrote a summary of what I think theme authors should be aware of:
https://developer.mozilla.org/User:Dao/Get_your_theme_ready_for_the_Tab_Toolbar
I see some odd behavior (on clean profile)

The constructor for tabbrowser-tabs binding run twice at start up.

also the constructor run one time when you open customize toolbar window, and one more time when you close it.

when you close the browser window the constructor run one last time AFTER the destructor.
(In reply to comment #68)
> I see some odd behavior (on clean profile)
> 
> The constructor for tabbrowser-tabs binding run twice at start up.
> 
> also the constructor run one time when you open customize toolbar window, and
> one more time when you close it.

I'm pretty sure this is the right behavior in terms of XBL. tabbrowser-tabs moves and thus the binding is reconstructed.

The first construction at start up could only be avoided if we did comment 44 (which already causes problems) *without* immediately appending the element somewhere (which would likely cause more problems).

> when you close the browser window the constructor run one last time AFTER the
> destructor.

This is because I call this.removeTabsFromToolbar() in the destructor, which seems unnecessary on a second thought. Will try to remove it.
Attachment #344643 - Attachment is obsolete: true
Attachment #344753 - Flags: review?(mconnor)
Attachment #344643 - Flags: review?(mconnor)
if you hide the tabbar 
when you start Customize... the tabbar is showing angain
other hidden toolbars are not shown

when you exit Customize the tabbar remain visible
(In reply to comment #71)
> if you hide the tabbar 
> when you start Customize... the tabbar is showing angain
> other hidden toolbars are not shown
> 
> when you exit Customize the tabbar remain visible

I think I already fixed this -- can you try it again with the latest tryserver build? Thanks.
Attachment #344753 - Attachment is obsolete: true
Attachment #344915 - Flags: review?(mconnor)
Attachment #344753 - Flags: review?(mconnor)
Depends on: 461631
Attached patch patch v2.12 (obsolete) — Splinter Review
updated to trunk
Attachment #344915 - Attachment is obsolete: true
Attachment #344927 - Flags: review?(mconnor)
Attachment #344915 - Flags: review?(mconnor)
No longer blocks: 460228
(In reply to comment #72)
> (In reply to comment #71)
> > if you hide the tabbar 
> > when you start Customize... the tabbar is showing angain
> > other hidden toolbars are not shown
> > 
> > when you exit Customize the tabbar remain visible
> 
> I think I already fixed this -- can you try it again with the latest tryserver
> build? Thanks.

OK. it's working

some other bug....

1. open Firefox with one tab
2. in Tools > Options > Tabs: check Hide the tab bar when only one web site is open (set browser.tabs.autoHide to true

tab bar is hidden as expected

3. in Tools > Options > Tabs: uncheck Hide the tab bar when only one web site is open (set browser.tabs.autoHide to false)

tab bar is still not visible !!!
the problem is in AutoHideTabbarPrefListener.toggleAutoHideTabbar

i also think that when use change the tab visibility from View > Toolbars or from toolbar context menu it also need to toggle browser.tabs.autoHide

other bug that i have seen is that the tab height is changing depending on the button combination (tested with default theme)

when we remove the All Tab button in customizing the tba tool bar height shrink from 26px to 23px. when you remove also the New Tab button and the height is 19px

In regular mode: with All Tab button and New Tab button the height is 26px
with New Tab button (remove All Tab button) when only one tab is open height is 23px with 2 or more tabs the height is 24px

without any button: with one tab the height is 22px with 2 tabs or more 24px
So, I played with this, and while I like where its going, I don't think we want to jam it into 3.1 at this stage.  I'll try to get a first-pass review done in the next couple of weeks.
Flags: wanted-firefox3.1?
Flags: wanted-firefox3.1-
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
(In reply to comment #76)
> So, I played with this, and while I like where its going, I don't think we want
> to jam it into 3.1 at this stage.  I'll try to get a first-pass review done in
> the next couple of weeks.

What about Bug 457187?  Is there going to be an alternative solution for removing the new tab button from the tab bar and putting it back on the tool bar?
(In reply to comment #77)
> (In reply to comment #76)
> > So, I played with this, and while I like where its going, I don't think we want
> > to jam it into 3.1 at this stage.  I'll try to get a first-pass review done in
> > the next couple of weeks.
> 
> What about Bug 457187?  Is there going to be an alternative solution for
> removing the new tab button from the tab bar and putting it back on the tool
> bar?

I agree. 3.1 cannot be released with the new tab button fixed at the right side. It's not user friendly at all and completely out of the way. Either the tabbar needs to be its own toolbar or I wouldn't mind having the new tab button float after the last tab, like Google Chrome.
I agree. I want to have new-tab button the old way - right near the back/forward buttons.
Please guys just vote on the bug instead of commenting "me too".
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
This was wanted-firefox3.1- which means it won't be there, look at bug 457187 for further consolation.
Is it for sure this won't be in for Firefox3.1? I don't see what bug 457187 gives for consolation, I don't see anything of consolation there.
(In reply to comment #81)
Sorry meant bug 457651, which is a blocker ;-)
I don't see anything of consolation in bug 457651.
A big part of the complaint in this bug is that the "new-tab" button has been moved and changed to a fixed position on the tab-bar. see bug 457187 for that which depends on this bug, bug 457651 is consolation for those that were complaining about the new-tab button (ala comment 77, 78, 79) with ui decisions on its new placement, otherwise, yeah this new feature (tab-bar was never customizable) is solely dependant on this bug.
So it definitely won't be included into 3.1?
Flags: blocking-firefox3.1- → blocking-firefox3.1?
lockalsash: If the bug has already been marked - for blocking, you can't just keep asking for it to block the same release without giving very good reason...you gave none at all.
Flags: blocking-firefox3.1? → blocking-firefox3.1-
Mconnor, any chance to get the attached patch reviewed in the near future?
Whiteboard: [has patch][needs review mconnor]
It's highly unlikely I'll have time for this in the next six weeks, between 3.1 and some other higher-priority projects on my plate.  It's certainly not happening before 3.2, so there's no major rush to put this in front of other things.
Duplicate of this bug: 472977
No longer blocks: 456984
Attachment #344927 - Flags: review?(mconnor) → review?(gavin.sharp)
Comment on attachment 344927 [details] [diff] [review]
patch v2.12

I'm definitely not going to have cycles for properly reviewing this in the next few months, punting to Gavin.
Whiteboard: [has patch][needs review mconnor] → [has patch][needs review gavin]
Comment on attachment 344927 [details] [diff] [review]
patch v2.12

90% of that patch doesn't apply anymore. I'll have to update it, although I don't think it will work, due to XBL changes (e.g. http://hg.mozilla.org/mozilla-central/rev/1963ead6e19b).
Attachment #344927 - Attachment is obsolete: true
Attachment #344927 - Flags: review?(gavin.sharp)
No longer depends on: 380960
Whiteboard: [has patch][needs review gavin]
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Does this bug have any priority anymore? I know a lot of people are focusing on performance rather than functionality at the moment. I'm curious if I should break the bug 457187 dependency and try and petition for that to be fixed separately?
I don't think we're going to be able to take this so late in the cycle.

I believe that we want it, but we need to fix this on trunk early to see what the impact is to add-ons, etc. Probably worth getting a discussion going in dev-apps-firefox to talk about what changes to the tabstrip need to be made in general, and figuring things out from there.
Flags: wanted-firefox3.6?
Flags: wanted-firefox3.6-
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Blocks: 544815
Depends on: 354048, 508499, 402147
Now that all the dependents have been fixed, what is left to do here?
Attached patch wip (obsolete) — Splinter Review
moveTabTo is broken, drag and drop is broken, tooltips are broken, Windows and Mac styling is likely broken.
Depends on: 551712
Attached patch wip 2 (obsolete) — Splinter Review
drag and drop and tooltips fixed
Attachment #431892 - Attachment is obsolete: true
Attached patch wip 3 (obsolete) — Splinter Review
Windows styling fixed
Attachment #432091 - Attachment is obsolete: true
Attached patch wip 4 (obsolete) — Splinter Review
I still don't know why moveTabTo misbehaves. Mossop, any ideas?
Attachment #432124 - Attachment is obsolete: true
Attachment #432164 - Flags: feedback?(dtownsend)
Attached patch wip 5 (obsolete) — Splinter Review
Got rid of the tabbrowser-strip binding. For some reason, this fixed moveTabTo.
Attachment #432164 - Attachment is obsolete: true
Attachment #432164 - Flags: feedback?(dtownsend)
Attached patch patch (obsolete) — Splinter Review
Tests passed on the tryserver. Ts and Twinopen are unchanged on Linux, Windows XP and Vista. The results on OS X 10.5 look unreasonable and are accompanied by a 100ms ts_shutdown increase, which I guess is bug 519893.
Attachment #432302 - Attachment is obsolete: true
Attachment #432425 - Flags: review?(vladimir)
Is there any try build that developers could use to see the impact on third-party add-ons?
Is it still this document valid?
https://developer.mozilla.org/User:Dao/Get_your_theme_ready_for_the_Tab_Toolbar
https://build.mozilla.org/tryserver-builds/dgottwald@mozilla.com-try-ee1dd3ed545c/

The document isn't valid anymore. I had to implement this from scratch. It's similar to the old patches but not the same.
I hope you will compose and post a similar document.
Why did you change a property name?  Is this necessary to fix this bug?
gBrowser.mContextTab
to
gBrowser.tabContainer._contextTab
Because all the context menu stuff moved to the tabContainer's binding.
mContextTab 
to
_contextTab

what does it mean?
I don't understand your question.
> I don't understand your question.
I'm not Alice, but methods starting with an underscore normally imply an internal implementation detail that extensions should not rely on since it may change. Previously it was "mContextTab" which, since it is a public semi-api, many extensions use or depend on.
Attached patch patch (obsolete) — Splinter Review
The m prefix has the same meaning as the underscore. That said, I can just implement the old property.
Attachment #432425 - Attachment is obsolete: true
Attachment #432521 - Flags: review?(vladimir)
Attachment #432425 - Flags: review?(vladimir)
Comment on attachment 432521 [details] [diff] [review]
patch

Looks fairly straightforward to me -- you may want to use Services.* in tabbrowser instead of the magic gPrefService, but that's no worse than today.

Grab a review from gavin on this when he gets back as well?  Can be done post-landing, though.
Attached patch patch (obsolete) — Splinter Review
Using Services.prefs instead of gPrefService
Attachment #432521 - Attachment is obsolete: true
Comment on attachment 432751 [details] [diff] [review]
patch

>+      <xul:menupopup anonid="tabContextMenu"
>+                     onpopupshowing="this.parentNode._updateContextMenu(this);"
>+                     onpopuphidden="this.parentNode._contextTab = null;">

I don't know how many extensions might add a submenu to the context menu but you probably only want to do this when the main menu is hidden. Something like:

onpopuphidden="if (event.target === this) this.parentNode._contextTab = null;">

[It may also make sense to do the same on popupshowing but the current code doesn't bother]
Blocks: 404770
This patch affects on accessible tree. I wonder what screen readers were used to test to ensure the patch doesn't have negative impact on AT users and why were accessibility mochitests changes landed without review from accessibility module peer?
I'd definitely like to see a try-server build and give this a checkup before this lands. It has all sorts of impact on a much-used feature for screen reader users exposure- and keyboard-nav-wise.
This has landed already:

http://hg.mozilla.org/mozilla-central/rev/8ca8630b0c88

I addressed comment 112 beforehand.

Let me know if this causes problems for a11y, preferably in a new bug.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a4
(In reply to comment #115)

> Let me know if this causes problems for a11y, preferably in a new bug.

If it regress a11y then I would like to see this patch backed out. If it doesn't regress a11y then any way accessibility code needs to fit new tabbrowser markup. Personally I would like to be notified when anything that affects on accessibility is landed because this is kind of respect to our work.
Additionally, these Mochitests mimic in part what a screen reader is doing, so in a way these Mochitests give us an indication that screen readers are expected to work with a certain set of conditions. Changing these tests kind of is expecting screen readers to still cope, but not being sure, without checking this manually as well.

I just built with this bug fix locally, and here are my observations:
1. When tabbing into the tabs toolbar from the Google Search field, NVDA now announces something like "toolbar, tabs, <page name> tab". Previously, it would indicate that focus just landed on a tab. Both the toolbar and tabs are unnamed. To the user experience, this is a REGRESSION.

2. Through this bug, I was made aware that the tabs should have sub menus attached to them. This fact is not communicated to screen readers like NVDA yet. An example where a tab has a sub menu attached, and which *is* communicated, is in Firebug, any of the tabs there have a sub menu, and this is communicated to screen readers. Otherwise, users will never know that they can press the APPLICATIONS key and get more options that way. This is a MISSING feature from this bug fix.

3. The buttons discussed such as "new tab" and "show all tabs" are not reachable via keyboard. There is, I believe, at least 1 bug on that, since these buttons were there before. And while it can be argued that this is a separate bug, while touching this code anyway this could have been rectified rather than having to go back to it later and fix that other bug post-mortem.

So from this landing, we have 1 regression and 1 missing a11y feature, along with a missed chance to take another bug as a ride-along.
Reopen bug. I'd suggest to back out the patch since it was landed without proper reviews and has regressions. At least in this case we don't need to rush in regressions fixes and making the changes in accessibility code to deal correctly with new tabbrowser hierarchy.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Some other issues: 
1. extension toolbars such as StumbleUpon and Facebook are now below the tabbar
2. toolbarbuttons from extensions have all disappeared, even from the Customize Toolbar palette.
This screws up the tab strip in some themes.
Also the addon FireGestures doesn't work.  I tracked it down using the hourlies and it seems to point to this bug.
Looks like it also doesn't get aligned with the sidebar being open, which sort of makes sense being a toolbar, but breaks the way it has always worked.
This break any extension dealing with tabs, tab mix plus, tab utilities and many more
WORST IDEA EVER!.
Unless you have something really useful to point out, please avoid fud commenting.

https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
(In reply to comment #117)
> I just built with this bug fix locally, and here are my observations:
> 1. When tabbing into the tabs toolbar from the Google Search field, NVDA now
> announces something like "toolbar, tabs, <page name> tab". Previously, it would
> indicate that focus just landed on a tab. Both the toolbar and tabs are
> unnamed. To the user experience, this is a REGRESSION.

Should we hide the toolbar and the tabs elements to screen readers? (Via role="nothing"?) Or should we actually label them? Should file a new bug about this, as it doesn't seem the patch should be backed out because if this.

> 2. Through this bug, I was made aware that the tabs should have sub menus
> attached to them. This fact is not communicated to screen readers like NVDA
> yet. An example where a tab has a sub menu attached, and which *is*
> communicated, is in Firebug, any of the tabs there have a sub menu, and this is
> communicated to screen readers. Otherwise, users will never know that they can
> press the APPLICATIONS key and get more options that way. This is a MISSING
> feature from this bug fix.

You're saying that this bug didn't change this, right?

> 3. The buttons discussed such as "new tab" and "show all tabs" are not
> reachable via keyboard. There is, I believe, at least 1 bug on that, since
> these buttons were there before. And while it can be argued that this is a
> separate bug, while touching this code anyway this could have been rectified
> rather than having to go back to it later and fix that other bug post-mortem.

This doesn't belong in this bug at all.

(In reply to comment #122)
> This break any extension dealing with tabs, tab mix plus, tab utilities and
> many more

No, it didn't break any extension dealing with tabs. Of course, it was expected that tab mix plus wouldn't continue to work right away. It just breaks very often when we touch tabbrowser.xml.
(In reply to comment #124)
> Unless you have something really useful to point out, please avoid fud
> commenting.
> 
> https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Unless you have something useful to add to the browser, avoid fud patches.

There is practically no point to this from a users point of view, other then to cause them strife from suddenly invalidated extensions for no observable purpose.

Benefit to User: NONE.
If you would like to appeal against this bug I would suggest you post in one of the Mozilla newsgroups or forums. 

Such comments don't really belong in the actual bug and you won't make a long of headway insulting people's hard work to make Firefox more customisable for the end user.
(In reply to comment #119)
> 2. toolbarbuttons from extensions have all disappeared, even from the Customize
> Toolbar palette.

Works for me. There's probably some extension interfering with this in your case.
(In reply to comment #126)
> There is practically no point to this from a users point of view, other then to
> cause them strife from suddenly invalidated extensions for no observable
> purpose.

This patch changes our platform so that the tabstrip can be rearranged along with the other toolbars such as the menu bar, the main toolbar, the bookmark toolbar, etc. It provides a bunch of extra flexibility, actually.

Extensions will break on in-development branches. That's why they're called in-development branches.

> Benefit to User: NONE.

Quite some, actually. If you've nothing more to add to this discussion, I strongly suggest you leave. Thanks for your feedback, and our apologies for your broken extensions. They'll work on Firefox 3.6, still, and we'll make sure they work on this version of Firefox when it's released.
(In reply to comment #125)
> Should file a new bug about
> this, as it doesn't seem the patch should be backed out because if this.

Marco filed couple bugs already which were caused by this patch. It sounds they aren't critical, i.e. AT users can still use firefox but it affect on their user experience negatively. I wouldn't ask to back out the patch if we can make sure all regressions can be fixed quickly.

Another point is regression bugs were found by manual testing and I worry we can still have couple undiscovered issues. Ideally somebody needs to analyze the changes to realize what else it can affect on.

I'd suggest to open meta bug to move accessibility related discussions there to avoid spamming everyone and leave this bug open until we come to consensus.
Attached patch as landedSplinter Review
Looking for gavin's input as suggested by vlad. I'll file new bugs for any identified issues.
Attachment #432751 - Attachment is obsolete: true
Attachment #433267 - Flags: feedback?(gavin.sharp)
No longer blocks: 553157
Depends on: 553157
Closing again, this is still fixed on trunk, leaving it in another state is only going to confuse people.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Depends on: 553175
Blocks: 443826
The missing toolbarbuttons reported in comment #119 where caused by the 'PDF Download' extension, version 3.0.0.1. Not sure (yet) if this is related to this tabbar change itself.
The StumbleUpon toolbar usually resides, in it's default position, below the Personal Toolbar and above the tabs. Now it is below the tabs and cannot be moved back above the tabs. I alerted the developers of the StumbleUpon toolbar, but I'm not sure where the problem is, with their toolbar, or with this patch.
(In reply to comment #134)
Bug 553175
Depends on: 553389
Depends on: 553390
Depends on: 553393
Depends on: 553396
Depends on: 553399
No longer depends on: 553396
despite this breaking 2 extensions i work on, i'm glad to see it moving
forward.  i'll document an issue, however.  here for lack of a better place.

i overlay the DOM with a tree box below menubar and above the browser, and it's quite important that tabs belong with the content window, directly above it.  this change places them above the tree box and the sidebar.  it's quite easily fixed by simply doing an insertBefore of the TabsToolbar as firstChild of #appcontent, on init.  in the old days this kind of move would have lost
listeners etc etc, but everything works fine so far.

even though the TabsToolbar hasn't (yet) been hooked up to toolbar
customization, the clear benefit here is that it will be.  this will prevent
the above fix by exposing a flaw in toolbar customization in that they all
currently must be children of #navigator-toolbox.

TotalToolbar, which i also own, allows the DOM to be littered with toolbars
everywhere (even the tab bar and statusbar) and also plug into customization. 
i hope we can redo customization to handle an array of toolboxes, not just
#navigator-toolbox.
Depends on: 553404
Blocks: 424520
Depends on: 553797
Blocks: 359804
Depends on: 554279
No longer blocks: 130061
Depends on: 558064
Attachment #433267 - Flags: feedback?(gavin.sharp)
Depends on: 563588
this bug break Bug 343628

in tabbrowser-close-tab-button binding _blockDblClick is set for the button instead of tabContainer.

        tabContainer.tabbrowser.removeTab(bindingParent);
-       this._blockDblClick = true;
+       tabContainer._blockDblClick = true;


-       var self = this;
        var clickedOnce = false;
        function enableDblClick(event) {
          if (event.detail == 1 && !clickedOnce) {
            clickedOnce = true;
            return;
          }
          setTimeout(function() {
-           self._blockDblClick = false;
-           tabContainer._blockDblClick = false;
          }, 0);
          tabContainer.removeEventListener("click", enableDblClick, false);
(In reply to comment #138)
> this bug break Bug 343628

Confirming bug 343628 regressed.  double click closes tab and opens a new one on latest trunk.
Depends on: 570918
(In reply to comment #138)
> this bug break Bug 343628
> 
> in tabbrowser-close-tab-button binding _blockDblClick is set for the button
> instead of tabContainer.

Thanks. Filed bug 570918 on this.

If you attach your pseudo patch as a real patch there, I'll review it.
(In reply to comment #138)
> this bug break Bug 343628

In the future, please file these kind of issues as a new bug directly, and CC the relevant people, rather than just commenting in a resolved bug!
Blocks: 577762
Depends on: 586234
Blocks: 576345
With all the docs having been done for various tab related changes, I think this is covered.
Depends on: 642355
Depends on: 561482
Flags: in-testsuite?
See Also: → 130061
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.