66848 - Fix all the menu positioning problems that matter, and don't regress anything, beyotch.

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

There are menu positioning problems with submenus of scrolling menus that are 
themselves submenus. 

This is due to a couple of problems:
- The widget that is being used to position menus is incorrect, it's not the 
widget associated with the parent view of the spawning frame, but the root view 
of the popup window widget that contains the frame.
- The root view is the client, not the screen, and better care when adding up 
numbers needs to be made. 

Patch coming.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: fix this bug (includes diffs for resizable popups which have already been reviewed. oops)

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: required changes to widget

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Needed to add a 'GetWindowType' method to nsIWidget (& implement it on 
nsBaseWidget, it is the analog of SetWindowType) so that I can figure out when a 
view is the root view of a popup window. It seems that this sort of method might 
be useful in other places too. 

Comment 4

[timeless]

+ PRBool ParentIsScrollableView(nsIView* aStartView);
+ PRBool ParentIsScrollableView(nsIView* aStartView)
?? and why aren't you using -u like everyone else?

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

How about I use whatever style of diff I like, and you mind your own business?

The lines you reference represent an internal helper method. 

Comment 6

[Brendan Eich [:brendan]]

Use diff -u, beyotch!

/be

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

No, I prefer diff -c because I find it easier to read. As the layout/xul part of 
this patch cannot easily be applied (since it contains portions of another patch 
which has already been reviewed by pinkerton, and as a result are not posted 
here), I chose readability over ease of patching. I could just as easily sent 
out these changes via email, and this bug not existed at all. You pick. 

Comment 8

[rods (gone)]

Looks good r=rods

Comment 9

[Mike Pinkerton (not reading bugmail)]

r=pinkerton, contingent on:
- building and tested on mac (i want to see it)
- removal/cleanup of |nsMacWindow::GetWindowType()|

Comment 10

[Mike Pinkerton (not reading bugmail)]

i recind my r=pink. i tried this patch and context menus aren't correctly 
positioned.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: a patch that you can apply

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: updates to widget/src/mac

Comment 13

[ajbu]

W2k, fresh source this morning, runs without patching.

I wanted to test out these patches. Patches still work on current tree. 
Applying the patch to nsMenuPopupFrame (attach_id=27305) makes me crash on 
gklayout.dll when opening any menu (also rightclick/contextmenu)
Applying both attach_id=27305 and attach_id=23670 (nsIWidget and nsBaseWidget) 
makes me crash at startup in gkwidget.dll

Comment 14

[ajbu]

Ignore my previous comment. After a complete clobber/rebuild it worked without 
crashes.
The only positioning problem I see after applying this patch is that if a 
submenu of a scrollable menu is ‘bounced’ of the bottom of the screen, or the 
submenu is so large it is a scrollable submenu itself, it is positioned (one 
menurow?) to low.
So a big scrollable submenu (larger then the height of the screen is positioned 
(top=1, bottom=screenheight+1, height=screenheight) instead of (top=0, 
bottom=screenheight).
Submenu’s aligned to the bottom of the screen are aligned to (screenheight+1) 
instead screenheight. This seems independent of the scrolled position of the 
scrollable menu.

Not sure my description is clear so I’ll attach two screenshots.

Comment 15

[ajbu]

Attachment: submenu of scrollable menu aligned to screenheight+1, last item of menu is offscreen (one more item after VOYAGER..)

Comment 16

[ajbu]

Attachment: The IE Favorites menu is positioned one menurow toolow (space above and arrow is offscreen)

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Throwing into .9.1 so I don't forget it. I'll investigate ajbanck's findings 
during that cycle.

Comment 18

[Mike Pinkerton (not reading bugmail)]

Attachment: [patch] ben's patch, all in one file

Comment 19

[Paul Chen]

nav triage team:

marking nsbeta1+ and p3

Comment 20

[Viswanath Ramachandran]

as discussed in team meeting, moving all Nav+ team members nsbeta1+ P3 bugs from 
mozilla0.9.1 to mozilla0.9.2. 

Comment 21

[Paul Chen]

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

Comment 22

[Paul Chen]

nav triage team:

Reassigning to pchen who has a 0.9.1 bug that uses this patch. Will mark this 
fixed once that bug is checked in.

Comment 23

[Paul Chen]

Oooops, I checked this fix in for mozilla0.9.1.