252750 - space and FAYT (find as you type) shortcuts go to help window

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[jvh]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040722 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040722 Firefox/0.9.1+

the previous versions of Firefox, hitting the "space" bar shifts a page down,
and shift-spacebar a page up.

so in the latest nightly builds, hitting spacebar goes directly to the help
window, a little inconvinient is is not ?

Reproducible: Always
Steps to Reproduce:
1. hitting spacebar
2. help window opens
3.

Actual Results:  
help window opens

Expected Results:  
shifting a page downwarts

Comment 1

[Jesse Ruderman]

Might have been caused by bug 251165.

Comment 2

[Ryan Polk (Quark)]

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

Comment 3

[Ryan Polk (Quark)]

Confirming based on dupe.

Comment 4

[Jennifer]

Happens for me too. Driving me nuts. Page down still works. Mac OS X, build
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040728
Firefox/0.9.1+

Comment 5

[Oakwine]

(In reply to comment #1)
> Might have been caused by bug 251165.

Looks likely; that bug was checked in on the 19th, and I had to go back to the
July 18 build to get rid of it.

Comment 6

[sairuh (rarely reading bugmail)]

this also occurs when I use / or ' to start find as you type, as well as when I
just start typing with the FAYT pref enabled. it makes find as you type rather
difficult to use on the Mac.

Comment 7

[Asa Dotzler [:asa]]

We need to correct this regression quickly. 

Comment 8

[Steffen Wilberg]

I notice the same behaviour if I use VK_HELP on Windows. Since I don't have a
Mac, I couldn't test it there.

So VK_HELP doesn't work at all. That's bug 124393, which I reopened.
I'd like to use Cmd+? instead, which has been suggested in bug 88739 comment 76,
but that doesn't work either. I just filed bug 253680 because of that.

So the only option for now is to #ifndef MAC_OSX the keyset.

Comment 9

[Steffen Wilberg]

Attachment: #ifndef XP_MACOSX the keyset

I meant #ifndef XP_MACOSX of course. This removes the keyboard shortcut on Mac.
The menuitem still works of course. I tested this with #ifndef XP_WIN.

I'm adding <!ENTITY openHelpMac.commandkey2  "?"> to be able to use that if bug
253680 is fixed before 1.0.

Comment 10

[Steffen Wilberg]

Comment on attachment 154756 [details] [diff] [review]
#ifndef XP_MACOSX the keyset

Mike, you r'ed bug 251165, so I'm asking you to review this one as well.

Comment 11

[Kathleen :Brade]

I don't understand the root cause of this bug.  Why would VK_HELP be triggered
by a spacebar press or "/" or "'"?  Seems like there is a bug in FAYT or the
help system.

I *really* don't like #ifdef'ing code just because it doesn't work on a
particular platform (and no one seems to know why).  Someone please explain how
the keyset is mapping all these keys to open help.

Comment 12

[Steffen Wilberg]

Can anybody using a Mac tell me if switching off Find As You Type (in
Tools->Options->Advanced->Accessibility) helps?

Brade, I'm not ifdefing the code altogether, I'm just excluding Mac, because it
doesn't work there right now. The code was intended to use
openHelpMac.commandkey (which is VK_HELP) on Mac, and openHelp.commandkey (which
is F1) on other platforms. I don't want to rewrite that altogether, that's why I
suggest to put a #ifndef XP_MACOSX around the keyset.

Comment 13

[sairuh (rarely reading bugmail)]

> Can anybody using a Mac tell me if switching off Find As You Type (in
> Tools->Options->Advanced->Accessibility) helps?

no, it doesn't. if I turn off that pref, but manually initiate FAYT by hitting
either the / or ' keys, the Help window pops up.

Comment 14

[Kathleen :Brade]

ok, here's what I think is going on:
 * VK_HELP is missing from the list in
mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp (around line 700 or so)
 * GetMatchingKeyCode returns 0 but other code seems to be looking for -1 for
"not found"
 * 0 seems to mean match any key event

note to self: VK_HELP is also missing from nsGuiEvent.h but I don't think that
relates to this bug

I think there needs to be another bug for implementing VK_HELP on
Windows/Linux/other OS.  F1 should give help if it's always help.  There
shouldn't be a need to #ifdef any of these.

Comment 15

[Steffen Wilberg]

Attachment: as suggested by brade

This makes VK_WIN not be triggered by any key on Windows, so I guess it fixes
the issue on Mac as well.
Brade, are you sure we don't need a modifier in nsMacEventHandler.cpp? I mean
VK_HELP should be triggered by Cmd+?, not by ? alone, because VK_HELP will be
used without modifiers, like the Help key on Mac, and F1 on other platforms in
the future.

Comment 16

[Steffen Wilberg]

Comment on attachment 154814 [details] [diff] [review]
as suggested by brade

Caillon, this is the patch you wanted to look at over the weekend :)

