603817 - Bookmarks toolbar items disappear if placed on tab bar with many tabs open

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[ratman]

This has been an ongoing issue with beta builds but I had not come up with steps to reproduce until today.  Currently using Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre.

1) Move the bookmarks toolbar items from the bookmarks toolbar to the tab bar, to the right end.
2) Open a bunch of tabs - on a new profile, this works by opening 11 tabs.
3) At the far right of the tab bar, the "show more bookmarks" button appears.  Click it.

Expected behavior:
4) All of the bookmarks toolbar items appear in the show more bookmarks menu.

Actual behavior:
4) Not all of the bookmarks toolbar items appear, only the ones at the right end.

An additional piece:
5) Close some of the tabs you just opened.

Expected behavior:
6) All of the bookmarks toolbar items reappear on the tab tab.

Actual behavior:
6) Only the items on the left end of the bookmark toolbar items reappear, the others have disappeared.

Work-around:
4a) or 6a) Resize the browser and/or close the browser and reopen (assuming tabs are reopened on the new session).  Missing bookmarks reappear.

Comment 1

[David Hopwood]

If you put a bookmarks folder on the end, the bookmarks folder disappears, but the linked icons stay. In a sense a temporary fix.

Comment 2

[Owen Jones]

Attachment: If personal-bookmarks are moved tab toolbar then attach event listeners for TabOpen and TabClose so it can be resize properly

One problem with this patch is that it doesn't quite do the right thing for TabClose. It tries to update the chevron before the tab has finished closing. This means that there is sometimes space for one more bookmark that hasn't been filled. Ideally _unlockTabSizing would fire an event we could listen for, but I have no idea whether this would be a good idea.

Comment 3

[Owen Jones]

Hi, this is my first patch. I'm not amazingly familiar with javascript or the firefox codebase so I may have made some glaring errors. I've been playing around with fixing this bug that's been annoying me as a way to familiarise myself with programming for Firefox. As I see it, this bug is caused by updateChevron() not being called often enough. I believe a lot of the code to deal with updateChevron() was done in Bug 382466. It dealt with all the cases that updateChevron() would need to be called if the personal-bookmarks were in their default position, the PersonalToolbar (window resizing, adding or removing an item from the personal-bookmarks). But when the personal-bookmarks are moved to the TabsToolbar there is another thing which needs to call updateChevron() : opening or closing a tab (actually we only care when personal-bookmarks is already in overflow). My patch adds event listeners for TabOpen and TabClose when personal-bookmarks finds itself in the TabsToolbar (and removes them as necessary) and adds handlers to call updateChevron() when these events occur. I've tested it on Ubuntu (32 bit) and it provides a much better experience for me.

As I mentioned above, this doesn't completely fix the problem of tabs closing because the TabClose event fires just before the tab is closed. updateChevron() sets a timer for 100ms but this isn't long enough. Even using a longer timer wouldn't fix all cases because when you close tabs by clicking on the cross in the tab bar the space taken up by the tabs doesn't shrink immediately. Ideally the function which eventually makes it shrink, _unlockTabSizing, could fire some kind of "the tabs are shrinking now" event which we could listen for instead of TabClose.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 603464 [details] [diff] [review]
If personal-bookmarks are moved tab toolbar then attach event listeners for TabOpen and TabClose so it can be resize properly

Review of attachment 603464 [details] [diff] [review]:
-----------------------------------------------------------------

Hi, thank you for your contribution and the time you spent on it. I hope to see even more contribution from you in future!

Your proposed change sounds acceptable, though not generic enough.  As you well know this element can be moved anywhere and may even exist in nonbrowser windows, so just handling tabs is not sufficient.

First of all, your change to "underflow" event is very welcome, we are doing things in the wrong order and updateChevron bails out too early! good catch.
Regarding TabOpen and TabClose, I think we should rather handle DOMNodeInserted and DOMNodeRemoved on this._viewElt.parentNode.parentNode (this._viewElt.parentNode is "personal-bookmarks", so its parent is the toolbar where it exists), it will basically do the same, but for any kind of addition/removal.

Comment 5

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [:mak] from comment #4)
> DOMNodeInserted and DOMNodeRemoved on this._viewElt.parentNode.parentNode

Those are mutation events, so you don't want to use them.

Comment 6

[Owen Jones]

Attachment: Second version of patch using DOM mutation events

Here is a second version of the patch, making the changes suggested by Marco. After seeing Dao's comment I read https://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/2f42f1d75bb906fb?pli=1 which says that DOM mutation events are not recommended because they slow Firefox down (in their absence Firefox makes certain optimisations). I leave it up to people who know more about it to decide which patch to use, but I would really like it if this issue could be resolved. My vote is for my original patch, because I would hate to slow down everyone's experience of Firefox for something which won't affect the vast majority of firefox users.

Comment 7

[Marco Bonardo [:mak]]

For some reason I read that as being an issue only for DOMAttrModified, but it's not the case, sorry.
The problem is that just listening to Tab changes is not really a solution to the problem, is just a solution to a single case of the problem, then we would need special cases for the add-ons bar and so on.  I'm not sure there's a decent way to detect size changes without mutation events.
One feasible thing would be to use those mutation listeners only if the bookmarks are not in the default position, would still be a perf hit for a limited number of users though.

