429219 - Ctrl+1, Ctrl+2, etc, regression (on fr(-FR) keyboard), after bug 359638

Status: VERIFIED FIXED

(Firefox :: Keyboard Navigation, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041401 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Up to this build, it has always "worked".


[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041502 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041513 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

Ctrl + "number" still work ...
... but, to achieve that, on French keyboard,
you actually have (now) to press Ctrl + _Shift_ + "number", on the left alpha-num keyboard.

(Ctrl + "number", using the right num-only keyboard, still works.)

Strictly speaking, I used to press Ctrl+& (1), Ctrl+é (2), etc,
so the new behavior is more consistent with what the Window menu says the shortcuts are.

Trouble is, having to press the Shift key too, kinda defeats the purpose of these "shortcuts" :-/

I'm not sure how I would want this to be "re-fixed". :-<

Comment 1

[Serge Gautherie (:sgautherie)]

More details:

Ctrl+"&" (1), as an example, with:

CapsLock + Shift = Result

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b4pre) Gecko/2008022702 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

No  + No  = Works
No  + Yes = nothing
Yes + No  = Works
Yes + Yes = nothing

Then:
*alpha-num: CapsLock is ignored, Shift acts independently.
*num-only : Ctrl+1 (in NumLock mode)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008041513 SeaMonkey/2.0a1pre] (SEA-WIN32-TBOX-trunk) (W2Ksp4)

No  + No  = nothing
No  + Yes = Works
Yes + No  = Works
Yes + Yes = Works

Then:
*alpha-num: CapsLock and/or Shift get it to work.
 Shouldn't we still have 2 "Works" and 2 "nothing" ?
*num-only : same as usual :-)

Comment 2

[Serge Gautherie (:sgautherie)]

That's with (en-US SeaMonkey and) fr-FR Windows/locale/keyboard.

Compared to bug 429180 comment 2,
it seems bug 359638 did break here what was working as intended.

Comment 3

[Karl Tomlinson (:karlt)]

With Gecko 1.8 en-US app and fr-FR keyboard:
Did Ctrl-à or Ctrl-Shift-0 or both reset zoom?
Did Ctrl-- decrease text size (or produce Ctrl-6)?

