Closed Bug 252750 Opened 20 years ago Closed 20 years ago

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

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
1.7 Branch
Platform:
PowerPC
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jvh, Assigned: steffen.wilberg)

References

Details

(Keywords: fixed-aviary1.0, fixed1.7.5, regression)

Attachments

(1 file, 4 obsolete files)

minimal fix for this bug
851 bytes, patch
bryner
: superreview+
asa
: approval-aviary+
roc
: approval1.7.5+
Details | Diff | Splinter Review 
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
Might have been caused by bug 251165.
Keywords: regression
Flags: blocking-aviary1.0mac?
*** Bug 252991 has been marked as a duplicate of this bug. ***
Confirming based on dupe.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-aviary1.0PR?
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+
(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
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
Assignee: aaronleventhal → firefox
Component: Keyboard Navigation → Find Toolbar / FastFind
QA Contact: jruderman
Assignee: firefox → steffen.wilberg
Status: NEW → ASSIGNED
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
Attached patch #ifndef XP_MACOSX the keyset (obsolete) — — Splinter Review 
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 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)
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.
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.
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.
Attached patch as suggested by brade (obsolete) — — Splinter Review 
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
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)
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)
Attached patch as suggested by brade, the real one (obsolete) — — Splinter Review 
The change in nsMacEventHandler.cpp was suggested by brade in bug 253680
comment 5.
Comment on attachment 154815 [details] [diff] [review]
as suggested by brade, the real one

This is the one.
Attachment #154815 - Flags: review?(caillon)
Attachment #154756 - Flags: review?(mconnor)
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-
Attached patch nsXBLPrototypeHandler.cpp only (obsolete) — — Splinter Review 
OK, here are the changes to nsXBLPrototypeHandler.cpp.
Attachment #154815 - Attachment is obsolete: true
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)
I moved the Mac part of the patch to bug 253680.
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.
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. 
*** Bug 254986 has been marked as a duplicate of this bug. ***
*** Bug 255257 has been marked as a duplicate of this bug. ***
Keywords: helpwanted
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+
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.
Summary: space and FAYT shortcuts go to help window → space and FAYT (find as you type) shortcuts go to help window
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...
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 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)
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.
Attached patch minimal fix for this bug — — Splinter Review 
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+
Attachment #155965 - Flags: approval-aviary?
Attachment #154756 - Attachment is obsolete: true
Attachment #154756 - Flags: approval-aviary?
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)
*** Bug 255516 has been marked as a duplicate of this bug. ***
Attachment #155965 - Flags: superreview?(bryner) → superreview+
Comment on attachment 155965 [details] [diff] [review]
minimal fix for this bug

One line, regression fix, no risk.
Attachment #155965 - Flags: approval-aviary?
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/.
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
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).
Whiteboard: [have patch]
Target Milestone: --- → Firefox1.0beta
Seems to be fixed here. Thanks!
Keywords: helpwanted
Whiteboard: [have patch] → [have patch], needed-aviary1.0
> 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

(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.
(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)?

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

this is VERY annoying !!!

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

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
See comment 45. Trunk is 1.8a3.
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.
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 on attachment 155965 [details] [diff] [review]
minimal fix for this bug

a=asa for aviary checkin.
Attachment #155965 - Flags: approval-aviary? → approval-aviary+
Can someone check this into the aviary branch, please?
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?
Thanks, Jesse!
This is fixed finally.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I guess I should read the last comment first. Reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: fixed-aviary1.0
Whiteboard: [have patch], needed-aviary1.0
Whiteboard: needed-1.7
-> 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
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+
Whiteboard: needed-1.7 → needed-1.7 [have patch]
Can somebody check this in for me on the MOZILLA_1_7_BRANCH, please?
fixed on 1.7 branch per comment 65
resolving as fixed
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Keywords: fixed1.7.3
Resolution: --- → FIXED
Whiteboard: needed-1.7 [have patch]
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: