Closed Bug 331297 Opened 18 years ago Closed 17 years ago

Bookmarks toolbar overflow popup opens large, then resizes

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME

People

(Reporter: philor, Unassigned)

References

Details

Appears to be Mac-only, when you click the chevron to open the popup menu for overflow on the Bookmarks Toolbar, it opens too big (three or four times bigger than it needs, by eye), then fairly quickly but not quickly enough to not see, resizes to fit its contents.
This happens in 1.5.0.x, too; I'm pretty sure I've seen a bug on it at one point (though I can't find it now).
The previous impl may have hacked around a core problem, then: with a -disable-places build, I *sometimes* see it the first time I open the popup after changing the size of its contents, though it's too quick to be bothersome, but not after that first opening. With places, it's every single time, and much slower.
Assignee: nobody → annie.sullivan
Priority: -- → P3
Target Milestone: --- → Firefox 2 alpha2
Unless this is a different bug with remarkably similar symptoms, it's actually showing a list of all items in the toolbar, briefly, before correcting. Hence the big menu. It's been reported as bug 308030. 

For that bug, ignore the Description (which seems to not be related to the Summary) but instead read bug 308030 comment 5.

*** This bug has been marked as a duplicate of 308030 ***
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → DUPLICATE
Um, no. That's an utterly worthless bug, since it got morphed from the CSS for the chevron to something I've never seen, trunk or branch, Mac, Linux, or Windows, with the popup rather than the chevron. Your bug, which shouldn't have been duped to that in either the original or the morphed state, is probably the same thing I'm talking about, but still the only potentially useful part of that bug is the blocker: if for some reason the Places implementation of toolbar overflow is toggling the menugenerated attribute every time, and the non-Places implementation didn't (because as you said in your original bug, pre-Places it happened once, and now it happens every single time), then we need to try to not do it (all the more because of bz's gloomy crashing-talk).
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
*** Bug 324938 has been marked as a duplicate of this bug. ***
I see... you're right, sorry. When my bug was first duped to that one, I noticed that the description didn't seem to match the summary, but just assumed the guy who duped it knew what he was doing. Looking at the activity history for bug 308030 now, I see he's made a really bad assumption. Since this one is already reopened, I've duped mine to this, and I'll let Boris know.
Status: REOPENED → NEW
OS: MacOS X → All
Hardware: Macintosh → All
Notes that I added to the other bug, which may be useful:

The regression occurred between the 20040809-09 and 20040810-15
Firefox trunk builds. It was also present in SeaMonkey by 20040811, but I can't
pin down an earlier date as there were no Mac builds at that time.

It didn't appear in the 0.9 or 1.0.x branches at all. So the first appearance
in an official release would be 1.5.

As mentioned before, it pre-dates Places, and used to only appear the first time you clicked on the chevron. But since Places was enabled, it now appears every time.

Possibly more fallout from the fix for bug 230170.

Boris commented that it's possibly dependent on bug 279703. I'll set it accordingly.
Depends on: 279703
Note that some actually usable steps to reproduce (starting from a clean Firefox install with a brand-new profile) would be really helpful here.  And in all bugs in general.
Sorry, I do know better.

STR:
1. Buy a Mac
2. In a new profile in a build with Places enabled, right-click bookmarks toolbar, select New Folder...
3. Copy New Folder, then paste enough to create overflow.
4. Click chevron to open overflow popup menu. A blank popup opens, smaller than what would contain all bookmarks toolbar items, but larger (in both dimensions) than what's required for the one or two items in it, which then resizes to fit the contents, in a fraction of a second but plenty long enough to be visually distracting.
5. Close popup, then click chevron again and again, seeing the same thing happen each time.

Expected: at least the pre-Places behavior, opening large once then opening the correct size until the window is resized enough to affect the overflow, if not the Windows/Linux behavior of not opening too large and resizing.

Actual: a very obvious flash of white every time the overflow popup opens.
And while I'm willing to be believe Adam saw *something* on Linux last September that made him morph that other bug, I'm sitting in front of hours-old trunk builds on Windows, Mac, and Linux, and seeing this only on Mac, so I'd say not All/All unless someone has different and workable STR for non-Mac platforms.
OS: All → MacOS X
Hardware: All → Macintosh
I can reproduce this easily in Windows XP (as well as Mac OS X 10.4.5), using the latest trunk nightly (20060329), and a fresh profile.
1. Start with a clean profile
2. Navigate to a number of different websites, dragging the icon of each new one from the location bar to the bookmarks toolbar, until the toolbar overflows and the chevron appears.
3. Click on the chevron. Note the vague outline of a much larger menu before the correct, smaller menu appears.