Comment 17

[Steffen Wilberg]

Comment on attachment 154814 [details] [diff] [review]
as suggested by brade

argh, wrong patch.

Comment 18

[Steffen Wilberg]

Attachment: as suggested by brade, the real one

The change in nsMacEventHandler.cpp was suggested by brade in bug 253680
comment 5.

Comment 19

[Steffen Wilberg]

Comment on attachment 154815 [details] [diff] [review]
as suggested by brade, the real one

This is the one.

Comment 20

[Kathleen :Brade]

Comment on attachment 154815 [details] [diff] [review]
as suggested by brade, the real one

First, I'd separate these patches.  They are related but are independent.  The
first patch is what is blocking this bug.  The other patch really belongs in a
different bug, specific to VK_HELP key events.

The nsMacEventHandler.cpp change needs to be a little smarter.	You need a
check for commandkey (e.g. ((eventModifiers & cmdKey) != 0) before assigning it
to VK_HELP (otherwise Mac users wouldn't be able to type a '?' character in an
edit field)).

A few more thoughts:  modifier keys (shift) are probably getting set; do they
need to be cleared for VK_HELP?  We shouldn't set VK_HELP if control-shift-/ is
pressed.  The Mac code may already take that into account.  Is someone going to
fix the windows code (and linux code) to send VK_HELP events?

r=brade on the nsXBLPrototypeHandler.cpp changes (although I'm not a module
owner there)

Comment 21

[Steffen Wilberg]

Attachment: nsXBLPrototypeHandler.cpp only

OK, here are the changes to nsXBLPrototypeHandler.cpp.

Comment 22

[Steffen Wilberg]

Comment on attachment 155019 [details] [diff] [review]
nsXBLPrototypeHandler.cpp only

Caillon, this is the same as before, only excludes the nsMacEventHandler.cpp
part.

Comment 23

[Steffen Wilberg]

I moved the Mac part of the patch to bug 253680.

Comment 24

[Cameron walters]

Bug still present in Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.7) Gecko/20040807 Firefox/0.9.1+

Space bar triggers help window.

Comment 25

[Kathleen :Brade]

Is someone going to review this?  I'd like it to land before I test the
Mac-specific patch.

Comment 26

[Ben Goodger (use ben at mozilla dot org for email)]

Steffen, please send email to solicit review in addition to using the flags. 

Comment 27

[Jo Hermans]

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

Comment 28

[Prognathous]

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

Comment 29

[Steffen Wilberg]

Comment on attachment 154756 [details] [diff] [review]
#ifndef XP_MACOSX the keyset

Requesting review on the ifndef-patch again.

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 154756 [details] [diff] [review]
#ifndef XP_MACOSX the keyset

r=me on the bandaid

Comment 31

[Steffen Wilberg]

Comment on attachment 154756 [details] [diff] [review]
#ifndef XP_MACOSX the keyset

Bandaaid until VK_HELP works.

Comment 32

[Asaf Romano (gone)]

Comment on attachment 155019 [details] [diff] [review]
nsXBLPrototypeHandler.cpp only

The patch doesn't work for me. I guess we should back to #ifndef XP_MACOSX, at
least for the PR.