(Also, the CapsLock situation doesn't seem right.)

Comment 4

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

We have an problem. Cmd+Shift+Num are reserved by system on MacOSX.

Therefore, we should be able to access them without shift key. So, on the current implementation, Ctrl+6 may not work with my idea on Mac. The patch is going to come.

Comment 5

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

Attachment: Patch v1.0 for browser

I'm not sure on Mac's special issue. I cannot solve that today. So, we should fix this on non-Mac platforms.

Browser code handles the Ctrl+[1-9] itself. But Javascript cannot access to the alternative charCodes. So, all shortcut keys are handled by <key/>.

Comment 6

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

(In reply to comment #5)
> So, all shortcut keys are handled by <key/>.

oops, I meant: all shortcut keys _should_ be handled by <key/>.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 315923 [details] [diff] [review]
Patch v1.0 for browser

(In reply to comment #5)
> Browser code handles the Ctrl+[1-9] itself. But Javascript cannot access to the
> alternative charCodes. So, all shortcut keys are handled by <key/>.

Do you mean that, while bug 359638 was a Core patch,
now, each/every application (FF, TB, SM, ...) will need its "duplicated" workaround ?
That doesn't sound good at all to me :-/

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> With Gecko 1.8 en-US app and fr-FR keyboard:

1.8.0 is obsolete, I think.
A regression between 1.8.1 and 1.9.0 (before bug 359638 landed) would be a separate bug.

> Did Ctrl-à or Ctrl-Shift-0 or both reset zoom?
> Did Ctrl-- decrease text size (or produce Ctrl-6)?

What should "I" actually be looking for ?
(other than what I already reported in this bug.)

I agree that having an action associated with both Ctrl+- and Ctrl+6 seems to be looking for trouble on an fr-FR keyboard :-/
(I had never noticed this, as I use Ctrl+- on the num-only keyboard, and use the menu to launch ChatZilla :-<)
For this too, I don't know how I would want it to work...

> (Also, the CapsLock situation doesn't seem right.)

May be the new behavior is better on the overall ?
But for the "inconsistency" with CapsLock.

Comment 9

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

(In reply to comment #7)
> (From update of attachment 315923 [details] [diff] [review])
> (In reply to comment #5)
> > Browser code handles the Ctrl+[1-9] itself. But Javascript cannot access to the
> > alternative charCodes. So, all shortcut keys are handled by <key/>.
> 
> Do you mean that, while bug 359638 was a Core patch,
> now, each/every application (FF, TB, SM, ...) will need its "duplicated"
> workaround ?
> That doesn't sound good at all to me :-/

Because such "cheap" handling cannot care the intl keyboard layout problem. The XUL apps should not handle the key events themselves. They should use <key/> for command handling. See bug 359638's discussion.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
> (In reply to comment #7)
> > Do you mean that, while bug 359638 was a Core patch,
> > now, each/every application (FF, TB, SM, ...) will need its "duplicated"
> > workaround ?
> > That doesn't sound good at all to me :-/
> 
> Because such "cheap" handling cannot care the intl keyboard layout problem. The
> XUL apps should not handle the key events themselves. They should use <key/>
> for command handling. See bug 359638's discussion.

I had a look at that bug, but it has 143 comments :-/
(I don't know which one to look for)
Then, I take your reply as a "Yes" to my question.

Comment 11

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

(In reply to comment #10)
> I had a look at that bug, but it has 143 comments :-/
> (I don't know which one to look for)
> Then, I take your reply as a "Yes" to my question.

Yes. The many comments mean the intl keyboard layout support is too difficult. The developers need to think very many things. So, such complex things should not be in *every* key event handler. Therefore, the key element should be used for the command handling always.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> (In reply to comment #3)
> > With Gecko 1.8 en-US app and fr-FR keyboard:
> 
> 1.8.0 is obsolete, I think.

You are right, sorry.  I meant 1.8.1.

> > Did Ctrl-à or Ctrl-Shift-0 or both reset zoom?
> > Did Ctrl-- decrease text size (or produce Ctrl-6)?
> 
> What should "I" actually be looking for ?
> (other than what I already reported in this bug.)

What I would like to know is whether the behavior of 1.8.1 is the same as what you reported for "rv:1.9pre) Gecko/2008041401" (before bug 359638 landed).

> I agree that having an action associated with both Ctrl+- and Ctrl+6 seems to
> be looking for trouble on an fr-FR keyboard :-/

Just as well we have a Shift key ;) ... except on Mac!
(Maybe Ctrl is available as an alternative to Cmd on Mac?)

> May be the new behavior is better on the overall ?

I thought so, but I'm interested in whether the behavior has always been the same in the past.

Comment 13

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

Comment on attachment 315923 [details] [diff] [review]
Patch v1.0 for browser

Won't this regress bug 298004? This adds a lot of redundant elements. Why does the code that was here before no longer work? Why are you removing the preventDefault() and stopPropagation()?

Comment 14

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

(In reply to comment #13)
> (From update of attachment 315923 [details] [diff] [review])
> Won't this regress bug 298004?

Oh, the non-ASCII numeric...

> This adds a lot of redundant elements. Why does
> the code that was here before no longer work?

For bug 414130, now we send the original charCode with the key event. Therefore, Ctrl+Num doesn't have numeric in the charCode. But nsKeyEvent::alternativeCharCodes has unmodified charCodes. C++ code can access and check it. But JS code cannot access to it.

Comment 15

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

O.K. I can use another way. I'll post the concept patch.

Comment 16

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

(In reply to comment #15)
> O.K. I can use another way. I'll post the concept patch.

er, no. JS code should not be possible to access the alternative charCodes. Because they have important order. So, the each apps might break the order...

Comment 17

[Karl Tomlinson (:karlt)]

Masayuki, do you know why Ctrl+CapsLock+& without Shift produces 1?

Serge, does CapsLock+& without Shift and without Ctrl produce 1 or &?
(i.e is it a ShiftLock rather than CapsLock?)

Comment 18

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

(In reply to comment #17)
> Masayuki, do you know why Ctrl+CapsLock+& without Shift produces 1?

Yes.

> Serge, does CapsLock+& without Shift and without Ctrl produce 1 or &?
> (i.e is it a ShiftLock rather than CapsLock?)

On Win32, CapsLock+Ctrl+& sends 1.

Comment 19

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

Attachment: Patch v2.0 (for browser)

Let's use this way.

1. XUL apps use <key/> for command handling.
2. for bug 298004, we should also append ASCII numeric to the shortcut candidates when the Unicode numeric is pressed.

Comment 20

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

(In reply to comment #19)
> Created an attachment (id=316173) [details]
> Patch v2.0 (for browser)
> 
> Let's use this way.
> 
> 1. XUL apps use <key/> for command handling.
> 2. for bug 298004, we should also append ASCII numeric to the shortcut
> candidates when the Unicode numeric is pressed.

Oops, if you want to test the patch, please apply attachment 315912 [details] [diff] [review] first. That is needed by this patch.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #11)
> So, such complex things should
> not be in *every* key event handler. Therefore, the key element should be used
> for the command handling always.

I believe you on this.
I guess what I was asking was whether these key/command bindings could live in "core/toolkit" and be inherited by all the apps,
rather than needed to be "duplicated" in each of them ?

(In reply to comment #17)
> Serge, does CapsLock+& without Shift and without Ctrl produce 1 or &?

(Both new and old SeaMonkey, or other application)
Without Ctrl:
& : no Shift and no CapsLock, or both.
1 : either Shift *or* CapsLock.

Comment 22

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

(In reply to comment #21)
> (In reply to comment #11)
> > So, such complex things should
> > not be in *every* key event handler. Therefore, the key element should be used
> > for the command handling always.
> 
> I believe you on this.
> I guess what I was asking was whether these key/command bindings could live in
> "core/toolkit" and be inherited by all the apps,
> rather than needed to be "duplicated" in each of them ?

no, we should separate the bugs for each app/commands.

moving this product to Firefox, sorry. The release deadline is too near. I cannot work on other apps now.

Comment 23

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

Sorry, beltzner, the blocking flag was reset by changing the product.

Comment 24

[Tony Mechelynck [:tonymec]]

In reply to comment #21

This may be off-topic, but just in case you guys try to write a cross-platform patch: on my openSUSE Linux 10.3 with KDE desktop and AZERTY (fr-BE) keyboard, CapsLock applies _only_ to [a-zA-Z].

(From what I read above, on Windows it's different, at least on fr-FR keyboards).

Comment 25

[Mats Palmgren (inactive)]

Just a drive-by comment:

+       aCharCode >= zero && i < NS_ARRAY_LENGTH(kZero); ++i, zero = kZero[i]) {

kZero[i] will potentially access outside the array.  Couldn't you just
move the assignment of 'zero' inside the loop and do the test with a 
"if (aCharCode >= zero) break;" 

Comment 26

[Mats Palmgren (inactive)]

> "if (aCharCode >= zero) break;" 

Well, I meant the opposite of that of course ;-)

Comment 27

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

Attachment: Patch v2.1

Thank you, Mats.

Comment 28

[Karl Tomlinson (:karlt)]

I did some experimenting with a US-localization Minefield (Firefox) and a
French keyboard layout assuming my US keyboard was interpreted by Windows as
shown here:

http://en.wikipedia.org/wiki/Keyboard_layout#French

For a 1.9pre 2008041707 build (without the patch, sorry)

This is the behavior I observed and I'm using a * to indicate what I consider
unexpected behavior.  Note that unlike SeaMonkey, Firefox is not able to
switch tabs even with Shift.

  Ctrl+- decreases size
* Ctrl+Shift+[1-6] have no effect (I only opened 6 tabs)
  Ctrl+Shift+0 resets size
  Ctrl+à has no effect

I'm not really sure what the best behavior is with ShiftLock.  Maybe it's the
*-marked cases that are wrong, or maybe its the others, but at least something
is wrong.

* ShiftLock+Ctrl+Shift+- has no effect
  ShiftLock+Ctrl+[1-6] switch tabs
  ShiftLock+Ctrl+0 resets size
* ShiftLock+Ctrl+Shift+à resets size

For 1.8.1.11 Firefox 2.0.0.11, Shift was basically ignored, which meant that
Ctrl+- wasn't available to decrease size.

* Ctrl+- switches to tab 6
  Ctrl+Shift+[1-6] switch tabs
  Ctrl+Shift+0 resets text size
* Ctrl+à resets text size

ShiftLock was also ignored.  Maybe that's the best behavior.

* ShiftLock+Ctrl+Shift+- switches to tab 6
  ShiftLock+Ctrl+[1-6] switch tabs
  ShiftLock+Ctrl+0 resets text size
* ShiftLock+Ctrl+Shift+à resets text size

So, at this stage, there are two issues here that I'm convinced are bugs:

1. Ctrl+Shift+[1-9] doesn't switch tabs in Firefox
2. ShiftLock behavior is inconsistent.

Is the ShiftLock issue covered by bug 429510?

Comment 29

[Karl Tomlinson (:karlt)]

Comment on attachment 316369 [details] [diff] [review]
Patch v2.1

It looks like this is addressing the Ctrl+Shift+[1-9] issue.

Isn't this problem caused by testing for Shift here?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.1027&mark=1321,1324#1315

and wouldn't the simplest fix be to remove the test for Shift there?

(There is a conflict with numeric document access keys [Bug 366084] on Linux
with French/Belgian keyboards whichever way this is fixed.)

Comment 30

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

(In reply to comment #29)
> Isn't this problem caused by testing for Shift here?
> 
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.1027&mark=1321,1324#1315

Yes.

> and wouldn't the simplest fix be to remove the test for Shift there?

I don't think so. Because this hardcoding makes the disability of localizing the keys. Now, we cannot change the DTD, so, we cannot improve the localizability, however, they should be localizable in next release. For that, we should use key elements for this.

And if we check the shift key state, it's too complex. It influences to other keyboard layouts. And when the command of key elements and Ctrl+Num conflicts, we cannot control the priority.

> (There is a conflict with numeric document access keys [Bug 366084] on Linux
> with French/Belgian keyboards whichever way this is fixed.)

Oh, really? Mac also has same issue. I think Ctrl+Num is too unusable key combination for French.

(If we also allow to Ctrl without Shift + num keys in the normal keys, I can fix that problem on nsContentUtils::GetAccelKeyCandidates(). If the shiftedCharCode is numeric, we add the charCodes to the result when the shift key is not pressed. However, Ctrl+- vs Ctrl+6 issue, Ctrl+- wins. Ctrl+6 can be accessed from Numpad or with Shift key. I think that this approach is smart and simple.)

Comment 31

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

(In reply to comment #30)
> > and wouldn't the simplest fix be to remove the test for Shift there?
> 
> I don't think so. Because this hardcoding makes the disability of localizing
> the keys. Now, we cannot change the DTD, so, we cannot improve the
> localizability, however, they should be localizable in next release. For that,
> we should use key elements for this.

Why do these need to be localized? The previous code handles all forms of numbers, and does so without duplicating many key/command elements.

> And if we check the shift key state, it's too complex. It influences to other
> keyboard layouts. And when the command of key elements and Ctrl+Num conflicts,
> we cannot control the priority.

I'm not sure I understand this.

Comment 32

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

(In reply to comment #31)
> (In reply to comment #30)
> > > and wouldn't the simplest fix be to remove the test for Shift there?
> > 
> > I don't think so. Because this hardcoding makes the disability of localizing
> > the keys. Now, we cannot change the DTD, so, we cannot improve the
> > localizability, however, they should be localizable in next release. For that,
> > we should use key elements for this.
> 
> Why do these need to be localized? The previous code handles all forms of
> numbers, and does so without duplicating many key/command elements.

I think that the all command key handling should be localizable. The localizers should be able to decide whether each command key combination should be localized.

> > And if we check the shift key state, it's too complex. It influences to other
> > keyboard layouts. And when the command of key elements and Ctrl+Num conflicts,
> > we cannot control the priority.
> 
> I'm not sure I understand this.

karlt decided that we should distinguish between shifted charCode and unshifted charCode at command handling. i.e., if Accel+= is not assigned to zoom-in, Accel+= should not work as zoom-in in US keyboard layout. That should work only when Accel+*Shift*+= is pressed.

If we always ignore the shift key state for some keyboard layout at handling the tab switching, Accel+Shift+Num will be also working as tab switching. And that may conflict with other commands.

And note that shift + '-' is '6' on French keyboard layout. So, I think Accel+Shift+- should be tab switching command. And Accel+- should be zoom-out. Even if we allow Accel+any working as Accel+num when the shifted charCode is numeric, Accel+- should win such behavior. This priority should be managed in command handling code.

Comment 33

[Karl Tomlinson (:karlt)]

(In reply to comment #32)
> If we always ignore the shift key state for some keyboard layout at handling
> the tab switching, Accel+Shift+Num will be also working as tab switching. And
> that may conflict with other commands.

Only if the charCode is still Num.  If it is another char (e.g. "-") as it would be with the top row of the main keyboard then that javascript code would (correctly) not match.

You are right though that javascript code cannot determine whether the Shift key modifies the charCode (with the current API), so if we want to distinguish between Ctrl-[0-9] and Ctrl-Shift-[0-9] from the numeric keypad, then we need more complicated logic and it would be nice to share that.

Comment 34

[Karl Tomlinson (:karlt)]

(In reply to comment #33)
> if we want to distinguish between Ctrl-[0-9] and Ctrl-Shift-[0-9] from
> the numeric keypad,

But I'm not sure that we do want to distinguish these.  Any behavior that relied on such a distinction would not be (readily) available on a laptop.

Comment 35

[Karl Tomlinson (:karlt)]

Comment on attachment 316369 [details] [diff] [review]
Patch v2.1

Bug 298004 was originally filed for a Persian keyboard layout, where IIUC
the Eastern Arabic numerals on that layout are in the same positions as the
European forms of Hindu-Arabic numerals on US keyboards. 

However, note that on some keyboards the digits are in different locations.
The first Thai numeral in the Kedmanee layout is on the key used for 2 in the
US layout.

http://www.thai-guide.info/language/computing/thai-keyboards-and-typing-in-thai.html

I'm afraid of the problems that may result from adding ASCII digits based on
the character's mathematical value.

We already add an ASCII digit to the alternative characters based on position
in a Latin keyboard layout when the key does not normally produce an ASCII
digit (on Windows and Linux - do we do anything on Mac?).

If a shortcut advertises the "2" key (in a menu for example) then I think the
key corresponding to the "2" character in US/European layout is the
appropriate key.

If we also add another digit based on the mathematical value of the character,
then there would sometimes be two different systems active at once, which
could lead to some confusion.

For tab switching AFAIK "2" is not actually advertised anywhere.  We
can choose whether it is the keyboard position (relative to European/US
layout) or the mathematical value of the native character.  But we should not
try both.

Comment 36

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

Karl:

Do you think that we should not try to that when the alternative chars are more than 2? (If the alternative char count is only one, widget didn't append the non-current keyboard layout characters.)

If we need to check the alternative char count, we should fix bug 306585 first. That will append the modifier key mask to each chars. That helps the alternative char counting.

Comment 37

[Karl Tomlinson (:karlt)]

(In reply to comment #36)
> Do you think that we should not try to that when the alternative chars are
> more than 2? (If the alternative char count is only one, widget didn't
> append the non-current keyboard layout characters.)

I'm not comfortable adding an ASCII digit in
nsContentUtils::GetAccelKeyCandidates or similar based on the mathematical
value of the character in any situation (because I think any shortcut labelled [for example] "3" should only be activated with a key physically corresponding to the "3" character.)

I'm happy with either of the following solutions:

A. Modify ctrlNumberTabSelection to only check the character, not the state of
   the Shift key (and so the shortcut would continue to be based on the
   mathematical value of the character rather than the physical key).

   This code doesn't seem to need the additional information in the native
   event, because most keyboard layouts provide a way to enter a numeral of
   some sort and this is provided in charCode.

   As Masayuki points out, this method doesn't provide for localizers wanting
   to do something different if the numerals are in inconvenient locations
   for example, but then it's too late to do that for FF3 anyway.

B. Use XUL key elements so that the shortcuts are based on the physical keys
   corresponding to ASCII digits.  If there is some situation where these
   ASCII digits from a US/European layout are not in the alternative character
   list but should be, then we should fix that rather than doing a translation
   based on the mathematical value.

(In reply to comment #30)
> (If we also allow to Ctrl without Shift + num keys in the normal keys, I can
> fix that problem on nsContentUtils::GetAccelKeyCandidates(). If the
> shiftedCharCode is numeric, we add the charCodes to the result when the
> shift key is not pressed. However, Ctrl+- vs Ctrl+6 issue, Ctrl+-
> wins.)

Solution B would allow us to try the numeral as a fallback alternative character
(like you say) if we consider that these shortcuts are important enough that
they should be accessible without requiring the Shift key on a French layout.

There may be a risk here of conflicting with XUL access keys on Linux.
I don't know which are tested first, but then I'm not aware of any situations
where digits are active access keys in the same window as the
Browser:NumberTabSelection shortcuts.

Comment 38

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

(In reply to comment #37)
> (In reply to comment #30)
> > (If we also allow to Ctrl without Shift + num keys in the normal keys, I can
> > fix that problem on nsContentUtils::GetAccelKeyCandidates(). If the
> > shiftedCharCode is numeric, we add the charCodes to the result when the
> > shift key is not pressed. However, Ctrl+- vs Ctrl+6 issue, Ctrl+-
> > wins.)
> 
> Solution B would allow us to try the numeral as a fallback alternative
> character
> (like you say) if we consider that these shortcuts are important enough that
> they should be accessible without requiring the Shift key on a French layout.

See bug 306585 comment 11 and bug 306585 comment 12. Ignoring the Shift key state is good thing for Mac.

However, I'm still not sure that the physical key matching should be correct.

Comment 39

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

Attachment: Patch v3.0

Ignoring the shift key state only when the key is not numeric at unshifted but it is numeric at shifted.

Comment 40

[Karl Tomlinson (:karlt)]

Comment on attachment 317688 [details] [diff] [review]
Patch v3.0

I'm happy with this logic thanks.

+      return _PR_TRUE;

PR_TRUE.

r=me on nsContentUtils.cpp with that change.

I'll leave the browser parts to Gavin.

Comment 41

[Karl Tomlinson (:karlt)]

(In reply to comment #38)
> See bug 306585 comment 11 and bug 306585 comment 12. Ignoring the Shift key
> state is good thing for Mac.

I don't understand the relevant of the Option key to the Shift key.
Are you making the comparison because Ctrl+Shift+[0-9] also requires disabling (or changing) system shortcuts?

Anyway, I'm not objecting to using the digit from the Shift level as a fallback.

Comment 42

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

(In reply to comment #41)
> (In reply to comment #38)
> > See bug 306585 comment 11 and bug 306585 comment 12. Ignoring the Shift key
> > state is good thing for Mac.
> 
> I don't understand the relevant of the Option key to the Shift key.
> Are you making the comparison because Ctrl+Shift+[0-9] also requires disabling
> (or changing) system shortcuts?

Oops, I forgot to point comment 4 of this bug. Cmd+Shift+Num may be reserved by system on Mac. So, we keep to check the Shift state, some keys combination cannot be used on AZERTY. (The key combination is not sent to us. So, MacOS eats the key event.)

Of course, Cmd+Shift+'-'('6') is reserved by system, French users cannot use Cmd+6. However, that is not the scope of this bug. It should be fixed by the localized build. ('-' should not be used for zoom-out, maybe)

Comment 43

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

Attachment: Patch v3.1

Karl:

Oh, sorry. The patch is older version. This is only changed the '_' problem.

Comment 44

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

Comment on attachment 317836 [details] [diff] [review]
Patch v3.1

r+ of core part is carried over.

Comment 45

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

Gavin:

Would you review the XUL part??

Comment 46

[Tony Chung [:tchung]]

we need to add a keyboard navigation litmus suite for these kind of tests

Comment 47

[Damon Sicore (:damons)]

We're at a point here were we don't have enough information to make a decision on what the correct behavior should be here.  So, the only thing we can really do here is minus this, CC as many people who we think might be affected, and hope for feedback.  

Gavin, also, looking forward to your review to determine if we should take this patch.

Comment 48

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

Comment on attachment 317836 [details] [diff] [review]
Patch v3.1

>Index: browser/base/content/browser-sets.inc

Can't you use <key oncommand="BrowserNumberTabSelection(event, ...)" and avoid all the command elements? You could also use a preprocessor #define to avoid duplicating all those <key> elements:

#ifdef XP_GNOME
#define NUM_SELECT_TAB_MODIFIER alt
#else
#define NUM_SELECT_TAB_MODIFIER accel
#endif

and then use, e.g.:
#subst    <key id="key_selectTab9" command="Browser:NumberTabSelection9" key="9" modifiers="__NUM_SELECT_TAB_MODIFIER__"/>

I think I'm OK with this if those are addressed.

Comment 49

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

Gavin:

#subst is unknown preprocessor on Win32... I failed to build with the approach.

Comment 50

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

Attachment: patch

Sorry, it is #expand and not #subst. For some reason #subst worked for me when I tried it in a different testcase, not sure what I did differently.

This patch works for me. You have tested that it doesn't regress bug 298004, right? r=me with that verified.

Comment 51

[Serge Gautherie (:sgautherie)]

Comment on attachment 318609 [details] [diff] [review]
patch

>Index: browser/base/content/browser-sets.inc
>===================================================================
>+#expand    <key id="key_selectTab1" oncommand="BrowserNumberTabSelection(event, 0);" key="1" modifiers="__NUM_SELECT_TAB_MODIFIER__"/>
...
>+#expand    <key id="key_selectTab9" oncommand="BrowserNumberTabSelection(event, 8);" key="9" modifiers="__NUM_SELECT_TAB_MODIFIER__"/>

Shouldn't there be a |key="0"| too ?

>Index: content/base/src/nsContentUtils.cpp
>===================================================================
>+    if (ch >= '0' && ch <= '9')
...
>+          if (ch >= '0' && ch <= '9') {

Comment 52

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

(In reply to comment #51)
> (From update of attachment 318609 [details] [diff] [review])
> >Index: browser/base/content/browser-sets.inc
> >===================================================================
> >+#expand    <key id="key_selectTab1" oncommand="BrowserNumberTabSelection(event, 0);" key="1" modifiers="__NUM_SELECT_TAB_MODIFIER__"/>
> ...
> >+#expand    <key id="key_selectTab9" oncommand="BrowserNumberTabSelection(event, 8);" key="9" modifiers="__NUM_SELECT_TAB_MODIFIER__"/>
> 
> Shouldn't there be a |key="0"| too ?

'0' is used for ZoomReset. not TabSelection.

(In reply to comment #50)
> This patch works for me. You have tested that it doesn't regress bug 298004,
> right? r=me with that verified.

Thank you for the patch. I'll test it tomorrow.

Comment 53

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

We should be able to write a browser chrome test for Ctrl+<Num> tab switching behavior in general, even if it doesn't catch this bug specifically given the need to synthesize key events at a lower level.

Comment 54

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

Comment on attachment 318609 [details] [diff] [review]
patch

o.k. I confirmed the Persian issue on Linux. It's not regressed.

Comment 55

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

Comment on attachment 318609 [details] [diff] [review]
patch

This doesn't need UI-review, since it doesn't impact UI.

Comment 56

[Damon Sicore (:damons)]

Comment on attachment 318609 [details] [diff] [review]
patch

a1.9+=damons

Comment 57

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

-> FIXED

Comment 58

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008050303 Minefield/3.0pre] (FX-WIN32-TBOX-trunk) (W2Ksp4)

V.Fixed, with Firefox.

(I filed bug 432002 as a follow-up for SeaMonkey.)