Tab switching shortcuts activated even with extra modifiers (interferes with e.g. Ctrl+Alt+5 for euro sign)

RESOLVED FIXED in Firefox 3 beta3

Status

()

RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: k.schasfoort, Assigned: jruderman)

Tracking

({intl})

Trunk
Firefox 3 beta3
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 years ago
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.
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

11 years ago
Created attachment 288231 [details] [diff] [review]
possible patch

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.
(Assignee)

Updated

11 years ago
Keywords: intl

Comment 3

11 years ago
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

11 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.
(Reporter)

Comment 5

11 years ago
I want to test, but how?
(Assignee)

Comment 6

11 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

11 years ago
Attachment #288231 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

11 years ago
Gijs verified that the patch has the desired effect on the tab-switching shortcuts on Windows, btw.

Updated

11 years ago
Duplicate of this bug: 414130

Comment 9

11 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

11 years ago
Blocks: 412273
Flags: blocking-firefox3?
Version: unspecified → Trunk
(Assignee)

Updated

11 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 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

11 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

11 years ago
Assignee: nobody → jruderman
Oh, right. Ignore my comment, then.
(Assignee)

Updated

11 years ago
Attachment #288231 - Flags: approval1.9?

Comment 13

11 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 on attachment 288231 [details] [diff] [review]
possible patch

a=beltzner
Attachment #288231 - Flags: approval1.9? → approval1.9+
(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

11 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

11 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

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 19

11 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: 412273
Flags: blocking-firefox3?
You need to log in before you can comment on or make changes to this bug.