256505 - context menu for object in scrollbox cut off by bottom of screen (e.g. Chatzilla bottom tabs)

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Felix Miata]

Tested on current Win, Linux & OS/2 trunks with CZ 0.9.64 and above.

To reproduce:
1-Open Chatzilla
2-Move bottom of CZ window near bottom of screen
3-Right click a tab

Actual behavior:
1-Menu displays below tab, partially invisible due to bottom of screen

Expected behavior:
2-Complete menu displays above tab

Note that Shift-F10 brings up a different menu (bug 175568). This is also
distinguishable from that bug in that it always happens in the same manner, plus
has to do with tabs (chrome rather than content). Bug 236911 also should be
distinguishable, since according to James Ross, CSS visibility is not applicable
in the CZ tabs context.

Comment 1

[Felix Miata]

Attachment: the complete menu that should be displaying above when tabs near bottom of screen

Chatzilla 0.9.64 menu

Comment 2

[Felix Miata]

Attachment: screenshot showing only portion of menu, down to screen edge

Comment 3

[Felix Miata]

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

Comment 4

[:Gijs (he/him)]

Attachment: Testcase #1

Since the bug on which this was all mentioned was marked dupe, I'm copying the
relevant information (which seems to be missing here).

Testcase because ChatZilla uses a *lot* of stuff for its menus, and this should
make things simpler.

The bug is probably a regression, and a very old one at that. The bug was
tracked down to two trunk nightlies (one works, the next one doesn't) and a
Bonsai list of 11 people. This should altogether make it easier to find out
what causes this bug. It's not reproducable on releases like Firefox 1.0(.1) or
the Mozilla Suite 1.7.* series.

To quote tH:
Actually, I was using Firefox builds for testing. The 2004072708 build worked
okay, the menus get cut off as described in the 2004072812 build.

Herman Schwab:
Your new timing reduces the number of people from 16 to 11:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-07-27+05%3A00&maxdate=2004-07-28+12%3A00&cvsroot=%2Fcvsroot


I hope this can help solving this bug as soon as possible, because it's
downright annoying to lose half your menu when right clicking a tab...

Comment 5

[Hermann Schwab]

adding keywords testcase, regression 

timeframe 2004072708 - 2004072812

Comment 6

[[e-mail]]

Note that roc changed the way scrollboxes worked in that timeframe, and
ChatZilla scrolls its tabs in a scrollbox; too suspicious a coincidence?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That change has been backed out, so if you can still reproduce this, it's not
me. OTOH if you can't reproduce it now, it probably was me.

Comment 8

[Hermann Schwab]

(In reply to comment #7)
> That change has been backed out, so if you can still reproduce this, it's not
> me. OTOH if you can't reproduce it now, it probably was me.


I can´t reproduce it on chatzilla, but I still see it, when I right-click on the
testcase. So seems to me the Bug 286796 / testcase isn´t a dupe of this bug.
Do you remember when this patch got checked out?

Comment 9

[:Gijs (he/him)]

The way this testcase handles the tab right-clicking should be identical to what
happens in CZ, otherwise we would be looking at multiple things having an
influence on this (which would suck, imho).

tH and I tested more builds on windows and kde knoppix, it now seems
2004-07-27-12 functions, and 2004-07-28-08 does not. This narrows the list down
to 10 people:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=SeaMonkeyAll&branch=HEAD&sortby=Date&hours=2&date=explicit&mindate=2004-07-27+12%3A00&maxdate=2004-07-28+08%3A00&cvsroot=%2Fcvsroot

Comment 10

[Bill Gianopoulos [:WG9s]]

(In reply to comment #8)
> I can´t reproduce it on chatzilla, but I still see it, when I right-click on the
> testcase. So seems to me the Bug 286796 / testcase isn´t a dupe of this bug.
> Do you remember when this patch got checked out?
> 
I can still reproduce this with Chatzilla 0.9.67 using Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050402 Firefox/1.0+

Comment 11

[:Gijs (he/him)]

Changing summary to better reflect the issue (tabs and ChatZilla specifically
don't seem to have anything to do with the real issue here).

Changing the testcase to use a checkbox or a button (for example) also produces
the bug. Removing the scrollbox fixes it instantly.

Roc, judging from
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsScrollBoxObject.cpp&rev=1.15&root=/cvsroot
your changes were not removed? 

My current guess (very vague, but I don't know any C(++), so slap me if it's
useless) is that because this patch introduces the box as a frame, the frame
height calculations in
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsMenuPopupFrame.cpp#805
mess up. I'm not at all sure, however, just a guess for now. If someone with
better means and more knowledge than me would be able to debug this, then that
would definately be helpful.

Comment 12

[Felix Miata]

With that summary no one who has no clue that Chatzilla's bottom tabs are in a
scrollbox, which isjust about everyone, is going to have any chance of finding
this bug searching only summaries. Changing summary to make if findable by
people finding the behavior.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry, I was confused about which checkin was at fault :-).

Comment 14

[:Gijs (he/him)]

I'm requesting blocking aviary 1.1 and mozilla 1.8b2. The context menu's just
going off-screen make them unusable, which is a very bad bug, imho. The
screenshot should give a pretty good idea of the problem here. This is a serious
regression, and in my opinion this should be fixed.

We have a timeframe, and it's very likely the checkin mentioned is what's
causing this. We just don't have anyone doing a patch.

CC-ing Brian Ryner and Boris Zbarsky, who seem to know this code.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's probably me.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

I'm not sure how this worked before, but there are two problems in the
SyncViewWithFrame code that I'm fixing:
-- aFrame->GetOffsetFromView doesn't consider whether aFrame itself has a view.
It seems that if aFrame has a view, we should make that the containingView.
Then if aFrame is a scrolled frame, we will successfully find the scrollable
view as the parent of containingView. Note that this change doesn't change the
meaning of 'offset', it continues to be the offset from aFrame's origin to the
origin of containingView.
-- We use GetNearestWidget to find the nearest widget for containingView and
then ignore the offset it returns! This can't be right. We should add the
offset to the coordinates we're building up.

Fixing these issues makes this testcase work and other menus seem to work as
before (tested scrolling menus, context menus in nested iframes, submenus and
menus flipping right-left and bottom-top).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 179786 [details] [diff] [review]
fix

>Index: layout/xul/base/src/nsMenuPopupFrame.cpp
>+  PRInt32 screenViewLocX = NSIntPixelsToTwips(screenParentWidgetRect.x,p2t) +
>+    (xpos - parentPos.x + parentViewWidgetOffset.x);

I think this would be clearer with the ')' after the |parentPos.x| (so we're
adding up the widget position, our offset from the view, and the view's offset
from the widget).

>+  PRInt32 screenViewLocY = NSIntPixelsToTwips(screenParentWidgetRect.y,p2t) +
>+    (ypos - parentPos.y + parentViewWidgetOffset.y);

Same here.

r+sr=bzbarsky with that.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in

Comment 19

[Felix Miata]

Verified fixed in 2005040610 OS/2 trunk. Thank you.

Comment 20

[:Gijs (he/him)]

Clearing blocking flags. Roc, thanks for fixing this :-).

Comment 21

[Boris Zbarsky [:bzbarsky]]

This caused bug 291390