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

RESOLVED FIXED

Status

()

P2
major
RESOLVED FIXED
14 years ago
7 years ago

People

(Reporter: jvh, Assigned: steffen.wilberg)

Tracking

({fixed-aviary1.0, fixed1.7.5, regression})

1.7 Branch
PowerPC
Mac OS X
fixed-aviary1.0, fixed1.7.5, regression
Points:
---
Bug Flags:
blocking-aviary1.0PR +
blocking-aviary1.0mac +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

14 years ago
Might have been caused by bug 251165.
Keywords: regression

Updated

14 years ago
Flags: blocking-aviary1.0mac?

Comment 2

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

Comment 3

14 years ago
Confirming based on dupe.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

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

14 years ago
(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.
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.
Severity: normal → major
Summary: space goes to help window → space and FAYT shortcuts go to help window

Comment 7

14 years ago
We need to correct this regression quickly. 
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0mac+
Flags: blocking-aviary1.0PR?
Flags: blocking-aviary1.0PR+
Version: unspecified → 1.0 Branch

Updated

14 years ago
Assignee: aaronleventhal → firefox
Component: Keyboard Navigation → Find Toolbar / FastFind
QA Contact: jruderman

Updated

14 years ago
Assignee: firefox → steffen.wilberg
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

14 years ago
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.
Component: Find Toolbar / FastFind → Keyboard Navigation
QA Contact: jruderman
(Assignee)

Comment 9

14 years ago
Created attachment 154756 [details] [diff] [review]
#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.
(Assignee)

Comment 10

14 years ago
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.
Attachment #154756 - Flags: review?(mconnor)

Comment 11

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

Comment 12

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

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

Comment 15

14 years ago
Created attachment 154814 [details] [diff] [review]
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.
Attachment #154756 - Attachment is obsolete: true
(Assignee)

Comment 16

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

Caillon, this is the patch you wanted to look at over the weekend :)
Attachment #154814 - Flags: review?(caillon)
(Assignee)

Comment 17

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

argh, wrong patch.
Attachment #154814 - Attachment is obsolete: true
Attachment #154814 - Flags: review?(caillon)
(Assignee)

Comment 18

14 years ago
Created attachment 154815 [details] [diff] [review]
as suggested by brade, the real one

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

Comment 19

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

This is the one.
Attachment #154815 - Flags: review?(caillon)
(Assignee)

Updated

14 years ago
Attachment #154756 - Flags: review?(mconnor)

Comment 20

14 years ago
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)
Attachment #154815 - Flags: review?(caillon) → review-
(Assignee)

Comment 21

14 years ago
Created attachment 155019 [details] [diff] [review]
nsXBLPrototypeHandler.cpp only

OK, here are the changes to nsXBLPrototypeHandler.cpp.
Attachment #154815 - Attachment is obsolete: true
(Assignee)

Comment 22

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

Caillon, this is the same as before, only excludes the nsMacEventHandler.cpp
part.
Attachment #155019 - Flags: review?(caillon)
(Assignee)

Comment 23

14 years ago
I moved the Mac part of the patch to bug 253680.

Comment 24

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

14 years ago
Is someone going to review this?  I'd like it to land before I test the
Mac-specific patch.
Steffen, please send email to solicit review in addition to using the flags. 
Priority: -- → P2

Comment 27

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

Comment 28

14 years ago
*** Bug 255257 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Keywords: helpwanted
(Assignee)

Comment 29

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

Requesting review on the ifndef-patch again.
Attachment #154756 - Attachment is obsolete: false
Attachment #154756 - Flags: review?(mconnor)
Comment on attachment 154756 [details] [diff] [review]
#ifndef XP_MACOSX the keyset

r=me on the bandaid
Attachment #154756 - Flags: review?(mconnor) → review+
(Assignee)

Comment 31

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

Bandaaid until VK_HELP works.
Attachment #154756 - Flags: approval-aviary?
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.

Updated

14 years ago
Summary: space and FAYT shortcuts go to help window → space and FAYT (find as you type) shortcuts go to help window

Comment 33

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

Comment 35

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

Comment 36

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

Brade and Mano said this doesn't fix the problem on Mac.
Attachment #155019 - Attachment is obsolete: true
Attachment #155019 - Flags: review?(caillon)

Comment 37

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

14 years ago
Created attachment 155965 [details] [diff] [review]
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 on attachment 155965 [details] [diff] [review]
minimal fix for this bug

r=caillon
Attachment #155965 - Flags: review+
(Assignee)

Updated

14 years ago
Attachment #155965 - Flags: approval-aviary?
(Assignee)

Updated

14 years ago
Attachment #154756 - Attachment is obsolete: true
Attachment #154756 - Flags: approval-aviary?
(Assignee)

Comment 40

14 years ago
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.
Attachment #155965 - Flags: approval-aviary? → superreview?(bryner)
(Assignee)

Comment 41

14 years ago
*** Bug 255516 has been marked as a duplicate of this bug. ***
Attachment #155965 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 42

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

One line, regression fix, no risk.
Attachment #155965 - Flags: approval-aviary?
(Assignee)

Comment 43

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

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

Comment 45

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

Updated

14 years ago
Whiteboard: [have patch]
Target Milestone: --- → Firefox1.0beta

Comment 46

14 years ago
Seems to be fixed here. Thanks!
Keywords: helpwanted
Whiteboard: [have patch] → [have patch], needed-aviary1.0

Comment 47

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

14 years ago
(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.
(Reporter)

Comment 49

14 years ago
(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.
(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)?
(Reporter)

Comment 51

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

this is VERY annoying !!!

240816
(Reporter)

Comment 53

14 years ago
I tested with the LATEST trunc (nighly build)

rv. 1.7.2 Gecko/20040816 Firefox/0.9.1+
1.7.2 is probably from latest-0.9... not trunk
(Assignee)

Comment 55

14 years ago
See comment 45. Trunk is 1.8a3.
(Reporter)

Comment 56

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

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

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

a=asa for aviary checkin.
Attachment #155965 - Flags: approval-aviary? → approval-aviary+
(Assignee)

Comment 59

14 years ago
Can someone check this into the aviary branch, please?

Comment 60

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

Comment 61

14 years ago
Thanks, Jesse!
This is fixed finally.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Comment 62

14 years ago
I guess I should read the last comment first. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

14 years ago
Keywords: fixed-aviary1.0
Whiteboard: [have patch], needed-aviary1.0
(Assignee)

Updated

14 years ago
Whiteboard: needed-1.7
(Assignee)

Comment 63

14 years ago
-> Browser, so that I can request approval1.7.x.
Component: Keyboard Navigation → Keyboard: Navigation
Product: Firefox → Browser
Target Milestone: Firefox1.0beta → ---
Version: 1.0 Branch → 1.7 Branch
(Assignee)

Comment 64

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

One-line fix, no risk, tested on trunk and aviary branch.
Attachment #155965 - Flags: approval1.7.3?
Attachment #155965 - Flags: approval1.7.3? → approval1.7.3+

Updated

14 years ago
Whiteboard: needed-1.7 → needed-1.7 [have patch]
(Assignee)

Comment 65

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

Comment 66

14 years ago
fixed on 1.7 branch per comment 65
resolving as fixed
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago14 years ago
Keywords: fixed1.7.3
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Whiteboard: needed-1.7 [have patch]
You need to log in before you can comment on or make changes to this bug.