Comment 33

[Kathleen :Brade]

Asaf Romano (comment 32) -- could you be more specific about what doesn't work
with the patch?  Are you still seeing the space keypress opening the help?

Comment 34

[Asaf Romano (gone)]

(In reply to comment #33)
> Asaf Romano (comment 32) -- could you be more specific about what doesn't work
> with the patch?  Are you still seeing the space keypress opening the help?

Same problem as before (space still opens the help viewer), if you want i can
check it one more time, but...

Comment 35

[Steffen Wilberg]

Brade, caillon said on IRC that he wants a better explanation for the "return
-1" change, since that affects this code:
578 nsXBLPrototypeHandler::KeyEventMatched(nsIDOMKeyEvent* aKeyEvent)
579 {
580   if (mDetail == -1 && mMisc == 0 && mKeyMask == 0)
581     return PR_TRUE; // No filters set up. It's generic.

604 nsXBLPrototypeHandler::MouseEventMatched(nsIDOMMouseEvent* aMouseEvent)
605 {
606 if (mDetail == -1 && mMisc == 0 && mKeyMask == 0)
607   return PR_TRUE; // No filters set up. It's generic.

<caillon> (I think the case for mouse event is stronger than key event in your case)
<caillon> but still
<caillon> should "unknown" equal "uninitialized"?  if so, why?
<caillon> I couldn't find a good reason to make that so in reading the source,
but I assume you have one for making the patch.
<caillon> I'd love to hear it
<caillon> because it could very well be the case.
<Steffen> caillon, brade said that GetMatchingKeyCode returns 0 but other code
seems to be looking for -1 for "not found"
<caillon> Steffen, right.  other code does check for -1.  which is the
uninitialized state.
<caillon> now, you're adding another path to get to that state, and I'd like
more than "someone told me to do this" before you do it.  :-)

I'm afraid I couldn't explain it better.

Comment 36

[Steffen Wilberg]

Comment on attachment 155019 [details] [diff] [review]
nsXBLPrototypeHandler.cpp only

Brade and Mano said this doesn't fix the problem on Mac.

Comment 37

[Kathleen :Brade]

I rebuilt my seamonkey tree with extensions/help and the patches from Steffen in
this bug as well as the xul keybinding patch for VK_HELP (thanks!) and in debug
mode (make -f client.mk build).  I don't see the problem.  I did see the problem
in my optimized build where I had only built extensions/help, content and
xpfe/bootstrap.  I contend the nsXBLPrototypeHandler.cpp only patch above is
correct (at least the first block).  More may be needed.

I haven't read caillon's comments (above) yet.

When I did see the bug, I thought it was interesting that typing a letter would
also cause the scroll (not just space)--any key combination not already mapped
to a keybinding.  That may have varied based on what the focus was.

Comment 38

[Kathleen :Brade]

Attachment: minimal fix for this bug

I believe this is the patch that should land to fix this bug.  (It's the first
half of the previous patch.)  It fixes the bug for me.

I am opposed to landing the #ifndef patch in this bug.

Comment 39

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

r=caillon

Comment 40

[Steffen Wilberg]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

I guess I need sr as well.
Brian, this is just a one-liner, and it fixes a nasty regression on Mac.

Comment 41

[Steffen Wilberg]

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

Comment 42

[Steffen Wilberg]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

One line, regression fix, no risk.

Comment 43

[Steffen Wilberg]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

Can someone check this in once it has approval, please? My cvs account is
limited to browser/ and toolkit/.

Comment 44

[Kathleen :Brade]

patch checked into trunk:
/cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v  <-- 
nsXBLPrototypeHandler.cpp
new revision: 1.82; previous revision: 1.81

leaving bug open for aviary branch

Comment 45

[Steffen Wilberg]

Thanks, Brade!

Can anybody confirm that this is fixed with Mac Firefox trunk (1.8a3) nightlies
starting 08/14? Help contents shouldn't open on pressing any key but the Help
key (the one left of the Home key).

Comment 46

[Neil Lee [:neilio]]

Seems to be fixed here. Thanks!

Comment 47

[Shoshannah Forbes]

> Can anybody confirm that this is fixed with Mac Firefox trunk (1.8a3) nightlies
> starting 08/14?

Looks good here with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8a3) Gecko/20040814 Firefox/0.9.1+ on osx 10.3.5

Comment 48

[Jo Hermans]

(In reply to comment #45)
> Thanks, Brade!
> 
> Can anybody confirm that this is fixed with Mac Firefox trunk (1.8a3) nightlies
> starting 08/14? Help contents shouldn't open on pressing any key but the Help
> key (the one left of the Home key).

Same here with build 20040815 on Mac OS X 10.2.8 (first generation iBook)

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040815
Firefox/0.9.1+

I can't test Cmd-Help though, the Help key is missing on my keyboard.

Comment 49

[jvh]

(In reply to comment #48)
> (In reply to comment #45)
> > Thanks, Brade!
> > 
> > Can anybody confirm that this is fixed with Mac Firefox trunk (1.8a3) nightlies
> > starting 08/14? Help contents shouldn't open on pressing any key but the Help
> > key (the one left of the Home key).
> 
> Same here with build 20040815 on Mac OS X 10.2.8 (first generation iBook)
> 
> Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040815
> Firefox/0.9.1+
> 
> I can't test Cmd-Help though, the Help key is missing on my keyboard.




NO, the bug remains on Mac X 10.3.5...
pressing the spacebar invokes the help page to show up, instead of a pagedown.

Comment 50

[Asaf Romano (gone)]

(In reply to comment #49)

> NO, the bug remains on Mac X 10.3.5...
> pressing the spacebar invokes the help page to show up, instead of a pagedown.

are you sure you did your test with a latest **trunk** build (not latest-0.9)?

Comment 51

[jvh]

the bug is still there: pressing the SPACE bar invokes opening a new window with
the help functions...

this is VERY annoying !!!

240816

Comment 52

[Asaf Romano (gone)]

(In reply to comment #51)
 read comment 50, thanks.

Comment 53

[jvh]

I tested with the LATEST trunc (nighly build)

rv. 1.7.2 Gecko/20040816 Firefox/0.9.1+

Comment 54

[Asaf Romano (gone)]

1.7.2 is probably from latest-0.9... not trunk

Comment 55

[Steffen Wilberg]

See comment 45. Trunk is 1.8a3.

Comment 56

[jvh]

ok, this seemes an other build than mine...

although its from the SAME date.

can you help me please, and say where I can find the build number of te browser
Macintosch version, when I do CMD-I, I get always the wrong and same info...

AND, the bug seemes FIXED !!!

Thanks.

Comment 57

[Adit Panchal]

I am using the latest trunk build:

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8a3) Gecko/20040816 Firefox/0.9.1+

and it works perfectly for me. Both spacebar and any lettered key on the keyboard do not trigger the 
error anymore.

Thanks for your good work. :)

Comment 58

[Asa Dotzler [:asa]]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

a=asa for aviary checkin.

Comment 59

[Steffen Wilberg]

Can someone check this into the aviary branch, please?

Comment 60

[Jesse Ruderman]

Checked in on aviary.  Leaving open for 1.7 branch to avoid forking Gecko
between aviary and 1.7.x.  Asa, can you approve for the 1.7 branch?

Comment 61

[Steffen Wilberg]

Thanks, Jesse!
This is fixed finally.

Comment 62

[Steffen Wilberg]

I guess I should read the last comment first. Reopening.

Comment 63

[Steffen Wilberg]

-> Browser, so that I can request approval1.7.x.

Comment 64

[Steffen Wilberg]

Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

One-line fix, no risk, tested on trunk and aviary branch.

Comment 65

[Steffen Wilberg]

Can somebody check this in for me on the MOZILLA_1_7_BRANCH, please?

Comment 66

[Kathleen :Brade]

fixed on 1.7 branch per comment 65
resolving as fixed