Closed Bug 143130 Opened 22 years ago Closed 22 years ago

ALT or F10 keys do not select menu items in MfcEmbed

Categories

(Core Graveyard :: Embedding: APIs, defect)

Product:
Core Graveyard
Component:
Embedding: APIs
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: chak, Assigned: deanis74)

Details

(Keywords: topembed+)

Attachments

(1 file, 1 obsolete file)

new patch, untested in mfcembed
1.60 KB, patch
aaronlev
: review+
alecf
: superreview+
Details | Diff | Splinter Review 
1. Launch MfcEmbed and load any URL into the window.

2. Press the ALT or F10 key - nothing happens
[However, pressing Alt-F, Alt-E etc. selects the appropriate menu items]

3. Clicking on the ALT or F10 key in Netscape 6.2 shifts focus to the first
application level menu item i.e. to the "File" menu item.

This behaviour is important in the case of customers embedding Gekco into their
own custom applications.
Keywords: topembed+
This looks to be the offending code, from hyatt. It's affecting a number of
embedded clients.

Should we wrap it in #ifdef MOZ_XUL or something?
I wish the comment was a little more specific about what the problem is.
Hyatt, can you explain?

           if (wParam == VK_MENU || (wParam == VK_F10 && !mIsShiftDown)) {
              // This is required to prevent Windows
              // default menu processing getting in the
              // way of XP menus and key handling.
              // Without this we call DefWindowProc which will
              // send us WM_COMMAND and/or WM_SYSCOMMAND messages.
              // Do not remove!  Talk to me if you have
              // questions. - hyatt@netscape.com
              result = PR_TRUE;
              *aRetValue = 0;          
            }
Isn't this where we trap an Alt keypress and activate our menu bar instead of
letting the keypress through to Windows?
Probably, should we be changing that in embedded clients, so that the keypress
gets through?
I recommend we surround hyatt's lines with 
#ifdef MOZ_XUL
#endif

Hyatt's comment says this is for XP menus.
Aaron,

Can you explain how MOZ_XUL is set ?
And where ?

Thanks
MOZ_XUL is set by the configure.in script, which uses .mozconfig to see what the
build options are. 

I hope embeddors already have --disable-xul when they build.
I already use #ifdef MOZ_XUL in the accessibility module to reduce the code
footprint for embeddors who don't use XUL. Or, maybe there's another #ifdef all
embeddors use that we can take advantage of.

BTW, have you seen the build configurator? It creates output for .mozconfig
http://webtools.mozilla.org/build/config.cgi

--disable-xul doesn't work at present, and I'm not convinced that #ifdef'ing
this section of code is the right fix.  I'd say instead this should be
conditional on the presence of an nsXULWindow, or something similar.

Yes, or if you want to go further, on the presence of a <menubar> within the 
XUL window.  The widget code is purely about making XP menus work when you 
have no native menus in the window.  If you have native menus in the window, 
this code is going to get in your way and shouldn't fire.

We could do some sort of callout mechanism, where the widget code calls out to 
(through an interface) to find out if it's a full-blown XUL window (with no 
embedding wrapper).

How about doing the opposite and letting the keypress pass through if there's a
native menu?  Just check if GetMenu() on the top-most window returns non-null.
Sounds good to me.  Just to be paranoid, will that work correctly in MDI
embedding cases, where the "top"level window (in one sense) isn't the one
containing the menubar?
What do you mean by that?  When will the top-level window not contain the menu bar?
Perhaps something like this...

PRBool hasMenu = PR_FALSE;
while (hWnd) {
  if (::GetMenu(hWnd)) {
    hasMenu = PR_TRUE;
    break;
  }
  hWnd = GetWindow(hWnd, GW_OWNER);  // or maybe GetParent(hWnd)
}

if (hasMenu) {
  // there's a window with a native windows menu - let it handle the keypress
  result = PR_FALSE;
  break;
} else {
  // handle the keypress
  result = PR_TRUE;
  *aRetValue = 0;
};
I'm just indicating my ignorance of MDI, and wanting to make sure that such an
approach would work in that case.
Here's a patch that implements what I described in comment 12.	Our menus still
work fine using this method, but I don't have MFC installed so I can't test
MFCEmbed.  Could someone give it a try?
Comment on attachment 98393 [details] [diff] [review]
proposed patch, untested in MFCEmbed

