279308 - [FIX]Folders don't draw correctly on bookmarks toolbar when opened for the first time

Status: RESOLVED FIXED

(Core :: XUL, defect, P1)

Description

[TimoT]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050121 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b) Gecko/20050121 Firefox/1.0+

After opening the browser folders on the bookmarks toolbar fail to open
correctly. After opening a folder once, the folder starts to work as expected
until closing the browser.

This regressed between 20050119 and 20050120.

Reproducible: Always

Steps to Reproduce:
1. Create two folders with bookmarks in them on the bookmarks toolbar
2. Restart browser
3. Click folder A so that the menu opens, while the menu is open move mouse on
folder B.

Actual Results:  
The browser doesn't draw folder B's menu. If you repeat the steps without
restarting the browser, the bookmarks toolbar folders work as expected.

Expected Results:  
The folder is opened and its contents are drawn

Comment 1

[Chris Blore]

WFM under Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b)
Gecko/20050121 Firefox/1.0+

Comment 2

[Steve England [:stevee]]

Confirming
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050121 Firefox/1.0+
This also happens for live bookmarks, so you could add another step
4. Move mouse over live bookmark in the bookmarks toolbar. Expect to see list of
links, instead none is shown.

Comment 3

[Martijn Wargers (dead)]

I think this is a regression from the fix of bug 244366. I saw this already when
I tried the patch from that bug (but then I thought it was something weird in my
build).
I can also confirm all the observations in comment 0.

Comment 4

[TimoT]

I backed out the patch from bug 244366 and it did indeed fix this bug.

Backing it out also fixed another little quirk I was having with mostly crystal
theme. Everytime I would go fullscreen (f11) and back the urlbar icon would
appear in two spots, in the url bar and about an inch below it until scrolling
the page or doing something similar.

http://www.tom-cat.com/mozilla/index.html

Comment 5

[Boris Zbarsky [:bzbarsky]]

Timo, since I can't reproduce this bug (on Linux hovering a folder while another
one is open does nothing, as it's supposed to), would you mind doing some
testing for me?

In particular, could you see whether the patch in bug 279205 helps?

If it does not, could you try changing the condition at
http://lxr.mozilla.org/seamonkey/source/view/src/nsViewManager.cpp#4202 from 

  4202   if (mHasPendingUpdates) {

To

  4202   if (1 || mHasPendingUpdates) {

and seeing whether that helps here?

Comment 6

[TimoT]

Just finished two quick test builds with the suggested changes, sorry to say but
neither of them had any effect on this bug.

However, the patch from bug 279205 seemed to fix the little quirk with the page
icon I mentioned.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I assume that you did a test build that had both changes in it, right?

Comment 8

[TimoT]

yeah, the first build I made just had the patch from bug 279205. The second
build had the patch and the change you suggested. I didn't make one with only
the 'if (1 || mHasPendingUpdates) {' change though.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, testing both changes together is what I wanted...

I'll try to figure out what could be causing the problem.  Might be fun without
a way to debug this... :(

Comment 10

[Martijn Wargers (dead)]

The "folders in bookmarks toolbar should behave like menus" behavior was
introduced with the fix for bug 203556.

When I changed this line at:
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarksMenu.js#922
target.open = true;
into this:
target.getElementsByTagName('menupopup')[0].showPopup(target);
then this bug doesn't occur anymore.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Martijn, is the open prop in question the one implemented at
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/menulist.xml#102 ?

Comment 12

[Martijn Wargers (dead)]

No, putting an alert there doesn't do anything.
This part is called:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/button.xml#33
I have no idea where it goes from there.

Comment 13

[Logan Ingalls]

*** Bug 279443 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Possible patch

Could someone test this?  I'm still working on a debug Firefox build that
actually runs...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

So what's going on here is that XUL menus rely on sync frame construction when
they make an attr change.  I just filed bug 279703 on that.

The Firefox bookmark code triggers this by setting the "open" attribute on the
menus directly.  So we get into nsMenuFrame::AttributeChanged, which means
we're inside the presshell's AttributeChanged function and so mChangeNestCount
is nonzero (with the patch in bug 244366).  Now the menu sets the attribute and
tries to flush out style reresolves, but the presshell ignores the flush, since
it feels (rightly!) that it's possibly in the middle of frame construction.

This patch makes the following changes:

1)  Remove the flushes I added for bug 230170 to menus
2)  Directly hack the nsCSSFrameConstructor::AttributeChanged code to process
    the "menugenerated" attribute on the affected nodes synchronously.
3)  Factor out the code to process style updates to deal with this, and clean
it
    up a tad (it looks like I had some carryovers from when it used static
    methods in an initial version of the patch in bug 230170).
4)  Make the change to nsPresShell::FlushPendingNotifications that I initially
    tried for this bug -- I still feel it's the right thing to do.

Comment 16

[Martijn Wargers (dead)]

I think you've already checked it yourself, but anyway, that last patch of yours
fixes this bug indeed.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 172326 [details] [diff] [review]
Patch

This special-casing is a bit gross, but I don't have any better ideas.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

This caused bug 287616