BUILD: pulled from CVS at "Wed Apr 3 00:14:01 EST 2002" (includes patch from bug 108099). STEPS TO REPRODUCE: 1) Load attached testcase. 2) Make sure iframe is near bottom of screen 3) Right-click in iframe 4) Go to the "Frame Options" submenu of the context menu EXPECTED RESULTS: All nine frame options shown ACTUAL RESULTS: eight of the nine options shown and scroll arrows are present allowing scrolling to the ninth option. This only happens when the submenu comes up near the bottom of the screen, however.

hm, the testcase seems to be gobbledeegook for me. here's another testcase: 1. go to http://wired.com/ 2. bring up context menu in bottommost frame. see this XP. pink, could this be related to an existing bug? methinks i've seen this before in, say, a really long bookmarks menu.

what is weird is that if you go to warp (which has two veritcal frames), you don't get the scroll arrows on the submenu. Everything is ok. I don't recall and outstanding bugs on menus having scrollarrows when they shouldn't.

As I said, this only happens when the submenu comes up near the bottom of the screen. On the vertical frames page, click near the very bottom of the frame to bring up the context menu.

The problem is more general than reported, it happens on *all* submenus. The easy way to reproduce it is this: - move the window down until the menubar is relatively close to the bottom of the screen - open a submenu that is big enough to reach the bottom of the screen - the submenu will display all options minus one, and scroll arrows will be enabled. I can reproduce this on Win2000 and Linux, both modern and classic skin. Also I have been seeing this behaviour for quite some time, probably before 0.9.9.

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

Transferring dependency from duplicate

This is likely related to bug 84121, as Dean says in bug 139228

I'm not convinced it is related to 84121. I set the same breakpoints in nsMenuPopupFrame.cpp that I set for that problem and I do NOT hit them when a submenu of a context menu is displayed. It seems like submenus are following a totally different path. If someone could give me that path, that would be helpful. For now, I will look at backing out my change and see how it affects this problem.

Interestingly, I see this on my home machine, but not on the machine at uni. Or at least I don't notice it there - I'll confirm that next week.

removing dependency that was added by a not-driver.

oops, sorry, didn't see this was a dupe transfer.

restoring dependency clobbered by stephan, who presumably had a very good reason to do that...

removing from RC2 list.

This fixes this problem for me.

Comment on attachment 81665 [details] [diff] [review] patch v1 Fixes it for me too. r=mkaply

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

Michiel, have you requested super-review for that patch? See http://mozilla.org/hacking/ and http://mozilla.org/hacking/reviewers.html (I recommend hyatt@netscape.com as the sr to ask).

hyatt's on vacation, but I did ask bryner to look at this, and he and I could not figure out why the patch is correct. It may work, but so would adding random numbers of appropriate magnitude. The patch treats mRect and parentRect as if they were stacked vertically. But they aren't, as far as bryner and I can tell. Can someone explain why the patch is correct (not why it just happens to work)? Thanks, /be

I believe that in this case, parentRect is referring to the part of the combobox that is displayed before the box is popped down. So the issue was that the height of parent rect was not being taken into consideration when doing the repositioning, just the height of the mRect which was the listbox that appears when you pop it down.

Boris: I actually asked hyatt, but did not know he was on vacation. I didn't had time to look for someone else. Brendan: This bug seems to be a regreesion of bug 84121. The patch there has: - PRInt32 ySpillage = (screenViewLocY + mRect.height) - screenBottomTwips; + PRInt32 ySpillage = (screenViewLocY + mRect.height + parentRect.height) - screenHeightTwips; (This is a few lines down of where my patch is.) Here, parentRect is used in the calulation wheter the arrows are needed. But parentRect is not used to move the menu up. That us what my patch does.

Comment on attachment 81665 [details] [diff] [review] patch v1 sr=brendan@mozilla.org, and I'm proposing this for 1.0. /be

Comment on attachment 81665 [details] [diff] [review] patch v1 a=chofmann please check in to 1.0 branch asap to get this in rc3.

Comment on attachment 81665 [details] [diff] [review] patch v1 I double sr= you! sr=ben@netscape.com (not sure what box to check)

I wonder if this patch will fix bug 84121 for good as well.

From an r= point of view, I'd like to see a comment, with an ascii art diagram, added to this section of code (with arrows, and a key, as I've added to other sections of this code) explaining how it all works, so we don't forget, and so other people can figure it out in an expeditious manner.

who can get this on the branch? hyatt is on sabatical. need some heavy lifting.

OK, I thought about this some more, and realized I know little enough to confuse myself. My sr= stands for technical reasonableness, but I don't think this should get r= until the function of this patch is clearly explained in english with diagrams. I'm not module owner here and neither is mkaply, the previous module owner who originally rewrote all this is a pansy who "absolves himself of all responsibility" *kicks pinkerton*. So I'm going to step in and assert partial ownership since I've touched some positioning stuff before, and say "let's not do anything with this until it's well documented." Michiel, please produce a new patch with a large comment diagram as I requested in my earlier comment. I know you're not responsible for the confusing nature and original lack of comments, but I figure since you're editing it, you *must* have understood it and thus can quickly produce some artwork showing everyone else why your patch is great. This is some of the most complicated code in the product. No tricky algorithms or other such matter, just a lot of basic math ;-)

Comment on attachment 81665 [details] [diff] [review] patch v1 I'm going to remove mkaply's r= from this and add 'needs work' until we get the comments I asked for ;-)

I made the first patch by lokking at what the patch in bug 84121 did. Now, when trying to draw the images asked for, I found what that patch should have done. So here is my patch with the images, and a better fix. It alse seems to fix bug 84121 and bug 100986 as they should. Hope my asci-art is clear.

As someone that hates the menu code (but can't seem to keep his hands out of it), those drawings make sense to me.

So, just to make sure I understand what's going on here... the problem is that |xpos| and |ypos| are relative to |containingView|, but we were computing the screen location using them as an offset from |parentView|? So we would be off by the value of |parentPos|, which will be the size of the scroll arrow?

Yes, Thats seems to be the problem. But we are not off bij |parentPos|. |parentPos| is the position of the first menu-item (When we are looking at a menu.) |parentRect| starts at the top of the menu, including the arrow. We are off by the difference of the two. In bug 84121 this was fixed by using some corrections the size of |parentRect.height|. This is the height of the selected menuitem, that opens a submenu. That is almost the same size as the arrow. So in most cases, is looked ok. But the calculation to see if the menu fits, didn't use the correction.

Can we get r= and sr= again, this time for sure? Thanks to Michiel for all the great work here, ripping out bad wallpaper from those other bugs, even! /be

Comment on attachment 84172 [details] [diff] [review] patch v2 >+ // Use containingView instead of parentView, to acouont for the scrollarrows >+ // that a parentmenu might have. >+ Fix the typo there and r=bryner.

Comment on attachment 84172 [details] [diff] [review] patch v2 Heh. The diagram rule always catches people out ;-) Just a few nitpicks about the comments: + // Use containingView instead of parentView, to acouont for the scrollarrows + // that a parentmenu might have. in addition to the typo as bryner mentions: - parent menu is two words ;-) fix that also and sr=ben@netscape.com

Fix checked in on trunk.

We need this on the 1.0 branch too. Ben, can you do the join and checkin? /be

Comment on attachment 84172 [details] [diff] [review] patch v2 marking a=brendan

checked in on the 1.0 branch.

Can't reproduce this anymore using 2002052404 on Win2K. Verified Fixed.

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

verified on the branch using 2002.06.27.0x comm bits, all platforms.