I tested this is both mozilla.exe and mfcembed.exe - and it works great. Code
looks nice too. r=aaronl, but could you please update the old comment that says
to contact hyatt@netscape.com, that's not even his email anymore. Instead, I
would perhaps explain the change and refer to this bug, since people are more
likely to get their answer from here now.

	      // This is required to prevent Windows
	      // default menu processing getting in the
	      // way of XP menus and key handling.
	      // Without this we call DefWindowProc which will 
	      // send us WM_COMMAND and/or WM_SYSCOMMAND messages.
	      // Do not remove!  Talk to me if you have
	      // questions. - hyatt@netscape.com
Attachment #98393 - Flags: review+
Thank Brendan for the pretty code, he sent me a few comments in email about my
original suggestion.

Why do we need that comment in there at all anymore?  It's obviously not the
case, and there are some comments in the new code.  There's really no need to
reference this bug, we don't do that for most of the other fixes we make.  How
about adding something simple before the hasMenu check...

// We need to pass this keypress on to Windows if there's a native
// menu bar somewhere in our containing window, otherwise we handle it.

dean: Doesn't *aRetValue get set to 0 at the top of the method?  If so, why set
it to 0 again?

aaronl: We don't need to reference bug#'s for most fixes, cvs annotate and the
checkin comment are enough to find out which bug motivated what code change.

/be
Yep, you're right Brendan.  I didn't see that before, I just used the existing
code.  So that can be trimmed down to simply |result = !hasMenu;|.

Dean, do you have plans to finish this or would you like someone from Netscape's
embedding team to take over the patch. It doesn't look like it's far from being
finished.
I'll do up a new patch when I get home tonight.
This patch takes into account comment 15, comment 17, and comment 18.  Again,
it works fine in mozilla.exe but I can't test it in mfcembed.exe.
Attachment #98393 - Attachment is obsolete: true
Comment on attachment 99454 [details] [diff] [review]
new patch, untested in mfcembed

I tested, this works in both Mozilla.exe and mfcembed.exe 

r=aaronl
Attachment #99454 - Flags: review+
Comment on attachment 99454 [details] [diff] [review]
new patch, untested in mfcembed

sr=alecf
(by the way, hyatt's new e-mail address, which you can pick up from the mailto:
links in the above comments, is hyatt@apple.com)
Attachment #99454 - Flags: superreview+
Arron,

Did the patch really work for MFCEmbed.exe ?

It did not for me.
Looking at the properties of window of embedded mozilla
by using SPY++ reveals that the mozilla window ( embedded ) did not have
any owner, but it has a parent.

So it failed to go up the chain when the first call to GetWindow(hWnd, GW_OWNER)
returns the NULL.

Also it did not work for the embedding client I am using either.

I switched GetWindow(hWnd, GW_OWNER) with ::GetParent(hWnd), and
it worked for mozilla.exe and MFCEmbed.exe and also the embedding client
I am using.

I think we should use GetParent() instead.
Isn't it part of the requirements for embeddor to provide, for mozilla, 
a site window, which is child window, which in turn can not be 
the owner window by Windows definition ?

Bottomline is that I think 
the patch needs more investigation before being final.

Thank you.

HK ( Hansoo Kim )
Actually, that makes sense to me.  All containing windows may have the same
owner, thus avoiding the loop.  Using GetParent will move up the window
hierarchy, which is the intended behavior.

Aaron and Alec, will your reviews still stand with that change, or do you want a
new patch?

(Taking bug from saari.)
Assignee: saari → dean_tessman
You can carry my r= with that change.
QA Contact: mdunn → bmartin
Alec, can I carry your sr= forward with that minor change?
yeah, that's fine.
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Brent please verify this fix on branch and trunk
mfcembed 2002-11-08-08-trunk:
Selecting Alt and F10 shifts focus to the first application level menu item
"File" as expected.

mfcembed 2002-11-08-08-1.0 (branch):
Selecting Alt and F10 will only shift focus to the first application level menu
item "File" IF the URL text is highlighted.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Since it is verified fixed on the trunk. We can leave it since it is not
nominated for the branch.
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
--> verified on the trunk
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: