Closed Bug 279308 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XUL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta1

People

(Reporter: ttolonen, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 1 obsolete file)

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
WFM under Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b)
Gecko/20050121 Firefox/1.0+
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.
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.
Blocks: 244366
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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?
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.
I assume that you did a test build that had both changes in it, right?
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.
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... :(
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.
Martijn, is the open prop in question the one implemented at
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/menulist.xml#102 ?
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.
*** Bug 279443 has been marked as a duplicate of this bug. ***
Attached patch Possible patch (obsolete) — Splinter Review
Could someone test this?  I'm still working on a debug Firefox build that
actually runs...
Assignee: firefox → nobody
Component: Menus → XP Toolkit/Widgets: Menus
OS: Windows XP → All
Product: Firefox → Core
QA Contact: bugzilla
Hardware: PC → All
Attached patch PatchSplinter Review
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.
Assignee: nobody → bzbarsky
Attachment #172292 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #172326 - Flags: superreview?(roc)
Attachment #172326 - Flags: review?(roc)
Priority: -- → P1
Summary: Folders don't draw correctly on bookmarks toolbar when opened for the first time → [FIX]Folders don't draw correctly on bookmarks toolbar when opened for the first time
Target Milestone: --- → mozilla1.8beta
I think you've already checked it yourself, but anyway, that last patch of yours
fixes this bug indeed.
Comment on attachment 172326 [details] [diff] [review]
Patch

This special-casing is a bit gross, but I don't have any better ideas.
Attachment #172326 - Flags: superreview?(roc)
Attachment #172326 - Flags: superreview+
Attachment #172326 - Flags: review?(roc)
Attachment #172326 - Flags: review+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: 230170
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.