401086 - Ctrl + + doesn't work for text zoom

Status: RESOLVED FIXED

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

Description

[Hideo Oshima]

Ctrl + +(full key) deesn't work for text zoom.
Ctrl + -, Ctrl + 0 and Ctrl + +(numpad) are OK.

I use Japanese 106 keyboard.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102423 Minefield/3.0a9pre

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Related to bug 198233?

Comment 2

[Hideo Oshima]

Attachment: Japanese 106 keyboard layout. This is from GNOME 2.20.

+ in Japanese 106 keyboard is shift + ; key.
I don't know this is related to the problem.

Comment 3

[Hideo Oshima]

Both ctrl + ; and ctrl + shift + ; don't work.
Is this regression of bug 295228?

Comment 4

[Matthew Cline]

It's bound to Ctrl-=, not Ctrl-+.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

'=' is assigned Shift + '-' in JIS layout. So, we cannot press '+' and '=' without Shift key.

Comment 7

[Karl Tomlinson (:karlt)]

Zoom In is bound to Ctrl-+ with fullZoomEnlargeCmd.commandkey, as indicated in the View -> Zoom menu.  The problem seems to be that this combination is not detected when Shift is down.

US keyboards have the same problem with Ctrl-+ (but they are fortunate to have Ctrl-= as a workaround through fullZoomEnlargeCmd.commandkey2).

Comment 8

[Karl Tomlinson (:karlt)]

(Ctrl-+ works fine on US keyboard with FF2)

Comment 9

[Karl Tomlinson (:karlt)]

Same regression on MS Windows XP

Comment 10

[Johnny Stenback (:jst)]

Karl, any chance you could look into this regression? Seems like you've already done some investigation here.

Comment 11

[Johnny Stenback (:jst)]

This really seems like a frontend regression, over to the Firefox component.

Comment 12

[Mike Connor [:mconnor]]

Couple of thoughts:

* If this is a regression, please find the regression range.
* AFAICT, nothing changed in the front end
* I don't think this specifically blocks, if the ja builds work properly.  We bind + in order for the menus to work properly, we don't explicitly bind Accel + Shift + + anywhere, so if core code changed so that Accel + Shift + foo is not the same as Accel + foo, that's a core regression, or a specific platform change that really needs announcing somewhere...

The front end hackaround for this, I suspect, is to add another key for accel+shift+fullZoomEnlargeCmd.commandkey in browser-sets.inc but I'm not sure I think that's correct.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think this is not front end bug. See bug 359638.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

/me tosses back over to core::keyboard navigation as per comment 13

Comment 15

[(not currently active) Ted Mielczarek]

minusing per comment 12, unless someone has other info.

Comment 16

[Karl Tomlinson (:karlt)]

Looks like a regression from the removal of these lines in bug 295228:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp&rev=1.96&mark=881,882#876

Similar regressions were picked up in bug 306686 and bug 307423 but their fixes only fixed the individual shortcuts.

Comment 17

[Karl Tomlinson (:karlt)]

