Closed Bug 397111 Opened 12 years ago Closed 12 years ago

Places Organizer , next menu won't expand when the pointer gets there

Categories

(Firefox :: Bookmarks & History, defect, P3)

x86
All
defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta3

People

(Reporter: Peter6, Assigned: martijn.martijn)

References

Details

(Whiteboard: [has patch][has review])

Attachments

(1 file, 1 obsolete file)

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092112 Minefield/3.0a8pre ID:2007092112

repro:
Open FF
menu [Bookmarks] -> [Organize Bookmarks]
In the Places Organizer window click on menuitem [Organize] (the menu expands)
move the pointer to the right, over [Views] or [Import and Backup]

result:
The [Organize] does not collapse and [Views] or [Import and Backup] don't expand

expected:
behave like main menubar in FF
Linux trunk build also has the same problem.
OS: Windows XP → All
Duplicate of this bug: 397195
Blocks: 393514
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Target Milestone: --- → Firefox 3 M11
Duplicate of this bug: 406574
Attached patch patch by Martijn Wargers (obsolete) — Splinter Review
This patch has been posted by on Bug 406574

He said:
> Press "Organize" in Places Organizer. A menu will show up. Drag the mouse to
> the right. I would expect "Views" to pop up now, but this does not happen.
> 
> This makes it work for me. I basically copied this from bug 203556.
>
Attachment #291427 - Flags: review?(mano)
Duplicate of this bug: 407480
Assignee: nobody → martijn.martijn
Status: NEW → ASSIGNED
Comment on attachment 291427 [details] [diff] [review]
patch by Martijn Wargers

>Index: browser/components/places/content/places.js
>===================================================================

>+var PlacesToolbar = {
>+  // make places toolbar act like menus
>+  openedMenuButton: null,
>+  autoOpenMenu: function (aTarget)
>+  {
>+    if (this.openedMenuButton &&
>+        this.openedMenuButton != aTarget &&
>+        aTarget.localName == "toolbarbutton" &&
>+        (aTarget.type == "menu" ||
>+         aTarget.type == "menu-button")) {

in fewer lines please, could also just check the "menu-button" case.

r=mano otherwise.
Attachment #291427 - Flags: review?(mano) → review+
Duplicate of this bug: 409542
Whiteboard: [has patch][has review][needs final patch for landing]
(In reply to comment #4)
> Created an attachment (id=291427) [details]
> patch by Martijn Wargers

I tested the patch.
It seems good for me.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2007122311 Minefield/3.0b3pre
Martjin i hope you don't mind if i addressed review comments in your patch (only changed some newline)

Mano: menu check cannot be taken away, making only menu-button check does not work

bringing over Mano's review
Attachment #291427 - Attachment is obsolete: true
Attachment #294416 - Flags: review+
notice that the toolbar acts like a menu with mouse but not with keyboard right/left cursor (should i fill a bug on that?)
Keywords: checkin-needed
Whiteboard: [has patch][has review][needs final patch for landing] → [has patch][has review]
(In reply to comment #10)
> notice that the toolbar acts like a menu with mouse but not with keyboard

Maybe we should use an actual menubar from the start?
(In reply to comment #11)
> (In reply to comment #10)
> > notice that the toolbar acts like a menu with mouse but not with keyboard
> 
> Maybe we should use an actual menubar from the start?
> 

I would vote for this, too. I guess the Mac version will try to style it as a menu too, and there is a bug to remove the icons for Linux builds. 
Checking in places.xul;
/cvsroot/mozilla/browser/components/places/content/places.xul,v  <--  places.xul

new revision: 1.100; previous revision: 1.99
done
Checking in places.js;
/cvsroot/mozilla/browser/components/places/content/places.js,v  <--  places.js
new revision: 1.120; previous revision: 1.119
done

Marco, thanks for updating the patch. I checked this in now.

There is probably a bug filed for using a normal menubar instead.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Marvellous; verified fixed.
Status: RESOLVED → VERIFIED
[[ there is a bug to remove the icons for Linux builds ]]
What icons?
The icons in front of the "menus"/buttons (Organize, View,  Import). The current icons normally have other meanings, so they don't really help at all.
So, Firefox must send request for icons (to freedesktop).
No, Firefox should be using a good old menu bar without icons. That's the underlying issue and "There is probably a bug filed" isn't really satisfying, which is why I wouldn't have resolved this bug. Re-implementing all menu bar features to emulate the correct behavior can't be the way to go.
"There is probably a bug filed" isn't really a satisfying answer, but this bug was only about the "Places Organizer , next menu won't expand when the pointer gets there" issue.
Feel free to reopen this bug, if you think this bug should be about the "Firefox should be using a good old menu bar without icons" issue.
(In reply to comment #19)
> Feel free to reopen this bug, if you think this bug should be about the
> "Firefox should be using a good old menu bar without icons" issue.

Ok, iiuc, this will happen in bug 400703.
@Dao: menubars should die :P . They're old and not idiot-friendly :P .
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.