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.
If you put a bookmarks folder on the end, the bookmarks folder disappears, but the linked icons stay. In a sense a temporary fix.
Created 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 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 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.
(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.
Created attachment 604621 [details] [diff] [review] 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.
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.
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 :(
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.
(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 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;
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.
(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.
Created attachment 605173 [details] [diff] [review] 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 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.
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.
And it also need when execute "Move To Group" tab context menu.
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.