Ctrl+Enter being treated as Ctrl+J when captured by a Javascript event (download manager appears incorrectly)

VERIFIED FIXED

Status

()

Core
Widget: Win32
P3
normal
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: Rob Mueller, Assigned: Ere Maijala (slow))

Tracking

({regression, testcase})

Trunk
x86
Windows XP
regression, testcase
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
blocking1.8.1 -
blocking1.8.0.1 -
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

If you attach a keydown event handler to a document, and then try and show a "cofirm" dialog in that event handler when someone uses the keys ctrl-enter, the download manager appears as well as the confirm dialog.

This appears to be a new bug in FF1.5, it was fine in FF1.07. Our web-site uses the Ctrl-Enter combination as a shortcut for sending an email with a confirmation (like Outlook Express) and this is now basically un-useable because of this bug.

Reproducible: Always

Steps to Reproduce:
1. Go to the above URL
2. Hold down the Ctrl key, hit the Enter key


Actual Results:  
Javascript confirm dialog appears. Download manager also appears and has focus.

Expected Results:  
Only javascript confirm dialog should appear. From the documentation, ctrl-j is the shortcut for the download manager, not ctrl-enter. Maybe some internal confusion between Enter = ctrl-j?

Had DOM Inspector + Copy URL extensions installed, but have seen this in 1.5 beta and RC candidates without any extensions.

Comment 1

12 years ago
This is very similar to Bug 268275, but since that bug was for FF 0.9, I don't think this is a dupe, at least not of 268275. Confirming bug.

Prog.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: clean-report, regression, testcase

Comment 2

12 years ago
I can repro using Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8) Gecko/20051111 Firefox/1.5.

Nominating for Firefox 1.5.0.1 because this is a regression with a lot of votes.
Flags: blocking1.8.0.1?
I don't see how this can block if no one's working on it. Find an assignee and get a patch reviewed and into the trunk, *then* ask for approval for a 1.5.0.x release
Flags: blocking1.8.1?
Flags: blocking1.8.0.1?
Flags: blocking1.8.0.1-

Comment 4

12 years ago
CCing bryner, hoping he can help.

Comment 5

