135076 - Frame context submenu has scroll arrows

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase

Comment 2

[sairuh (rarely reading bugmail)]

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.

Comment 3

[Mike Pinkerton (not reading bugmail)]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Pedro Lopes]

Attachment: screenshot

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Transferring dependency from duplicate

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Mike Kaply [:mkaply]]

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.

Comment 10

[Bradley Baetz (:bbaetz)]

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.

Comment 11

[Asa Dotzler [:asa]]

removing dependency that was added by a not-driver.

Comment 12

[Asa Dotzler [:asa]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Asa Dotzler [:asa]]

removing from RC2 list.

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

This fixes this problem for me.

Comment 16

[Mike Kaply [:mkaply]]

Comment on attachment 81665 [details] [diff] [review]
patch v1

Fixes it for me too.
r=mkaply

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[Mike Kaply [:mkaply]]

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.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

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 22

[Brendan Eich [:brendan]]

Comment on attachment 81665 [details] [diff] [review]
patch v1

sr=[e-mail], and I'm proposing this for 1.0.

/be

Comment 23

[Dawn Endico]

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 24

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

Comment on attachment 81665 [details] [diff] [review]
patch v1

I double sr= you! 

sr=[e-mail] (not sure what box to check)

Comment 25

[:deanis74]

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

Comment 26

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

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. 

Comment 27

[chris hofmann]

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

Comment 28

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

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 29

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

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 ;-)

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

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.

Comment 31

[:deanis74]

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

Comment 32

[Brian Ryner (not reading)]

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?

Comment 33

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 34

[Brendan Eich [:brendan]]

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 35

[Brian Ryner (not reading)]

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 36

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

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=[e-mail]

Comment 37

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

Fix checked in on trunk. 

Comment 38

[Brendan Eich [:brendan]]

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

/be

Comment 39

[Dawn Endico]

Comment on attachment 84172 [details] [diff] [review]
patch v2

marking a=brendan

Comment 40

[Brian Ryner (not reading)]

checked in on the 1.0 branch.

Comment 41

[:deanis74]

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

Comment 42

[Mike Potter]

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

Comment 43

[sairuh (rarely reading bugmail)]

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