Clicking on submenu item in bookmarks toolbar causes menu to go away

RESOLVED FIXED

Status

()

P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: vlad, Assigned: jaas)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

15.49 KB, patch
Details | Diff | Splinter Review
On your bookmarks toolbar, if you have a folder, say called "Foo".. and inside that folder you have another folder called "Bar", or a live bookmark called "Bar".. when you open Foo and click on Bar (the actual folder item), the entire menu closes.

In the main title bar, single-clicking on a folder doesn't do anything, but clicking the same item again (after any delay) closes the menu.  In other Mac apps clicking on a submenu folder causes the menu item to show the "activate" animation and close the menu, but nothing happens.  I don't think we should ever tear down menus after a single click on an item that's a submenu.
Flags: blocking1.9?
So I can't reproduce this easily -- sometimes it's there, sometimes it's not; usually I see other breakage (like undo last close tab not showing up, the entire recently closed tabs menu being grayed out) when this happens.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P1
(Assignee)

Comment 2

11 years ago
Created attachment 288950 [details] [diff] [review]
fix v1.0
Attachment #288950 - Flags: review?(smichaud)
(Assignee)

Comment 3

11 years ago
BTW, the main part of this patch is ported from gtk2, see the "check_for_rollup" function in "widget/src/gtk2/nsWindow.cpp".
Comment on attachment 288950 [details] [diff] [review]
fix v1.0

I have only one minor problem with this patch -- I think
isEvent:overWindow: should have an early out for the case where
aWindow is nil (it should return PR_FALSE in this case).  This would
make it behave the same as its predecessor
(mouseEventIsOverRollupWidget:).

Otherwise (except for the new code in maybeRollup:) I think your patch
preserves all the previous logic, while also being much neater than
the previous code.

If I understand this bug report correctly, the new code in
maybeRollup: does fix the problem.

Here's how I tested:

I right-clicked on the Bookmarks toolbar, chose "New Folder", and
named the folder "Foo".  Then I right-clicked on the "Foo" menu, chose
"New Folder", and named the folder "Bar".

Prior to your patch I saw the following behavior:

1) Left-click on Foo and scroll down to Bar (an "Empty" menu item
   opens off to the right).
2) Left-click on Bar and the entire menu rolls up.

After your patch the behavior changed as follows:

1) Left-click on Foo and scroll down to Bar (an "Empty" menu item
   opens off to the right).
2) Left-click on Bar and nothing happens.  This is regardless of how
   many times you click on Bar.
Attachment #288950 - Flags: review?(smichaud) → review+
(Assignee)

Updated

11 years ago
Attachment #288950 - Flags: superreview?(roc)
(Assignee)

Comment 5

11 years ago
"I think isEvent:overWindow: should have an early out for the case where
aWindow is nil (it should return PR_FALSE in this case)"

Sure, I'll add that on checkin.
(Assignee)

Comment 6

11 years ago
Created attachment 289066 [details] [diff] [review]
fix v1.1

Actually, here is an updated patch.
Attachment #288950 - Attachment is obsolete: true
Attachment #289066 - Flags: superreview?(roc)
Attachment #288950 - Flags: superreview?(roc)

Comment 7

11 years ago
Comment on attachment 289066 [details] [diff] [review]
fix v1.1

nsISupportsArray is deprecated, and shouldn't be used.

You probably want nsIArray or one of the other array types available here:

http://developer.mozilla.org/en/docs/XPCOM_array_guide
(Assignee)

Comment 8

11 years ago
That usage is part of the nsIMenuRollup API, I'm not going to rewrite that for this.
Comment on attachment 289066 [details] [diff] [review]
fix v1.1

+  nsCOMPtr<nsIWidget> rollupWidget = do_QueryInterface(gRollupWidget);

Why is do_QueryInterface needed? I think you can just remove it.

+    NSWindow* currentPopup = (NSWindow*)rollupWidget->GetNativeData(NS_NATIVE_WINDOW);

static_cast

We need a bug on deCOMtaminating nsIMenuRollup... I'll file one.
Attachment #289066 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 10

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.