(In reply to comment #12)
> * I don't think this specifically blocks, if the ja builds work properly.

ja localizations uses the same zoom character codes as US so I don't expect ja builds to work properly:

http://bonsai-l10n.mozilla.org/cvsblame.cgi?file=/l10n/ja/browser/chrome/browser/browser.dtd&rev=1.12&root=/l10n&mark=286,287,291,292#284

I don't know how to read Japanese.  Is this a translation or a curse?

<!-- + is above this key on many keyboards --><!-- それは US キーボードだけの話ですが… (^^; -->

> We bind + in order for the menus to work properly, we don't explicitly bind
> Accel + Shift + + anywhere, so if core code changed so that
> Accel + Shift + foo is not the same as Accel + foo, that's a core regression,
> or a specific platform change that really needs announcing somewhere...

That's the regression.

> The front end hackaround for this, I suspect, is to add another key for
> accel+shift+fullZoomEnlargeCmd.commandkey in browser-sets.inc but I'm not sure
> I think that's correct.

There must be a better way than trying to guess which short cut char codes might require Shift to access them (and which short cut keys the localization will want to distinguish between with Shift).

(In reply to comment #15)
> minusing per comment 12, unless someone has other info.

Requesting restoration of the blocking flag, as comment 12 is not a reason for minusing.

Comment 18

[Hideo Oshima]

(In reply to comment #17)
> http://bonsai-l10n.mozilla.org/cvsblame.cgi?file=/l10n/ja/browser/chrome/browser/browser.dtd&rev=1.12&root=/l10n&mark=286,287,291,292#284
> 
> I don't know how to read Japanese.  Is this a translation or a curse?
> 
> <!-- + is above this key on many keyboards --><!-- それは US
> キーボードだけの話ですが… (^^; -->

I tried to translate the sentence to English,
"It is a story only for US keyboard."

Comment 19

[Johnny Stenback (:jst)]

Karl, can you work on this? If not, let me know...

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: be less picky about the shift modifier

This reverts to the behavior of ignoring the Shift state of the event when the handler is a key handler with a character code (not a key code), and shift isn't a required modifier, but still maintains a Shift-state distinction for alphabetic letters with upper and lower cases.

It more or less backs out

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=/mozilla/content/xbl/src&command=DIFF_FRAMESET&file=nsXBLPrototypeHandler.cpp&rev2=1.97&rev1=1.96

but makes a change to nsXBLPrototypeHandler::KeyEventMatched so that no new
assumptions are made about the case of the character in the event (essentially
fixing bug 295228 in a different way).

(I doubt that non-BMP character codes work but I didn't want to add any
further PRUnichar problems.)

Who is the best reviewer for this?
jst please pass on to someone else if you like.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 310647 [details] [diff] [review]
be less picky about the shift modifier

Neil reviewed the fix for bug 295228 and should review here. I'll sr once reviewed...

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Karlt:

Why did you post the patch? The patch of bug 359638 fixes this bug (it's under your review).

Comment 23

[[e-mail]]

Comment on attachment 310647 [details] [diff] [review]
be less picky about the shift modifier

First, I don't think you should bother with UTF-32 yet.

Secondly, what I think you're trying to do is to make it so that all keys that don't have upper/lower case distinction ignore the shift key but I don't think you're doing it correctly, as your code looks as if it will break the existing shift key ignoring code available to XBL, which looks like this:

<handler event="keypress" key="A"> <!-- unshifted a or A -->
<handler event="keypress" key="A" modifiers="shift"> <!-- shifted a or A -->
<handler event="keypress" key="A" modifires="shift any"> <!-- a or A -->

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> (From update of attachment 310647 [details] [diff] [review])
> Secondly, what I think you're trying to do is to make it so that all keys that
> don't have upper/lower case distinction ignore the shift key but I don't think
> you're doing it correctly, as your code looks as if it will break the existing
> shift key ignoring code available to XBL, which looks like this:
> 
> <handler event="keypress" key="A"> <!-- unshifted a or A -->
> <handler event="keypress" key="A" modifiers="shift"> <!-- shifted a or A -->
> <handler event="keypress" key="A" modifires="shift any"> <!-- a or A -->

Wow, that's interesting. My patch of bug 359638 also breaks that feature.
However, this feature is not good idea for many keyboard layout supporting. So, I think that we should change the behavior now. (Even if we prefer the shift key state, it should be only when the key is an alphabet key.)

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And I'm not sure what means the "any" keyword. Because it looks like the handler ignores the all modifier keys. I.e., Ctrl/Alt/Meta keys are also ignored... And I cannot find that the feature is being used.
http://mxr.mozilla.org/mozilla/search?string=ENTITY.*modifiers.*any&regexp=on&find=\.dtd%24&findi=&filter=&tree=mozilla

Comment 26

[Karl Tomlinson (:karlt)]

(In reply to comment #24)
> (In reply to comment #23)
> > <handler event="keypress" key="A"> <!-- unshifted a or A -->
> > <handler event="keypress" key="A" modifiers="shift"> <!-- shifted a or A -->
> > <handler event="keypress" key="A" modifires="shift any"> <!-- a or A -->
> 
> Wow, that's interesting. My patch of bug 359638 also breaks that feature.
> However, this feature is not good idea for many keyboard layout
> supporting. So, I think that we should change the behavior now. (Even if we
> prefer the shift key state, it should be only when the key is an alphabet
> key.)

Yes, Shift states and non-alphabet keys don't mix.

I think if any handler demands a particular shift state for a character
without upper and lower case forms, that is asking for trouble.

But currently the key attribute is often in a localizable file while the
modifiers attribute is in a different non-localizable file, so that could
result in some problems.

(In reply to comment #25)
> And I'm not sure what means the "any" keyword. Because it looks like the
> handler ignores the all modifier keys. I.e., Ctrl/Alt/Meta keys are also
> ignored...

Only the modifiers preceding the "any" are ignored.  Required modifiers can be
listed after the "any".

http://developer.mozilla.org/en/docs/XUL:key#a-modifiers

Comment 27

[Karl Tomlinson (:karlt)]

Attachment: be less picky about the shift modifier (and handle "shift any")

(In reply to comment #23)

Thank you, Neil for the prompt review.

> (From update of attachment 310647 [details] [diff] [review])
> what I think you're trying to do is to make it so that all keys that
> don't have upper/lower case distinction ignore the shift key but I don't think
> you're doing it correctly, as your code looks as if it will break the existing
> shift key ignoring code available to XBL, which looks like this:

> <handler event="keypress" key="A" modifires="shift any"> <!-- a or A -->

Yes, you are right.  The last patch was restoring the FF2 behavior where only
"A" (not "a") would match.  This patch now handles "A" and "a".

> I don't think you should bother with UTF-32 yet.

There do seem to be some people who would like non-BMP characters to work, but
I guess you know about that (bug 297943).

I think this patch now has the minimum reasonable handling of the UTF-32/16
conversions.

Comment 28

[Karl Tomlinson (:karlt)]

(In reply to comment #22)
> Why did you post the patch? The patch of bug 359638 fixes this bug (it's under
> your review).

In bug 359638 comment 63 I was trying to say (but perhaps not very clearly)
that I think that bug 359638 and this bug are different bugs and therefore
should be fixed in different ways.

In bug 359638 you are suggesting checking different characters on the same key
from different Shift states to solve the problem.

I'm saying that the best way to fix this bug is to use the character from the
event (with some case conversion) but ignore the state of the Shift key (as
done in FF2).

Does this interfere with your plans to fix bug 359638 or bug 414130?
I thought it would fit in fine with what you are doing.

Comment 29

[Karl Tomlinson (:karlt)]

(In reply to comment #24)
> (In reply to comment #23)
> > <handler event="keypress" key="A" modifires="shift any"> <!-- a or A -->
> 
> Wow, that's interesting. My patch of bug 359638 also breaks that feature.
> However, this feature is not good idea for many keyboard layout
> supporting. So, I think that we should change the behavior now. (Even if we
> prefer the shift key state, it should be only when the key is an alphabet
> key.)

I think I better understand why you made this comment now.
With

<handler event="keypress" key="4" modifiers="shift any">

I assume the intended meaning is that the character "4" should be accepted
with any Shift state (as was the default before 295228 and the default
suggested here as a fix for this bug) so that both US and French keyboard
users can access the "4".

I don't think the intended meaning is that Shift-$ should be accepted from a
US keyboard or that "'" should be accepted from a French keyboard.

I now see your point that

<handler event="keypress" key="4" modifiers="shift">

may be intended to mean the key with a "4" together with the Shift key, but
with the patch here this would only be accepted from a French keyboard, while
your patch in bug 359638 would accept this on either US and French keyboards.

Comment 30

[[e-mail]]

It would help me to review this if you could clarify what your intention is, as I'm a little confused:
"a" matches "a", "A" (with caps lock)
"a", shift matches shifted "a", shifted "A"
"a", any matches all of the above
"+" matches "+", but with or without shift?
"+", shift matches what?
"+", any matches what?

Comment 31

[Karl Tomlinson (:karlt)]

(In reply to comment #30)
My intention is to retain the current behavior for alphabetic characters from
bicameral scripts.

> "a" matches "a", "A" (with caps lock)
> "a", shift matches shifted "a", shifted "A"

Yes.

"a", "shift any" matches all of the above

But for punctuation and numerals, it is not possible for the author to know
which Shift state is required to produce the character or to know which
characters might be on the same key, so my intention is to restore the
previous behavior where the Shift state is often ignored.  (The Shift state
isn't really completely ignored because it affects which character is
produced.)

> "+" matches "+", but with or without shift?

Yes.

"+", "shift any" also matches the same as above.
(It is just stated explicitly.)

> "+", shift matches what?

The previous and current behavior was/is to only match on keyboards where the
"+" character can be produced with Shift.  This is not a useful combination
but I'm not proposing any change to the behavior: "+", shift matches "+" only
with Shift.

For characters from unicase scripts my intention is to restore the previous
behavior which is the same as the behavior described above for numerals and
punctuation.  AFAICT keys for these characters on keyboards with these scripts
produce different characters when Shift is down in the same way as keys for
numerals and punctuation do on keyboards with bicameral scripts.

Comment 32

[Karl Tomlinson (:karlt)]

(In reply to comment #29)
> <handler event="keypress" key="4" modifiers="shift">
> 
> may be intended to mean the key with a "4" together with the Shift key

However, I don't think this would be a useful behavior to support, as there is no corresponding combination to mean "the key with a 4 character on it somewhere _without_ Shift".  Even if there was such a pair of combinations, I think such a feature would only cause problems due to different characters being on the same key in different layouts.

Comment 33

[[e-mail]]

(In reply to comment #31)
>The previous and current behavior was/is to only match on keyboards where the
>"+" character can be produced with Shift.  This is not a useful combination
>but I'm not proposing any change to the behavior: "+", shift matches "+" only
>with Shift.
If you insist, but I would have thought that the code would have been simpler if Shift was ignored for characters other than bicameral letters.

Comment 34

[Karl Tomlinson (:karlt)]

(In reply to comment #33)
> (In reply to comment #31)
> >The previous and current behavior was/is to only match on keyboards where the
> >"+" character can be produced with Shift.  This is not a useful combination
> >but I'm not proposing any change to the behavior: "+", shift matches "+" only
> >with Shift.
> If you insist, but I would have thought that the code would have been simpler
> if Shift was ignored for characters other than bicameral letters.

I won't insist.  I was just trying to minimize changes in behavior at this stage.

Ignoring an explicit shift might actually be helpful in the situation where the modifiers attribute is in a non-localizable file but there are no bicameral letters on the keyboard.  But then the character from the shifted level can be specified.  Ignoring shift might allow a character from an unshifted level on a different key to be used if preferred.  Not sure if there are any use cases or not.  Not sure if there are any other possible issues.

I was just being cautious as there seem to be plenty of hidden surprises in key navigation.

Comment 35

[[e-mail]]

Sorry for the delay, but I wanted to be clear in my mind.

If the shortcut denies shift, then lowercase the character to match, otherwise lowercase it.

If the shortcut is shift-sensitive and the shift key is not pressed, then lowercase the character pressed, otherwise uppercase it.

Except as above, never test the keypress shift key state. The first alternate way of achieving this I toyed with is to use the shift flag to indicate shift-sensitive and set the shift mask as if the key was insensitive. It then occurred to me that it was much easier simply to ignore the shift key in ModifiersMatchMask in the case of a key press (but still check it in the case of a mouse click).

Comment 36

[[e-mail]]

Comment on attachment 310911 [details] [diff] [review]
be less picky about the shift modifier (and handle "shift any")

>-  PRBool ModifiersMatchMask(nsIDOMUIEvent* aEvent);
>+  PRBool ModifiersMatchMask(nsIDOMUIEvent* aEvent, PRInt32 aModifiersMask);
[Had we wanted this change it ought to have been made static as well]

>-#include "nsReadableUtils.h"
> #include "nsGkAtoms.h"
> #include "nsGUIEvent.h"
> #include "nsIXPConnect.h"
> #include "nsIDOMScriptObjectFactory.h"
> #include "nsDOMCID.h"
> #include "nsUnicharUtils.h"
> #include "nsReadableUtils.h"
I did wonder about the - until I saw the line again lower down!

>+  PRInt32 modKeyMask = mKeyMask;
... so this and its related changes go away ...

>+    // non-BMP characters are currently not handled.
>+    if (!IS_IN_BMP(code))
>+      return PR_FALSE;
I'm not convinced over this, since I don't think we get BMP codes anyway.

>-  if (mKeyMask & cShiftMask) {
>+  if (aModifiersMask & cShiftMask) {
>     key ? key->GetShiftKey(&keyPresent) : mouse->GetShiftKey(&keyPresent);
... and here, you only need do the check if it's a mouse event.

Comment 37

[Karl Tomlinson (:karlt)]

Attachment: be less picky about the shift modifier even when shift is explicitly required

(In reply to comment #35)
> If the shortcut denies shift, then lowercase the character to match, otherwise
> lowercase it.

I've looked at this several times.  The only thing that I've come up with that
makes any sense to me is that you are asking me to take advantage of the way
things are implemented such that mKeyMask & cShift is true when (mKeyMask &
cShiftMask) is false.  Feels like a little slight of hand to me, but I'm
assuming you are asking for this because...

> If the shortcut is shift-sensitive and the shift key is not pressed, then
> lowercase the character pressed, otherwise uppercase it.

...I can't think of any other reason why you are asking for a change from
lower to upper case for the "shift any" situation.

(In reply to comment #36)
> >+    // non-BMP characters are currently not handled.
> >+    if (!IS_IN_BMP(code))
> >+      return PR_FALSE;
> I'm not convinced over this, since I don't think we get BMP codes anyway.

GTK at least should be able to provide non-BMP characters:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsGtkKeyUtils.cpp&rev=1.13&mark=260,261#256
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/keysym2ucs.c&rev=1.6&mark=880-882#869

> 
> >-  if (mKeyMask & cShiftMask) {
> >+  if (aModifiersMask & cShiftMask) {
> >     key ? key->GetShiftKey(&keyPresent) : mouse->GetShiftKey(&keyPresent);
> ... and here, you only need do the check if it's a mouse event.

or a key event with a key code instead of a character code.

Comment 38

[Karl Tomlinson (:karlt)]

Attachment: be less picky about the shift modifier even when shift is explicitly required (v4)

(mMisc correction)

Comment 39

[[e-mail]]

(In reply to comment #37)
>Feels like a little slight of hand to me
Maybe when I originally wrote the mask code I should have flipped the sense of the bit so you could have used (mKeyMask & (cShift | cShiftMask)).

An alternative you may prefer (along with a corresponding change) could be
if ((cShift | cShiftMask) & ~mKeyMask) // is shift required?
  ToUpperCase(key);
else
  ToLowerCase(key);

>or a key event with a key code instead of a character code.
Thanks for spotting that!

Comment 40

[[e-mail]]

Weird; unfortunately I was trying a non-debug build; a key for Shift+P worked, but another key for Ctrl+Shift+R failed.

Comment 41

[[e-mail]]

Comment on attachment 311977 [details] [diff] [review]
be less picky about the shift modifier even when shift is explicitly required (v4)

OK, so this seems to work fine in my debug build. I guess my other build must have been at fault.

Comment 42

[Karl Tomlinson (:karlt)]

Attachment: be less picky about the shift modifier even when shift is explicitly required (v5)

(In reply to comment #39)
> (In reply to comment #37)
> ... so you could have used (mKeyMask & (cShift | cShiftMask)).
> 
> An alternative you may prefer (along with a corresponding change) could be
> if ((cShift | cShiftMask) & ~mKeyMask) // is shift required?

Thanks.  I find this a little hard to understand, but I think it means
"is shift not required".

I can't really see much advantage over:

 if ((mKeyMask & cShiftMask) && (mKeyMask & cShift)) // is shift required?

How about the following (with corresponding change in KeyEventMatched)?

 if ((mKeyMask & (cShift | cShiftMask)) == (cShift | cShiftMask)) // is shift required?

It should only require one bitwise and one comparison operation at run-time
and it looks easier to understand to me.

Comment 43

[Karl Tomlinson (:karlt)]

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206671627&maxdate=1206671700&who=karlt%2B%25karlt.net

Comment 44

[Karl Tomlinson (:karlt)]

Backed out due to mochitest failures on Linux.

Comment 45

[Karl Tomlinson (:karlt)]

Checked in again:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1206681362&maxdate=1206681542&who=karlt%2B%25karlt.net

(It seems the mochitest failures were unrelated.)

Comment 46

[pal-moz]

maybe regression from this checkin,
see http://forums.mozillazine.org/viewtopic.php?p=3315828#3315828

Comment 47

[[e-mail]]

(In reply to comment #46)
> maybe regression from this checkin,
> see http://forums.mozillazine.org/viewtopic.php?p=3315828#3315828

Oops. I thought I'd tried all of the shifted symbols, and I overlooked the most obvious one...

Both space and shift+space are defined as separate bindings, so they absolutely need to be able to distinguish the shift state.

Comment 48

[Steve England [:stevee]]

Bug 425739 was filed on the spacebar issue.. Don't know how you want to resolve it.

Comment 49

[Karl Tomlinson (:karlt)]

Backed out again for bug 425739.

We can either use something like attachment https://bugzilla.mozilla.org/attachment.cgi?id=310911 to restore the FF2 behavior, but that probably only works because of the order of the handler definitions, or use something like Masayuki's approach in bug 359638 of only accepting different Shift states when the character is different.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Karl:

Do you retry to fix this? Or using my approach of bug 359638? I'm waiting this bug...

Comment 51

[Karl Tomlinson (:karlt)]

Masayuki:

I think to fix this reliably we need information about whether the Shift-key changed the character (i.e. whether it is a consumed modifier).

Your unshifted and shifted character codes provide that information, so please fix this bug too if you like.

If unshifted and shifted characters codes are only included in the event when charCode is from level 0 or 1, then (using the notation from bug 359638 comment 69), I think the following sequence could work well when the shift key is pressed:

2.2.3 charCode
2.2.1 shifted charcodes
2.2.4 shifted charcodes without shift key checking only when the shifted charcode differs from the corresponding unshifted char code (in a case-insensitive way).

(I guess the order of 2.2.3 and 2.2.1 doesn't matter when shifted/unshifted charcodes are only present when charCode is from level 0 or 1. i.e. charCode is the first shifted charcode.)

I suggest a small modification to the method in that only the corresponding unshifted charcode is checked for each shifted charCode in 2.2.4 rather than all unshifted charcodes.  I'm saying that because I believe the user would only be thinking about one layout when deciding whether to use Shift.  But you may be able to think of examples to persuade me otherwise?

Comment 52

[[e-mail]]

(In reply to comment #49)
> something like Masayuki's approach in bug 359638 of only
> accepting different Shift states when the character is different.
That doesn't help in the space case, because the character is the same...

Comment 53

[costinel]

I can confirm this bug on latest Firefox trunk using a  HP nc6320 laptop keyboard
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032904 Minefield/3.0pre



However the latest Seamonkey trunk zooms in with either "Ctrl =" and "Ctrl +"
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032900 SeaMonkey/2.0a1pre

Comment 54

[costinel]

Forgot to mention that I am using an US English keyboard.

Comment 55

[Karl Tomlinson (:karlt)]

(In reply to comment #52)
> (In reply to comment #49)
> > something like Masayuki's approach in bug 359638 of only
> > accepting different Shift states when the character is different.
> That doesn't help in the space case, because the character is the same...

I didn't completely explain.  I think the approach we really want is to
consider whether each modifier affected the character generated.  If a
modifier was pressed in order to generate a different character then we
shouldn't require that modifier state to match.

The concept is that of consumed modifiers discussed here:

http://developer.gimp.org/api/2.0/gdk/gdk-Keyboard-Handling.html#gdk-keymap-translate-keyboard-state

For example, if the user has the modifiers Ctrl and Shift pressed and presses
the US key with "=" and "+" then Shift is a consumed modifier and Ctrl is
unconsumed.  We should only require the Ctrl-state to match, and be permissive
about the Shift state.

With the space case the character generated by the key is the same with or
without the Shift, and so in this case Shift is an unconsumed modifier and
should be required to match.

Because we match bicameral letters in a case-insensitive way, Shift should be
considered an unconsumed modifier when it only changes the case of the
character.

Shift is not the only modifier that can be a consumed modifier but it is the
most common.  Masayuki's approach provides information about the characters
that would have been generated with and without Shift and uses that to
determine whether Shift is a consumed modifier.

(In reply to comment #51)
> I think the following sequence could work well when the shift key is
> pressed:
> 
> 2.2.3 charCode
> 2.2.1 shifted charcodes
> 2.2.4 shifted charcodes without shift key checking only when the shifted
> charcode differs from the corresponding unshifted char code (in a
> case-insensitive way).

Actually considering each layout/shifted-charcode in turn would probably be
better.  When charCode is the character produced with Shift, the algorithm
would be something like:

foreach shifted-charcode

  if shifted-charcode case-insensitive-== corresponding-unshifted-charcode

    try shifted-charcode and require Shift to match.  (like 2.2.1)

  else

    try shifted-charcode without requiring Shift to match.  (like 2.2.4)

Comment 56

[Karl Tomlinson (:karlt)]

(In reply to comment #53)
> However the latest Seamonkey trunk zooms in with either "Ctrl =" and "Ctrl +"

Seamonkey apparently has two handlers: one for modifiers="accel" and one for
modifiers="accel,shift":

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/communicator/resources/content/viewZoomOverlay.xul&rev=1.11&mark=52,53#50

But note that there is only modifiers="accel" for key_textZoomReduce so "-"
would be inaccessible on some keyboards.

Comment 57

[[e-mail]]

(In reply to comment #55)
Ah, you only want to ignore the [modifier] state when it changes the character (excluding case changes, since we match case-insensitively anyway). Nice.

Comment 58

[Karl Tomlinson (:karlt)]

Comment on attachment 312150 [details] [diff] [review]
be less picky about the shift modifier even when shift is explicitly required (v5)

(Masayuki's patch in bug 359638 should work better)

Comment 59

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The patch of bug 359638 was landed.

-> FIXED

Comment 61

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task