Closed
Bug 429160
Opened 17 years ago
Closed 17 years ago
[Mac]Regression: Command-Option-F does not select search box
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9
People
(Reporter: louise6380, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaas
:
review+
roc
:
superreview+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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
Comment 1•17 years ago
|
||
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
Assignee | ||
Comment 2•17 years ago
|
||
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?
Comment 3•17 years ago
|
||
Masayuki, so this issue is not OS X only? Then please set the OS field to all. Thanks.
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee | ||
Updated•17 years ago
|
Component: Keyboard Navigation → Keyboard: Navigation
Flags: blocking-firefox3?
Product: Firefox → Core
QA Contact: keyboard.navigation → keyboard.navigation
Target Milestone: --- → mozilla1.9
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → masayuki
Comment 7•17 years ago
|
||
(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?
Assignee | ||
Comment 8•17 years ago
|
||
(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
Comment 9•17 years ago
|
||
(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.
Assignee | ||
Comment 10•17 years ago
|
||
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)
Assignee | ||
Comment 11•17 years ago
|
||
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+
Assignee | ||
Comment 12•17 years ago
|
||
Comment on attachment 315912 [details] [diff] [review]
Patch v1.0 for XP (checked-in)
easy mistake. no risk.
Attachment #315912 -
Flags: approval1.9?
Comment 13•17 years ago
|
||
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?
Assignee | ||
Comment 14•17 years ago
|
||
(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...
Assignee | ||
Comment 15•17 years ago
|
||
(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.
Assignee | ||
Comment 16•17 years ago
|
||
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 17•17 years ago
|
||
Comment on attachment 315912 [details] [diff] [review]
Patch v1.0 for XP (checked-in)
a1.9=beltzner
Attachment #315912 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•17 years ago
|
Attachment #315912 -
Attachment description: Patch v1.0 for XP → Patch v1.0 for XP (checked-in)
Assignee | ||
Comment 18•17 years ago
|
||
I posted the new patch to bug 306585. Because all parts are needed by the bug. But the patch also fixes this bug.
Comment 19•17 years ago
|
||
(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.
Assignee | ||
Comment 20•17 years ago
|
||
ok, this is simplest patch.
Attachment #315913 -
Attachment is obsolete: true
Attachment #316404 -
Flags: review?(mozbugz)
Comment 21•17 years ago
|
||
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+
Assignee | ||
Comment 22•17 years ago
|
||
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+
Updated•17 years ago
|
Attachment #316439 -
Flags: approval1.9+
Updated•17 years ago
|
Whiteboard: [needs landing]
Assignee | ||
Comment 23•17 years ago
|
||
checked-in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Comment 24•17 years ago
|
||
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
Assignee | ||
Comment 25•17 years ago
|
||
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.
Updated•6 years ago
|
Component: Keyboard: Navigation → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•