Closed
Bug 429219
Opened 17 years ago
Closed 16 years ago
Ctrl+1, Ctrl+2, etc, regression (on fr(-FR) keyboard), after bug 359638
Categories
(Firefox :: Keyboard Navigation, defect)
Tracking
()
VERIFIED
FIXED
Firefox 3
People
(Reporter: sgautherie, Assigned: masayuki)
References
(Depends on 1 open bug)
Details
(Keywords: regression, Whiteboard: [key hell])
Attachments
(1 file, 5 obsolete files)
8.67 KB,
patch
|
Gavin
:
review+
roc
:
superreview+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
[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". :-<
Flags: blocking1.9?
Reporter | ||
Updated•17 years ago
|
Summary: Ctrl+1, Ctrl+2, etc, regression, after bug 359638 → Ctrl+1, Ctrl+2, etc, regression (on fr(-FR) keyboard), after bug 359638
Reporter | ||
Comment 1•17 years ago
|
||
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 :-)
Reporter | ||
Comment 2•17 years ago
|
||
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•17 years ago
|
||
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.)
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 4•17 years ago
|
||
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.
Assignee: nobody → masayuki
Assignee | ||
Comment 5•17 years ago
|
||
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/>.
Attachment #315923 -
Flags: ui-review?(beltzner)
Attachment #315923 -
Flags: review?(mconnor)
Assignee | ||
Comment 6•17 years ago
|
||
(In reply to comment #5)
> So, all shortcut keys are handled by <key/>.
oops, I meant: all shortcut keys _should_ be handled by <key/>.
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•17 years ago
|
||
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 :-/
Reporter | ||
Comment 8•17 years ago
|
||
(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.
Assignee | ||
Comment 9•17 years ago
|
||
(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.
Reporter | ||
Comment 10•17 years ago
|
||
(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.
Assignee | ||
Comment 11•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
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()?
Assignee | ||
Comment 14•17 years ago
|
||
(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.
Assignee | ||
Comment 15•17 years ago
|
||
O.K. I can use another way. I'll post the concept patch.
Assignee | ||
Updated•17 years ago
|
Attachment #315923 -
Flags: ui-review?(beltzner)
Attachment #315923 -
Flags: review?(mconnor)
Assignee | ||
Comment 16•17 years ago
|
||
(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•17 years ago
|
||
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?)
Assignee | ||
Comment 18•17 years ago
|
||
(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.
Assignee | ||
Comment 19•17 years ago
|
||
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.
Attachment #315923 -
Attachment is obsolete: true
Attachment #316173 -
Flags: review?(mozbugz)
Attachment #316173 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 20•17 years ago
|
||
(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.
Reporter | ||
Comment 21•17 years ago
|
||
(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.
Assignee | ||
Comment 22•17 years ago
|
||
(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.
Assignee: masayuki → nobody
Status: ASSIGNED → NEW
Component: Keyboard: Navigation → Keyboard Navigation
Flags: blocking1.9+
Product: Core → Firefox
QA Contact: keyboard.navigation → keyboard.navigation
Target Milestone: mozilla1.9 → Firefox 3
Assignee | ||
Comment 23•17 years ago
|
||
Sorry, beltzner, the blocking flag was reset by changing the product.
Assignee: nobody → masayuki
Flags: blocking-firefox3?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Updated•17 years ago
|
Whiteboard: [has patch][needs review gavin]
Comment 24•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
> "if (aCharCode >= zero) break;"
Well, I meant the opposite of that of course ;-)
Assignee | ||
Comment 27•17 years ago
|
||
Thank you, Mats.
Attachment #316173 -
Attachment is obsolete: true
Attachment #316369 -
Flags: review?(mozbugz)
Attachment #316369 -
Flags: review?(gavin.sharp)
Attachment #316173 -
Flags: review?(mozbugz)
Attachment #316173 -
Flags: review?(gavin.sharp)
Comment 28•17 years ago
|
||
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•17 years ago
|
||
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.)
Assignee | ||
Comment 30•17 years ago
|
||
(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•17 years ago
|
||
(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.
Assignee | ||
Comment 32•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
(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•17 years ago
|
||
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.
Assignee | ||
Comment 36•17 years ago
|
||
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•16 years ago
|
||
(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.
Assignee | ||
Comment 38•16 years ago
|
||
(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.
Assignee | ||
Comment 39•16 years ago
|
||
Ignoring the shift key state only when the key is not numeric at unshifted but it is numeric at shifted.
Attachment #316369 -
Attachment is obsolete: true
Attachment #317688 -
Flags: review?(mozbugz)
Attachment #317688 -
Flags: review?(gavin.sharp)
Attachment #316369 -
Flags: review?(mozbugz)
Attachment #316369 -
Flags: review?(gavin.sharp)
Comment 40•16 years ago
|
||
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.
Attachment #317688 -
Flags: review?(mozbugz) → review+
Comment 41•16 years ago
|
||
(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.
Assignee | ||
Comment 42•16 years ago
|
||
(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)
Assignee | ||
Comment 43•16 years ago
|
||
Karl:
Oh, sorry. The patch is older version. This is only changed the '_' problem.
Attachment #317688 -
Attachment is obsolete: true
Attachment #317836 -
Flags: review?(gavin.sharp)
Attachment #317688 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 44•16 years ago
|
||
Comment on attachment 317836 [details] [diff] [review]
Patch v3.1
r+ of core part is carried over.
Attachment #317836 -
Flags: review+
Updated•16 years ago
|
Whiteboard: [has patch][needs review gavin] → [has patch][needs review gavin][key hell]
Comment 46•16 years ago
|
||
we need to add a keyboard navigation litmus suite for these kind of tests
Flags: in-litmus?
Comment 47•16 years ago
|
||
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.
Flags: blocking-firefox3+ → blocking-firefox3-
Comment 48•16 years ago
|
||
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.
Assignee | ||
Comment 49•16 years ago
|
||
Gavin:
#subst is unknown preprocessor on Win32... I failed to build with the approach.
Comment 50•16 years ago
|
||
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.
Attachment #317836 -
Attachment is obsolete: true
Attachment #318609 -
Flags: review+
Attachment #317836 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Whiteboard: [has patch][needs review gavin][key hell] → [has patch][key hell]
Reporter | ||
Comment 51•16 years ago
|
||
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') {
Assignee | ||
Comment 52•16 years ago
|
||
(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•16 years ago
|
||
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.
Flags: in-testsuite?
Assignee | ||
Comment 54•16 years ago
|
||
Comment on attachment 318609 [details] [diff] [review]
patch
o.k. I confirmed the Persian issue on Linux. It's not regressed.
Attachment #318609 -
Flags: ui-review?(beltzner)
Attachment #318609 -
Flags: superreview?(roc)
Attachment #318609 -
Flags: superreview?(roc) → superreview+
Comment 55•16 years ago
|
||
Comment on attachment 318609 [details] [diff] [review]
patch
This doesn't need UI-review, since it doesn't impact UI.
Attachment #318609 -
Flags: ui-review?(beltzner)
Assignee | ||
Updated•16 years ago
|
Attachment #318609 -
Flags: approval1.9?
Comment 56•16 years ago
|
||
Comment on attachment 318609 [details] [diff] [review]
patch
a1.9+=damons
Attachment #318609 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 57•16 years ago
|
||
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][key hell] → [key hell]
Reporter | ||
Comment 58•16 years ago
|
||
[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.)
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•