ALT or F10 keys do not select menu items in MfcEmbed

VERIFIED FIXED

Status

()

Core
Embedding: APIs
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: Chak Nanga, Assigned: Dean Tessman)

Tracking

({topembed+})

Trunk
x86
Windows 2000
topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

16 years ago
Keywords: topembed+

Comment 1

16 years ago
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;          
            }
(Assignee)

Comment 2

16 years ago
Isn't this where we trap an Alt keypress and activate our menu bar instead of
letting the keypress through to Windows?

Comment 3

16 years ago
Probably, should we be changing that in embedded clients, so that the keypress
gets through?

Comment 4

16 years ago
I recommend we surround hyatt's lines with 
#ifdef MOZ_XUL
#endif

Hyatt's comment says this is for XP menus.

Comment 5

16 years ago
Aaron,

Can you explain how MOZ_XUL is set ?
And where ?

Thanks

Comment 6

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

Comment 8

16 years ago
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).
(Assignee)

Comment 9

16 years ago
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?
(Assignee)

Comment 11

16 years ago
What do you mean by that?  When will the top-level window not contain the menu bar?
(Assignee)

Comment 12

16 years ago
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.
(Assignee)

Comment 14

16 years ago
Created attachment 98393 [details] [diff] [review]
proposed patch, untested in MFCEmbed

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 15

16 years ago
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+
(Assignee)

Comment 16

16 years ago
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
(Assignee)

Comment 18

16 years ago
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;|.

Comment 19

16 years ago
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.
(Assignee)

Comment 20

16 years ago
I'll do up a new patch when I get home tonight.
(Assignee)

Comment 21

16 years ago
Created attachment 99454 [details] [diff] [review]
new patch, untested in mfcembed

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 22

16 years ago
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 23

16 years ago
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+

Comment 24

16 years ago
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 )
(Assignee)

Comment 25

16 years ago
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

Comment 26

16 years ago
You can carry my r= with that change.

Updated

16 years ago
QA Contact: mdunn → bmartin
(Assignee)

Comment 27

16 years ago
Alec, can I carry your sr= forward with that minor change?

Comment 28

16 years ago
yeah, that's fine.
(Assignee)

Comment 29

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 30

16 years ago
Brent please verify this fix on branch and trunk

Comment 31

16 years ago
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 → ---

Comment 32

16 years ago
Since it is verified fixed on the trunk. We can leave it since it is not
nominated for the branch.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 33

16 years ago
--> verified on the trunk
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.