Closed
Bug 403387
Opened 17 years ago
Closed 17 years ago
Tab switching shortcuts activated even with extra modifiers (interferes with e.g. Ctrl+Alt+5 for euro sign)
Categories
(Firefox :: Keyboard Navigation, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3 beta3
People
(Reporter: k.schasfoort, Assigned: jruderman)
Details
(Keywords: intl)
Attachments
(1 file)
1.28 KB,
patch
|
Gavin
:
review+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; nl; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Can't use symbol in text fields like the euro symbol: € (Ctrl+Alt+5) becouse when you hit Ctrl+5, you get the 5th tab.
Reproducible: Always
Steps to Reproduce:
1. Go to a text-field
2. Make a Euro symbol or other symbol you make with Ctrl+Alt+[number]
3. Firefox sees it like Ctrl+[number], so you get a tab
Actual Results:
You get an other tab and not the symbol
Expected Results:
Make the symbol and not show a tab.
Comment 1•17 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111023 Minefield/3.0b2pre
The alt gr key ( http://nl.wikipedia.org/wiki/Alt_Gr ), present on my laptop keyboard, works well here to make euro's: €€€. Also Ctrl+Alt + 5 makes euro's here. €€€. I go indeed to the fifth tab if I first press Ctrl+5 and then Alt.
Assignee | ||
Comment 2•17 years ago
|
||
This patch makes the tab-switching shortcuts more precise. For example, on Mac, it makes them only trigger on Cmd+number and not on Cmd+Ctrl+number or Cmd+Alt+number.
I'll ask someone to test on Windows trunk (both without and with the patch) before requesting review from Mano.
I'd like to confirm the bug: on Windows XP using Ctrl+Alt+number or Ctrl+Shift+number selects the numbered tab, but it shouldn't. This behaviour prevents extensions to assign custom actions to Ctrl+Alt+number or Ctrl+Shift+number key combinations.
Assignee | ||
Comment 4•17 years ago
|
||
telega, can you test my patch on Windows and tell me whether it fixes the bug (without breaking Ctrl+number tab switching)? I'm on Mac and I need someone to test it on Windows before I request review.
Assignee | ||
Comment 6•17 years ago
|
||
Getting Firefox building on Windows takes some effort (see http://developer.mozilla.org/en/docs/Build_Documentation). But once you get it building, just apply this patch using the "patch" utility, rebuild, and see if it fixes the bug.
I think this patch is an improvement even if it doesn't fix the bug, so I'm going to request review. If it goes in, you can test a nightly to see if it fixes this bug.
Assignee | ||
Updated•17 years ago
|
Attachment #288231 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•17 years ago
|
||
Gijs verified that the patch has the desired effect on the tab-switching shortcuts on Windows, btw.
Comment 9•17 years ago
|
||
Confirming. Gavin, can you take a look at this patch? It's very small, but it fixes a rather annoying problem for people (also see the bug I just duped).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Can't use symbol like euro € (Ctrl+Alt+5) becouse when you hit Ctrl+5, you get the 5th tab. → Tab switching shortcuts activated even with extra modifiers (when using a euro sign or ZWNJ)
Updated•17 years ago
|
Assignee | ||
Updated•17 years ago
|
Summary: Tab switching shortcuts activated even with extra modifiers (when using a euro sign or ZWNJ) → Tab switching shortcuts activated even with extra modifiers (interferes with e.g. Ctrl+Alt+5 for euro sign)
Comment 10•17 years ago
|
||
Comment on attachment 288231 [details] [diff] [review]
possible patch
>Index: browser/base/content/browser.js
> #ifdef XP_MACOSX
>- if (!event.metaKey)
>+ // Mac: Cmd+number
>+ if (!event.metaKey || event.ctrlKey || event.altKey || event.shiftKey)
> #else
> #ifdef XP_UNIX
>- // don't let tab selection clash with numeric accesskeys (bug 366084)
>- if (!event.altKey || event.shiftKey)
>+ // Linux: Alt+number
>+ if (!event.altKey || event.ctrlKey || event.metaKey || event.shiftKey)
> #else
>- if (!event.ctrlKey)
>+ // Windows: Ctrl+number
>+ if (!event.ctrlKey || event.metaKey || event.altKey || event.shiftKey)
> #endif
> #endif
Can you factor out the common parts into a separate check rather than adding them to each conditional? It will make the pre-processed (pre-pre-processed?) source easier to read.
Attachment #288231 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 11•17 years ago
|
||
The only common parts are "if" and "event.shiftKey". I suppose I could replace
#ifdef XP_MACOSX
// Mac: Cmd+number
if (!event.metaKey || event.ctrlKey || event.altKey || event.shiftKey)
#else
#ifdef XP_UNIX
// Linux: Alt+number
if (!event.altKey || event.ctrlKey || event.metaKey || event.shiftKey)
#else
// Windows: Ctrl+number
if (!event.ctrlKey || event.metaKey || event.altKey || event.shiftKey)
#endif
#endif
return;
with
if (event.shiftKey ||
#ifdef XP_MACOSX
// Mac: Cmd+number
!event.metaKey || event.ctrlKey || event.altKey)
#else
#ifdef XP_UNIX
// Linux: Alt+number
!event.altKey || event.ctrlKey || event.metaKey)
#else
// Windows: Ctrl+number
!event.ctrlKey || event.metaKey || event.altKey)
#endif
#endif
return;
Do you want me to do that? I don't think it makes much of a difference to readability.
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → jruderman
Comment 12•17 years ago
|
||
Oh, right. Ignore my comment, then.
Assignee | ||
Updated•17 years ago
|
Attachment #288231 -
Flags: approval1.9?
Comment 13•17 years ago
|
||
Is it right just to replace the patched "browser.js" with the one in "chrome/browser.jar" or I need to go through the whole building process?
Comment 14•17 years ago
|
||
Comment on attachment 288231 [details] [diff] [review]
possible patch
a=beltzner
Attachment #288231 -
Flags: approval1.9? → approval1.9+
Comment 15•17 years ago
|
||
(In reply to comment #13)
> Is it right just to replace the patched "browser.js" with the one in
> "chrome/browser.jar" or I need to go through the whole building process?
Once you've applied the patch to your source tree, you can just |make -C <objdir>/browser/base| (followed by |make -C browser/app| if you're on a Mac). If you want to manually patch your build you'll have to replace the browser.js that lives inside the browser.jar package.
Comment 16•17 years ago
|
||
(In reply to comment #13)
> Is it right just to replace the patched "browser.js" with the one in
> "chrome/browser.jar" or I need to go through the whole building process?
browser.js needs to be preprocessed before being added to the jar file. You'll need the build process, unless you're willing to go through the code and look for lines beginning with # and preprocess them by hand... Now that this patch has an approval, it will be checked in soon, so you can test it in the first nightly after this patch.
Target Milestone: --- → Firefox 3 M11
Comment 17•17 years ago
|
||
(In reply to comment #16)
> (In reply to comment #13)
> > Is it right just to replace the patched "browser.js" with the one in
> > "chrome/browser.jar" or I need to go through the whole building process?
>
> browser.js needs to be preprocessed before being added to the jar file. You'll
> need the build process, unless you're willing to go through the code and look
> for lines beginning with # and preprocess them by hand... Now that this patch
> has an approval, it will be checked in soon, so you can test it in the first
> nightly after this patch.
>
I went through the code and replaced
//@line 1281 "/e/fx19rel/WINNT_5.2_Depend/mozilla/browser/base/content/browser.js"
if (!event.ctrlKey)
{
//@line 1284 "/e/fx19rel/WINNT_5.2_Depend/mozilla/browser/base/content/browser.js"
with
//@line 1281 "/e/fx19rel/WINNT_5.2_Depend/mozilla/browser/base/content/browser.js"
if (!event.ctrlKey || event.metaKey || event.altKey || event.shiftKey)
{
//@line 1284 "/e/fx19rel/WINNT_5.2_Depend/mozilla/browser/base/content/browser.js"
Now the tab switching shortcut is not activated, but the character I was going to insert (ZWNJ using Ctrl+Shift+2) is not inserted either.
Assignee | ||
Comment 18•17 years ago
|
||
Patch checked in. If you still can't enter ZWNJ or the euro currency symbol in tomorrow morning's builds, please file a new bug report and cite the bug number here. I don't think that problem would be due to this code.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 19•17 years ago
|
||
Typing the ZWNJ doesn't work with this patch as well. I'll reopen bug 414130 to track this issue. Any ideas on the possible cause?
No longer blocks: Persian-Fx3.5
Flags: blocking-firefox3?
You need to log in
before you can comment on or make changes to this bug.
Description
•