Comment 8

[Marco Bonardo [:mak]]

So dao, would you be fine if we listen to TabOpen, TabClose? I really don't see a possible solution apart handling each toolbar case :(

Comment 9

[Dão Gottwald [:dao]]

We can use TabOpen/TabClose now and file a bug on using an efficient replacement for DOM mutation events when it's available.

However, note that elements can also resize without nodes being added or removed.

Comment 10

[Marco Bonardo [:mak]]

(In reply to Dão Gottwald [:dao] from comment #9)
> We can use TabOpen/TabClose now and file a bug on using an efficient
> replacement for DOM mutation events when it's available.
> 
> However, note that elements can also resize without nodes being added or
> removed.

true, but that's a minor shift that may be acceptable (half of a bookmark may be hidden or there may be some empty space).  Right now once enter an overflow state and a tab is opened or closed without causing an overflow change bookmarks are just not reappearing at all.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 603464 [details] [diff] [review]
If personal-bookmarks are moved tab toolbar then attach event listeners for TabOpen and TabClose so it can be resize properly

Review of attachment 603464 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, let's keep this version as a workaround for now then.

::: browser/components/places/content/browserPlacesViews.js
@@ +908,5 @@
>    this._addEventListeners(window, ["resize", "unload"], false);
>  
> +  // If the personal-bookmarks have been dragged to the TabsToolbar then
> +  // we need to detect when a tab is opened or closed because it may
> +  // affect the size of the personal-bookmarks

let's change this to:
// If personal-bookmarks has been dragged to the tabs toolbar,
// we have to track addition and removals of tabs, to properly
// recalculate the available space for bookmarks.
// TODO (bug XXYYZZ): Use a performant mutation listener when available.

File a bug to investigate that, and replace XXYYZZ with the bug number please.

@@ +1109,2 @@
>          this.updateChevron();
>          break;

merge these 2 as
case "TabOpen":
case "TabClose":
  this.updateChevron();
  break;

Comment 12

[Owen Jones]

I have filed bug 734730 as a follow up.

A few questions:

(i) When merging case "TabOpen" and case "TabClose" should I also merge case "resize", which has the same code?

(ii) We still aren't dealing with the TabClose event quite right. It normally ends up leaving out a bookmark that it could have fitted in. One problem is that TabClose is fired before the tab is closed, so even with a 100ms timer we call updateChevron() before the tab has disappeared from the tabs toolbar. This could be solved by having a bigger timer, though I'm not sure how to do that elegantly. A bigger problem is that if you close tabs with your mouse (and in some other circumstances) then we don't shrink the space the tabs are taking up until _unlockTabSizing() is called (this is in browser/base/content/tabbrowser.xml). I think (but I haven't tried this out) that we could deal with this nicely by making _unlockTabSizing fire an event which we could listen for instead of TabClose. However, I'm not sure what the overhead of firing an event is. Is it worth me trying to do this, or shall we just go with the 90% solution we have already?

(iii) I think bugs 686993 and 668909 are duplicates of this bug. I think the original bug in bug 595080 has been fixed and then André changed it to be a duplicate of this bug (because his bug 598579 which was marked as a duplicate of 595080 really was a duplicate of this bug). Bug 675021 is this bug plus wanting to be able to specify how much space in the tabs toolbar should be given to tabs and how much to personal-bookmarks. Bug 643274 covers a lot of the same ground. When this bug is closed we should update them appropriately.

Comment 13

[Marco Bonardo [:mak]]

(In reply to Owen Jones from comment #12)
> I have filed bug 734730 as a follow up.
> 
> A few questions:
> 
> (i) When merging case "TabOpen" and case "TabClose" should I also merge case
> "resize", which has the same code?

it has an additional comment, so likely not.

> (ii) We still aren't dealing with the TabClose event quite right...

I think all of this is something to deal with later, not worth complicating this simple patch for edge-cases.

> (iii) ... When this bug is closed we should update
> them appropriately.

Sure, feel free to suggest changes in those bugs once this is done and we can update them.

Comment 14

[Owen Jones]

Attachment: Final patch

I've tested this on an up-to-date copy of moz-central. It seems to fix the bug and I didn't see any problems.

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 605173 [details] [diff] [review]
Final patch

Review of attachment 605173 [details] [diff] [review]:
-----------------------------------------------------------------

thank you, will try to push shortly to make the merge.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d2909f0e40

Comment 17

[Alice0775 White]

How about TabPinned and TabUnpinned event?
in this case, tab strip and boolmarks items width will be changed.
So, I think it needs to call this.updateChevron() respectively.

Comment 18

[Alice0775 White]

And it also need when execute "Move To Group" tab context menu.

Comment 19

[Marco Bonardo [:mak]]

Those are less used events and likely being used on a few tabs, let's see how things are going with the most common  events handled first. Filing a follow-up for further cases is fine, though I'm not sure we want to add lot of complication for exotic setups.

Comment 20

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e8d2909f0e40