The problem in XP is that this happens MUCH faster than it does on my Mac. So I usually don't see the bogus menu appear in full, but usually see a black line representing its bottom edge, and/or the sides. Maybe that's why you haven't noticed it. Occasionally it's been slow enough for me to glance the menu briefly before it changes, and note some of the bogus menu contents.

Keep hitting the chevron quickly and hopefully you'll glance it. It's easier to see if you have a white page background where it will appear, and I do find it's easier to detect if I hit it a number of times in a row. But maybe it's just that your PC is much faster than mine (Athlon 2400+), so the bogus menu is vanishing too quickly to detect.

One other thing...
> A blank popup opens, smaller than what would contain all bookmarks
> toolbar items, but larger (in both dimensions)

This bogus menu *definitely* contains the contents of the bookmarks toolbar, including site icons. Again, maybe your machines are too fast for you to detect it, so you're only seeing the white flash of a menu. On both Mac and PC I've been able to register items listed there that are from my bookmarks toolbar. It's not always easy to detect, as sometimes the bogus menu vanishes faster than other times.

It looks something like the browser generates a list of all bookmarks toolbar items, and then pops up the menu before it gets around to paring it down to only the overflow items. 
Well, at least while I'm being wrong, I'm more than strident enough about it. You're right on both counts, size and platform.

STR to see that the size is proportional to the total number of bookmarks:
1. Add four bookmarks to the toolbar.
2. Resize the window so three show, and one is in the overflow.
3. Get a setup that will let you see the flash, and open the popup, it should be fairly small even in the larger size.
4. Maximize the window, and add more bookmarks until you have overflow again.
5. Open the popup, the flash will be larger.

But, here's my STR on Windows, with a not-that-hot P4 2.4GHz and what it claims oddly is 448MB of RAM:

1. Open a dozen tabs each in three windows, putting in plenty of Flash among them.
2. Use Task Manager to set the priority for firefox.exe to low.
3. Start building Firefox.
4. For bonus fun, start another build, building debug.
5. At this point, with the CPU pegged and memory paging like mad, trigger the bookmarks toolbar overflow popup, watching closely to get a feel for where it will be: the dropshadows on the Windows theme make the outer edge the easiest part to spot. For me, in that state, the flash is barely visible, although it's sometimes as much as five seconds between clicking and the popup opening.

