Closed Bug 429160 Opened 16 years ago Closed 16 years ago

[Mac]Regression: Command-Option-F does not select search box

Categories

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

All
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9

People

(Reporter: louise6380, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041420 Minefield/3.0pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041420 Minefield/3.0pre

Command+Option+F stopped selecting search box between build 20080414_2047 and 20080414_2130, regression of bug 348472 possibly caused by Bug 359638.

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1208231220&maxdate=1208233799


Reproducible: Always

Steps to Reproduce:
1.hit Command+Option+F

Actual Results:  
nothing happens

Expected Results:  
search box should be selected like Command+K
Blocks: 359638
Version: unspecified → Trunk
Confirmed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008041504 Minefield/3.0pre ID:2008041504

This is a major keyboard shortcut on OS X. I hope that it can be fixed for Firefox 3. Asking for blocking.
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
Keywords: regression
Hardware: Macintosh → All
I know the reason. But I need more discussion with karlt.

Karl:

I didn't send the alternative charCodes at AltGr(Win/Linux) and Alt(Mac) being pressed. However, that makes this regression. I think we need to send the alternative keys at that time on *all* platforms. How do you think?
Masayuki, so this issue is not OS X only? Then please set the OS field to all. Thanks.
I'm not sure this is XP bugs. Because I don't know win32 build has Ctrl+Alt+foo combination command, and Linux has Ctrl+AltGr+foo key combination. However, this is a design problem of the patch of bug 359638.
Component: Keyboard Navigation → Keyboard: Navigation
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: keyboard.navigation → keyboard.navigation
Target Milestone: --- → mozilla1.9
Assignee: nobody → masayuki
Reasking blocking because of product change.
Flags: blocking1.9?
Yeah, we should fix this.  +'ing.
Flags: blocking1.9? → blocking1.9+
(In reply to comment #2)
> I didn't send the alternative charCodes at AltGr(Win/Linux) and Alt(Mac) being
> pressed. However, that makes this regression. I think we need to send the
> alternative keys at that time on *all* platforms. How do you think?

I think the right thing to do depends on the platform.

Mac:

I'm not so familiar with Mac, but IIUC the Option key behaves similarly to
AltGr when Command is not pressed, but it is also used with Command as a
modifier for shortcuts.

This results in similar problems to using the Shift key as a shortcut
modifier.  It can be unclear whether the Option key is used to select the
character (consumed modifier e.g. ⌘[ on a Danish keyboard) or as a modifier
for the shortcut (⌘⌥8).  The problem is discussed here:

http://sigpipe.macromates.com/2005/09/24/deciphering-an-nsevent/

There are some heuristics suggested there, but I guess they are not always
going to be reliable.

Was there ever a stage when both possibilities worked?  (See bug 429211.)  If
so, I'd consider doing whatever is necessary to restore the previous behavior.

If these didn't ever both work, then either we choose which to support, or
(ideally I guess) we need to send a list of possible character/modifier
combinations to try.  Sending a list of characters with just one modifier set
won't match both.  (Perhaps some sort of
possibly-ignore-alt-when-command-is-pressed logic can be worked, but the
multiple modifier combinations feels more generic as that could handle Shift
also.)

Linux:

On X11 AltGr is called ISO_Level3_Shift and corresponds to its own separate
modifier (mod5).  On that platform it is possible to press any combination of
Alt/Ctrl/Meta together with either of the characters produced with or without
AltGr.  If the user has pressed AltGr then they mean the AltGr character, so
the other character should not be sent.

Windows:

I hope an event with only the AltGr modifier doesn't look like any shortcut
event.  I'm assuming here that we don't use Ctrl-Alt together in any
shortcuts.

http://blogs.msdn.com/oldnewthing/archive/2004/03/29/101121.aspx

I don't know whether it is possible to differentiate Ctrl-AltGr-char from
AltGr-char.  If not then I guess we can only assume AltGr-char, and it is not available for shortcuts.  But perhaps they can be differentiated by recording the separate Ctrl-down event?  If so then is there any reason why a non-AltGr character should be sent when AltGr is down?
(In reply to comment #7)
> (In reply to comment #2)
> > I didn't send the alternative charCodes at AltGr(Win/Linux) and Alt(Mac) being
> > pressed. However, that makes this regression. I think we need to send the
> > alternative keys at that time on *all* platforms. How do you think?
> 
> I think the right thing to do depends on the platform.

O.K. I agree by following comments.

> Mac:
> 
> I'm not so familiar with Mac, but IIUC the Option key behaves similarly to
> AltGr when Command is not pressed, but it is also used with Command as a
> modifier for shortcuts.

yes.

> Linux:
> 
> On X11 AltGr is called ISO_Level3_Shift and corresponds to its own separate
> modifier (mod5).  On that platform it is possible to press any combination of
> Alt/Ctrl/Meta together with either of the characters produced with or without
> AltGr.  If the user has pressed AltGr then they mean the AltGr character, so
> the other character should not be sent.

O.K. Let's don't change the current design.

> Windows:
> 
> I hope an event with only the AltGr modifier doesn't look like any shortcut
> event.  I'm assuming here that we don't use Ctrl-Alt together in any
> shortcuts.
> 
> http://blogs.msdn.com/oldnewthing/archive/2004/03/29/101121.aspx
> 
> I don't know whether it is possible to differentiate Ctrl-AltGr-char from
> AltGr-char.  If not then I guess we can only assume AltGr-char, and it is not
> available for shortcuts.  But perhaps they can be differentiated by recording
> the separate Ctrl-down event?  If so then is there any reason why a non-AltGr
> character should be sent when AltGr is down?

I'm not sure... But there is a problem. AltGr means Ctrl+Alt. So, CtrlorAlt+AltGr means Ctr+Alt. Therefore, if AltGr is pressed we cannot fire the accelkey is only Alt or Ctrl commands...
Status: NEW → ASSIGNED
(In reply to comment #8)
> (In reply to comment #7)
> > Windows:
>
> I'm not sure... But there is a problem. AltGr means Ctrl+Alt. So,
> CtrlorAlt+AltGr means Ctr+Alt.

That's what I was getting at, yes.  But I suspect there must be a difference between AltGr and Ctrl+Alt because AltGr+Delete doesn't bring up the task manager.  Whether that distinction is available to applications is the question.  
Looks like it is tricky to detect:

http://cvsweb.xfree86.org/cvsweb/xc/programs/Xserver/hw/xwin/winkeybd.c?annotate=1.14#559

> Therefore, if AltGr is pressed we cannot fire
> the accelkey is only Alt or Ctrl commands... 

Yes, we may need to live with that if there is no way to detect the difference.
Even if there is a way, then we need not fix that right now.
I have a mistake. When the shift key is not pressed, only the first charCode is checked, sorry.
Attachment #315912 - Flags: superreview?(roc)
Attachment #315912 - Flags: review?(roc)
Attached patch Patch v1.0 for cocoa (obsolete) — Splinter Review
This is for cocoa part.

On Cocoa, even if option(alt) key is pressed, they send alternative chars too.
When option key is pressed, sending following list.
1. unshiftedAltChar and shiftedAltChar
2. unshiftedChar and shiftedChar

If option is not pressed, we are not sending the (1).
Attachment #315913 - Flags: review?(mozbugz)
Attachment #315913 - Flags: review?(joshmoz)
Attachment #315912 - Flags: superreview?(roc)
Attachment #315912 - Flags: superreview+
Attachment #315912 - Flags: review?(roc)
Attachment #315912 - Flags: review+
Comment on attachment 315912 [details] [diff] [review]
Patch v1.0 for XP (checked-in)

easy mistake. no risk.
Attachment #315912 - Flags: approval1.9?
Comment on attachment 315913 [details] [diff] [review]
Patch v1.0 for cocoa

-    // If Ctrl or Command is pressed, we should set shiftCharCode and
+    // If Alt or Ctrl or Command is pressed, we should set shiftCharCode and
     // unshiftCharCode for accessKeys and accelKeys.
-    if ((outGeckoEvent->isControl || outGeckoEvent->isMeta) &&
-        !outGeckoEvent->isAlt) {
+    if (outGeckoEvent->isControl || outGeckoEvent->isMeta ||
+        outGeckoEvent->isAlt) {

This is enough to fix this bug, right?

+        if (unshiftedAltChar || shiftedAltChar) {
+          nsAlternativeCharCode altCharCodes(unshiftedAltChar, shiftedAltChar);
+          outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes);
+        }

I can't see how these are going to be helpful though.
This looks like it is attempting to fix bug 306585 and bug 429211
(⌘[ when [ can only be produced when the Option key to be down).
But outGeckoEvent->isAlt is set, so doesn't that mean that this event won't
pass the modifier-match test in the handler anyway?
(In reply to comment #13)
> (From update of attachment 315913 [details] [diff] [review])
> -    // If Ctrl or Command is pressed, we should set shiftCharCode and
> +    // If Alt or Ctrl or Command is pressed, we should set shiftCharCode and
>      // unshiftCharCode for accessKeys and accelKeys.
> -    if ((outGeckoEvent->isControl || outGeckoEvent->isMeta) &&
> -        !outGeckoEvent->isAlt) {
> +    if (outGeckoEvent->isControl || outGeckoEvent->isMeta ||
> +        outGeckoEvent->isAlt) {
> 
> This is enough to fix this bug, right?

right.

> +        if (unshiftedAltChar || shiftedAltChar) {
> +          nsAlternativeCharCode altCharCodes(unshiftedAltChar,
> shiftedAltChar);
> +          outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes);
> +        }
> 
> I can't see how these are going to be helpful though.
> This looks like it is attempting to fix bug 306585 and bug 429211
> (⌘[ when [ can only be produced when the Option key to be down).
> But outGeckoEvent->isAlt is set, so doesn't that mean that this event won't
> pass the modifier-match test in the handler anyway?

Oh, right...
(In reply to comment #14)
> (In reply to comment #13)
> > +        if (unshiftedAltChar || shiftedAltChar) {
> > +          nsAlternativeCharCode altCharCodes(unshiftedAltChar,
> > shiftedAltChar);
> > +          outGeckoEvent->alternativeCharCodes.AppendElement(altCharCodes);
> > +        }
> > 
> > I can't see how these are going to be helpful though.
> > This looks like it is attempting to fix bug 306585 and bug 429211
> > (⌘[ when [ can only be produced when the Option key to be down).
> > But outGeckoEvent->isAlt is set, so doesn't that mean that this event won't
> > pass the modifier-match test in the handler anyway?
> 
> Oh, right...
> 

o.k. karl, you should continue this. bug 306585 and bug 429211 need more work on XP code (nsContentUtils and XBL). On Mac, when Ctrl or Cmd is pressed, we should be able to ignore the Alt flag after all command handling was failed in current rules.
No longer blocks: 429211
Comment on attachment 315913 [details] [diff] [review]
Patch v1.0 for cocoa

I'll post a new patch.
Attachment #315913 - Flags: review?(mozbugz)
Attachment #315913 - Flags: review?(joshmoz)
Attachment #315913 - Flags: review-
Comment on attachment 315912 [details] [diff] [review]
Patch v1.0 for XP (checked-in)

a1.9=beltzner
Attachment #315912 - Flags: approval1.9? → approval1.9+
Attachment #315912 - Attachment description: Patch v1.0 for XP → Patch v1.0 for XP (checked-in)
I posted the new patch to bug 306585. Because all parts are needed by the bug. But the patch also fixes this bug.
(In reply to comment #18)
It'll take me some time to study that patch/bug.
Can we do the part that fixes this bug separately, please, so that we can get this bug fixed sooner, as the part for this bug seems pretty simple.
Attached patch Patch v1.1 for cocoa (obsolete) — Splinter Review
ok, this is simplest patch.
Attachment #315913 - Attachment is obsolete: true
Attachment #316404 - Flags: review?(mozbugz)
Comment on attachment 316404 [details] [diff] [review]
Patch v1.1 for cocoa

+    // If Ctrl or Command Alt is pressed, we should set shiftCharCode and

r=me with "Command *or* Alt"
Attachment #316404 - Flags: review?(mozbugz) → review+
Attached patch Patch rv1.1.1Splinter Review
Attachment #316404 - Attachment is obsolete: true
Attachment #316439 - Flags: superreview?(roc)
Attachment #316439 - Flags: review?(joshmoz)
Attachment #316439 - Flags: review?(joshmoz) → review+
Attachment #316439 - Flags: superreview?(roc) → superreview+
Attachment #316439 - Flags: approval1.9+
Whiteboard: [needs landing]
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042004 Minefield/3.0pre ID:2008042004
Status: RESOLVED → VERIFIED
This bug now comes back. Sorry, that is my regression.
But the cause is really widget code. So, that is not related to bug 359638. So, *don't reopen this bug*. See bug 430419 for the new regression.
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.