Frame context submenu has scroll arrows

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: bzbarsky, Assigned: hyatt)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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.
OS: Linux → All
QA Contact: shrir → sairuh
Hardware: PC → All
Summary: Frame submenu has scroll arrows → Frame context submenu has scroll arrows
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.

Comment 5

17 years ago
Created attachment 80205 [details]
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.
*** Bug 139228 has been marked as a duplicate of this bug. ***
Transferring dependency from duplicate
Blocks: 138000
This is likely related to bug 84121, as Dean says in bug 139228

Comment 9

17 years ago
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.

Comment 11

17 years ago
removing dependency that was added by a not-driver.
No longer blocks: 138000

Comment 12

17 years ago
oops, sorry, didn't see this was a dupe transfer. 
Blocks: 138000

Updated

17 years ago
No longer blocks: 138000
restoring dependency clobbered by stephan, who presumably had a very good reason
to do that...
Blocks: 138000

Comment 14

17 years ago
removing from RC2 list.
No longer blocks: 138000
Created attachment 81665 [details] [diff] [review]
patch v1

This fixes this problem for me.
Keywords: patch, review

Comment 16

17 years ago
Comment on attachment 81665 [details] [diff] [review]
patch v1

Fixes it for me too.
r=mkaply
Attachment #81665 - Flags: review+
*** 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

Comment 20

17 years ago
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
Attachment #81665 - Flags: superreview+

Updated

17 years ago
Blocks: 143200

Comment 23

17 years ago
Comment on attachment 81665 [details] [diff] [review]
patch v1

a=chofmann
please check in to 1.0 branch asap to get this in rc3.
Attachment #81665 - Flags: approval+
Comment on attachment 81665 [details] [diff] [review]
patch v1

I double sr= you! 

sr=ben@netscape.com (not sure what box to check)

Comment 25

17 years ago
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. 

Comment 27

17 years ago
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 ;-)
Attachment #81665 - Flags: review+
Attachment #81665 - Flags: needs-work+
Attachment #81665 - Flags: approval+
Created attachment 84172 [details] [diff] [review]
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

17 years ago
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.
Attachment #84172 - Flags: review+
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
Attachment #84172 - Flags: superreview+
Fix checked in on trunk. 
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
We need this on the 1.0 branch too.  Ben, can you do the join and checkin?

/be

Comment 39

17 years ago
Comment on attachment 84172 [details] [diff] [review]
patch v2

marking a=brendan
Attachment #84172 - Flags: approval+
Keywords: fixed1.0.0
checked in on the 1.0 branch.

Updated

17 years ago
No longer blocks: 143200

Comment 41

17 years ago
Can't reproduce this anymore using 2002052404 on Win2K.  Verified Fixed.
Status: RESOLVED → VERIFIED

Comment 42

16 years ago
*** Bug 145304 has been marked as a duplicate of this bug. ***
verified on the branch using 2002.06.27.0x comm bits, all platforms.
Keywords: fixed1.0.0 → verified1.0.0

Updated

10 years ago
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: bugzilla → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.