225732 - Menu items don't activate properly (see comment 12)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jerry Baker]

2003111314 trunk (Tinderbox build)

It appear that all of the menus are messed up in Mozilla. Menu items do not
activate unless you mouseover the actual text of the menu item. This is contrary
to how Mozilla used to behave, and contrary to how all other Windows
applications behave. Menu items are supposed to activate any time the mouse is
over the menu anywhere. This is also a regression.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> This is also a regression.

Any chance of determining the regression window?

Comment 2

[Jerry Baker]

Not at all possible. This is because there haven't been any Win32 builds since
10-28. I can tell you that it worked fine then, and that's it. If there is a
repository of Tinderbox builds, I can look through them.

Comment 3

[Boris Zbarsky [:bzbarsky]]

There is no such beast.  Is this a problem with Firebird too?  Are there
Firebird builds from that time period?

This bug needs a real owner, since hyatt is not likely to be fixing it and I'm
not that owner (since I have no windows build, hence can't even tell what the
bug is about based on the description...)

Comment 4

[Jerry Baker]

There are no Firebird builds since 10-28 either. I have tried building from
source, but then there are problems. See bug 225059.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ugh.  ccing some people who may have windows builds to hand.  A two-week
regression window is huge... :(

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

well... I don't have 2003111314 available, but 2003111209 (also tinderbox build)
seems to behave just fine.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Jerry, could you look in
ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/tinderbox-builds/ and see whether
the builds there help?  Those go back further than I expected...

Comment 8

[Jerry Baker]

Luckily, the 2003111209 build does work correctly as Christian reported.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I downloaded the latest build and I do see this bug now (2003111413)...

so regression range seems to be 11/12/2003 09:00 to 11/13/2003 14:00

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe this one?
11/12/2003 20:13	bzbarsky%mit.edu 	mozilla/ layout/ xul/ base/ src/
nsBoxFrame.cpp 	1.224 	1/5  	BoxFrame should only be the point target in the
background layer, and should
propagate background layer point requests to kids. Bug 224013, r+sr=dbaron

(bonsai link for aforementioned range is
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F12%2F2003+09%3A00&maxdate=11%2F13%2F2003+14%3A00&cvsroot=%2Fcvsroot
)

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, that's the one I was thinking about.  Could someone clearly explain to me
what this bug is about?  Menus work fine over here in a Linux build, so what are
the _exact_ steps to reproduce?

Comment 12

[Jerry Baker]

For one example:

1. Click on the Tools menu.
2. Run your mouse down the right side of the menu (so it's near the right margin).

WHAT HAPPENS:
Nothing

SHOULD HAPPEN:
Each menu item should activate as you slide down the menu.

Comment 13

[Jerry Baker]

Make sure you're in-between the arrows and the words. It seems contrived, but
believe me it gets really weird real fast when using Mozilla. I noticed it
within a few minutes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, there we go.  That should have been in comment 0...

I do see this on Linux, and this is indeed a regression from bug 224013.

Anyone know how menu "activation" works exactly?  ;)

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw, another way this bug shows is this: click a bit to the right/left of the
menu text in the menu bar, and observe that no menu appears

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Never mind. This patch fixes it

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Slightly more extensive

Ok, this time I decided I'd look at all the GetFrameForPoint impls in XUL,
since they seem to have some similar issues...

The ones I didn't change seem fine as they are.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135517 [details] [diff] [review]
Slightly more extensive

David, what do you think?  I've run through a bunch of our UI trying to
exercise various XUL issues and not seeing any problems (and the menus are
fixed).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 135517 [details] [diff] [review]
Slightly more extensive

>Index: nsDeckFrame.cpp
>+  // if its not in our child just return us.
>+  *aFrame = this;

Need to check background paint layer before doing this (and do the correct
return value).

>Index: nsMenuFrame.cpp
>   if ((result != NS_OK) || (*aFrame == this)) {

Don't test |*aFrame| here -- it's a UMR, no?

>Index: nsSliderFrame.cpp
>+
>+  if (!mRect.Contains(aPoint))
>+    return NS_ERROR_FAILURE;
> 
>   // always return us (if visible)
>   if (GetStyleVisibility()->IsVisible()) {
>     *aFrame = this;
>     return NS_OK;
>   }
> 
>   return NS_ERROR_FAILURE;

Just merge these, i.e., if (mRect.Contains(aPoint) && Get...

>Index: nsSplitterFrame.cpp
>+  nsresult rv = nsBoxFrame::GetFrameForPoint(aPresContext, aPoint, aWhichLayer, aFrame);
>+  if (rv == NS_ERROR_FAILURE && mRect.Contains(aPoint)) {

Probably you should also test |aWhichLayer == ...FOREGROUND| in between these
two tests.

>+    *aFrame = this;
>+    rv = NS_OK;
>+  } 
> 
>+  return rv;
> }

Otherwise, looks fine to me.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Need to check background paint layer before doing this

Done.

> Don't test |*aFrame| here -- it's a UMR, no?

No.  *aFrame is looked at only if nsBoxFrame::GetFrameForPoint returned NS_OK. 
I'll change that to use NS_FAILED to make things clearer.

> Probably you should also test |aWhichLayer == ...FOREGROUND| in between these
> two tests.

Done.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to those comments

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in.