12 years ago
Gavin thinks this is a regression from bug 197474, "Event.altKey && Event.ctrlKey not true together unlike IE".
I narrowed the regression range for this down to between 2004-04-16 10:00 and 2004-04-18 09:00, leading me to suspect bug 197474, which was checked in right after 1.7 branched, and never landed on 1.7/aviary. On older builds, the Page Info dialog is opened instead of the Download Manager, so it seems like Ctrl+Enter is being treated as Ctrl+J somehow (Ctrl+J used to be the shortcut for Page Info, it's now the shortcut for the DM). Strange bug.
Assignee: nobody → win32
Component: General → Widget: Win32
Product: Firefox → Core
QA Contact: general → ian
Version: unspecified → 1.8 Branch
Created attachment 207546 [details]
testcase
Summary: Download manager incorrectly appears when ctrl-enter captured by Javascript event → Ctrl+Enter being treated as Ctrl+J Download manager incorrectly appears when ctrl-enter captured by Javascript event
Summary: Ctrl+Enter being treated as Ctrl+J Download manager incorrectly appears when ctrl-enter captured by Javascript event → Ctrl+Enter being treated as Ctrl+J when captured by a Javascript event (download manager appears incorrectly)

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+

Comment 8

11 years ago
Ere: Do you think you could take a look at this for FF2?  Thanks in advance!
Assignee: win32 → emaijala
--> beta2, needs to be done
Target Milestone: --- → mozilla1.8.1beta2
This is happening because we get into nsWindow::OnChar() with a charCode of 0x0a, and then we hit this code:

  if (mIsControlDown && charCode <= 0x1A) { // Ctrl+A Ctrl+Z, see Programming Windows 3.1 page 110 for details
    // need to account for shift here.  bug 16486
    if (mIsShiftDown)
      uniChar = charCode - 1 + 'A';
    else
      uniChar = charCode - 1 + 'a';
    charCode = 0;
  }

Ending up in that code is clearly not what we want since that'll convert our 0x0a ('\n') to 0x6a ('j'), and thus we end up opening the download manager when hitting Ctrl+Enter.

What that code has to do with Ctrl+A and Ctrl+Z I don't know, hitting those key combos don't even get us into OnChar(), same goes for the code below that as well:

  else if (mIsControlDown && charCode <= 0x1F && charCode != 0x0A) {
    // Fix for 50255 - <ctrl><[> and <ctrl><]> are not being processed.
    // also fixes ctrl+\ (x1c), ctrl+^ (x1e) and ctrl+_ (x1f)
    // for some reason the keypress handler need to have the uniChar code set
    // with the addition of a upper case A not the lower case.
    uniChar = charCode - 1 + 'A';
    charCode = 0;

None of those key combinations end up in OnChar() either, and in fact removing all of that code makes us do the right thing for Ctrl+Enter, and doesn't seem to break anything that I can tell.

Ere, you know what's going on here? We could easily special case Enter in this code, but that'd be only if we want a minimal patch for the branch, but for the trunk we should actually understand this and fix it for real (and doing that might simply mean removing the above code).
Minusing for blocking1.8.1 given the lack of traction and the fact that we shipped 1.8 with this bug.
Flags: blocking1.8.1+ → blocking1.8.1-
(Assignee)

Comment 12

11 years ago
I think those should be handled in OnKeyDown instead of OnChar. Need to check why that's not happening. OnKeyDown has been heavily reworked but OnChar has been lacking attention.
(Assignee)

Comment 13

11 years ago
I suck for not digging into this before. We have a problem in OnKeyDown: it's designed as if it could handle everything before a possibly following WM_CHAR message is processed, but that's not the case if keydown processing makes something happen as the next messages could be processed before OnKeyDown is finished. I think we need to check for ctrl+enter in OnChar and leave it unprocessed there. 
Status: NEW → ASSIGNED

Comment 14

11 years ago
Just a small note: With Firefox 2.0 for Windows, when a user does Ctrl+Shift+Enter (instead of just Ctrl+Enter), everything works as would have been expected when using just Ctrl+Enter. Interesting! 

Comment 15

10 years ago
The bug is still present in 2.0.0.3.
Any shortcut declared with javascript will also called ff main function.
We are trying to provide CTRL+S shortcut to our RIA user.
I think it is very important to fix this kind of bug to allow us providing WEB 2.0 full featured application.

Note : this works with IE :-(

Updated

10 years ago
Duplicate of this bug: 399125

Updated

10 years ago
Duplicate of this bug: 426212

Comment 18

10 years ago
Just to note that this bug has persisted through Gran Paradiso.  I've been using the betas and can confirm that the Ctrl+Enter bug is still present in Firefox 3 Beta 5.

Comment 19

9 years ago
... and it's still here in Firefox 3 RC2.  As this bug has persisted for nearly three years, I'm flagging it for consideration as blocking 1.9.1.  I suppose it's minor ... unless you use a UI that requires occasional Ctrl+Enter. :(

It looks like Johnny Stenback partially diagnosed this issue in 2006, when he discovered a conditional that converts a Ctrl+Enter into a Ctrl+J, but he wasn't certain why the code nested in this conditional was being used.  Ere Maijala dug  a bit further and had a suggested fix.  Sadly I don't know C++ or I'd have a better idea what they were talking about.
Flags: blocking1.9.1?
Definitely not blocking, but will try to find someone to work on it.
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Priority: -- → P3
Target Milestone: mozilla1.8.1beta2 → ---
Created attachment 331339 [details] [diff] [review]
fix as suggested by Ere Maijala

Ere Maijala's suggested fix does fix the testcase. This feels like a hacked solution though.
Attachment #331339 - Flags: review?(vladimir)
Comment on attachment 331339 [details] [diff] [review]
fix as suggested by Ere Maijala

This is terrifying, and I am reminded of OSX key hell.

roc, is this something that we can do some automated testing for using the framework you wrote?  Does this fix even seem right?
Attachment #331339 - Flags: superreview?(roc)
Attachment #331339 - Flags: review?(vladimir)
Attachment #331339 - Flags: review+
Comment on attachment 331339 [details] [diff] [review]
fix as suggested by Ere Maijala

This is scary but no more so than the rest of this code.

I think we could write a mochitest for this in http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_keycodes.xul. Rob/Ere, want to try?
Attachment #331339 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 24

9 years ago
I'll give a shot at adding the test.
(Assignee)

Comment 25

9 years ago
Looks like a test cannot be written, because synthesizeNativeKey quite doesn't. I'll check the patch in anyway.
Version: 1.8 Branch → Trunk
(Assignee)

Comment 26

9 years ago
Patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/20080822033426 Minefield/3.1a2pre. I verified using the URL in the bug. Also verified fixed on Mac Leopard using the equivalent nightly.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.