That's compared to my spanking-new dual-core Intel iMac with a GB of RAM, with nothing else running, where it's obvious and distracting, which is why I think that although it would be nice to fix the core widget issue, or to fix the Mac widget issue that makes it so much worse, at the very least we should try not to make Places make it so much worse than it was pre-Places, by not triggering it every time instead of just the first time, and by trying to be quicker (since I know we're doing a Mac-only walk up the DOM to see if we're in the bookmarks menu, and we might be doing more Mac-only stuff I don't know about).
OS: MacOS X → All
Hardware: Macintosh → All
Can someone tell me where this code lives?  I can think of a reason this would happen, but I'd have to mess with the code to test...
Hmm...  I don't see code at either place that obviously fills in that menupopup...
(In reply to comment #15)
> Hmm...  I don't see code at either place that obviously fills in that
> menupopup...

The "place" attribute controls the source of data.
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/toolbar.xml#228
Setting it calls _rebuild(), which adds the items to the menu.
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/toolbar.xml#104

I hadn't found http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/menu.xml#9 , which is what I suspect you are looking for.
When the popup first comes up it just contains all the bookmarks.  None of them are hidden.  The hiding is done via the popup showing callback (<method name="chevronPopupShowing">), but that runs after the popup is, well, shown.  That code should just be running earlier.
Flags: blocking-firefox2?
Flags: blocking-firefox2?
*** Bug 337831 has been marked as a duplicate of this bug. ***
Depends on: 337868
Flags: blocking1.9a2?
OK, as far as I can tell, this is how things work:
When the browser first opens, all the menus on the bookmarks toolbar (including the chevron one) are empty.
When a menu is popped, the |popupshowing| event is called in menu.xml
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/menu.xml#736
This calls onPopupShowing() which calls both _rebuild() and  popupShowingCallback():
_rebuild() adds the elements to the menu the first time the menu is opened. Latter times, it removes all entries from the menu and rebuilds it. So every time a menu is popped on the toolbar, the menu is rebuilt from scratch.
 
popupShowingCallback() is what calls chevronPopupShowing() in toolbar.xml
http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/toolbar.xml#275
As Boris said earlier, that's what hides the items in the chevron/overflow menu that shouldn't be there.

The problem seems to be in _rebuild(). It's not called *at all* until the menu is already being opened. So as it goes, it adds all elements to the menu and they appear immediately, as the menu is already open. At least that's how it seems to me. So it's too late to call chevronPopupShowing() after _rebuild().

Calling chevronPopupShowing() before _rebuild() is of course, useless because the first time the menu is opened, there's nothing in it yet to hide. Calling it on subsequent times is also useless, as _rebuild() undoes all the good work as it strips the menu and rebuilds it.

I tried forcing the menu to collapse during _rebuild(), but it didn't seem to work. It explicitly states that the container must be open during the rebuild, for whatever arcane reasons, and I guess this applies to the collapsed state as well. Setting it to hidden seemed to have the same effect.

I'm not sure how well these suggestions would work in practice, but all I can think of is:
(a) add a method to build the menus when the toolbar first loads (BEFORE any toolbar menu is opened), and hide the unwanted items at that point. Don't call _rebuild() when a menu is opened. I'm not sure if there's any reason for the toolbar menus to be rebuilt every time they're opened. Seems otherwise wasteful.
OR:
(b) Do away with chevronPopupShowing(). Instead, in _rebuild(), check if it's the chevron/overflow menu that is being opened. If so, then as each element is being added, check if it has a counterpart visible on the toolbar and set it to hidden if that's the case (much as chevronPopupShowing() does currently). I guess it wouldn't be too hard, but would add a lot of extra checks.

Maybe I'm not understanding how it works, but the above are the only ways out I can see. I had actually expected for the menu to be opened *after* the |popupshowing| event, which I thought would prevent this bug happening. But as far as I can tell, that doesn't seem to be the case here.
Blocks: 343582
Could someone in the know please offer some advice on the following?

I just noticed that by the time the popupshowing event is first triggered, the menugenerated attribute is already set to true.

So if I do: this.setAttribute("menugenerated", false); at the top of onPopupShowing, and then set it back to true at the bottom (i.e. after the menu has been rebuilt), then this bug goes away.

That makes me wonder whether menugenerated is meant be false at the time that the popupshowing event is triggered. I thought that popupshowing was supposed to be triggered just *before* the menu was opened, but menugenerated was supposed to be set to true as it *is* opened. Can anyone confirm that this is supposed to be the case?

Thanks!
menugenerated has to be set to true before the event gets fired, otherwise there isn't a popup frame to do anything with. However, the menugenerated attribute is used only to generate the popup frame, not make it visible. The popup is not made visible until after the event.

See http://wiki.mozilla.org/XUL:Popups for details.
I've been seeing similar behaviour recently with some other popups whose menuitems are shown or hidden from a popupshowing event. A wild guess is that it could be that the reflows caused by the change of hidden attribute are now being posted and don't take effect until a visible amount of time later.
Thanks for that! It's making more sense now.

If we can't get in a real fix before Firefox 2, could we maybe cheat by setting menugenerated to false in onPopupShowing (in menu.xml) and then setting it true again once the menu is generated? This might only need to be done if(this.popupShowingCallback)... at least that's the only thing I know of that dynamically changes a popup menu. I tested that scenario out before and it seemed to work fine. Not a proper solution, I know, but at least the bug will be hidden for Fx 2.
This bug is in places, not Firefox 2.

Also, I don't think Annie is working on this, so somebody else should feel free to take it.
Assignee: annie.sullivan → nobody
(In reply to comment #24)
> This bug is in places, not Firefox 2.

In the Moz 1.8 branch, it only occurs the first time the popup is opened, or if the browser window is resized, or new bookmarks added to the toolbar, so as to make it rebuild.

So it will be in Firefox 2. Places just made it worse on the trunk.
Yes, you're right, it happens without places as well. Changing the component...
Component: Places → Toolbars
QA Contact: places → toolbars
Version: Trunk → 2.0 Branch
Erm, if you don't like Places, where I filed it as a Mac Places bug because those two things (Places not caching popup contents, and Mac walking up the tree in every menu to see where it is) make it slow enough to be distractingly obvious, unlike non-Mac and/or pre-Places where it's essentially invisible, and you want to completely morph it into Wayne's bug 324938, then you probably should have moved it to where that correctly was in Widgets:Menus, not to someplace completely worthless and certain to be ignored like Firefox:Toolbars.

Oh, wait, you didn't cc yourself, and you don't watch this component, so that move did wfy, didn't it?
Priority: P3 → --
Target Milestone: Firefox 2 alpha2 → ---
Version: 2.0 Branch → Trunk
Flags: blocking1.9a2?
In current Places builds, I no longer see the comment 0 behavior I reported.
Status: NEW → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.