Last Comment Bug 318235 - Ctrl+Enter being treated as Ctrl+J when captured by a Javascript event (download manager appears incorrectly)
: Ctrl+Enter being treated as Ctrl+J when captured by a Javascript event (downl...
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: P3 normal with 24 votes (vote)
: ---
Assigned To: Ere Maijala (slow)
: Hixie (not reading bugmail)
Mentors:
http://forum.robm.fastmail.fm/ffctrle...
: 399125 426212 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-29 21:51 PST by Rob Mueller
Modified: 2008-08-22 15:45 PDT (History)
22 users (show)
vladimir: blocking1.9.1-
vladimir: wanted1.9.1+
dbaron: blocking1.8.1-
dveditz: blocking1.8.0.1-
mozillamarcia.knous: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (399 bytes, text/html)
2006-01-04 13:59 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
fix as suggested by Ere Maijala (552 bytes, patch)
2008-07-25 12:13 PDT, Rob Arnold [:robarnold]
vladimir: review+
roc: superreview+
Details | Diff | Splinter Review

Description Rob Mueller 2005-11-29 21:51:12 PST
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 Prognathous 2005-12-05 14:08:31 PST
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.
Comment 2 Jesse Ruderman 2005-12-19 15:37:25 PST
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.
Comment 3 Daniel Veditz [:dveditz] 2006-01-04 10:31:16 PST
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
Comment 4 Jesse Ruderman 2006-01-04 12:14:39 PST
CCing bryner, hoping he can help.
Comment 5 Jesse Ruderman 2006-01-04 13:33:39 PST
Gavin thinks this is a regression from bug 197474, "Event.altKey && Event.ctrlKey not true together unlike IE".
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-01-04 13:36:34 PST
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.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-01-04 13:59:46 PST
Created attachment 207546 [details]
testcase
Comment 8 Darin Fisher 2006-06-26 13:01:30 PDT
Ere: Do you think you could take a look at this for FF2?  Thanks in advance!
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2006-07-06 11:08:03 PDT
--> beta2, needs to be done
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2006-07-19 17:18:11 PDT
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).
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-07-25 15:36:21 PDT
Minusing for blocking1.8.1 given the lack of traction and the fact that we shipped 1.8 with this bug.
Comment 12 Ere Maijala (slow) 2006-07-29 01:12:29 PDT
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.
Comment 13 Ere Maijala (slow) 2006-11-16 15:43:57 PST
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. 
Comment 14 Hugo Heden 2006-11-19 13:48:06 PST
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 Mathias Muller 2007-05-10 06:10:15 PDT
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 :-(
Comment 16 Jesse Ruderman 2007-10-08 21:01:05 PDT
*** Bug 399125 has been marked as a duplicate of this bug. ***
Comment 17 Shawn Wilsher :sdwilsh 2008-04-01 14:51:33 PDT
*** Bug 426212 has been marked as a duplicate of this bug. ***
Comment 18 Ed Holden 2008-04-02 16:32:46 PDT
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 Ed Holden 2008-06-11 14:55:39 PDT
... 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.
Comment 20 Vladimir Vukicevic [:vlad] [:vladv] 2008-07-22 13:46:15 PDT
Definitely not blocking, but will try to find someone to work on it.
Comment 21 Rob Arnold [:robarnold] 2008-07-25 12:13:24 PDT
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.
Comment 22 Vladimir Vukicevic [:vlad] [:vladv] 2008-07-25 12:29:27 PDT
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?
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-29 00:19:48 PDT
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?
Comment 24 Ere Maijala (slow) 2008-08-02 19:29:36 PDT
I'll give a shot at adding the test.
Comment 25 Ere Maijala (slow) 2008-08-02 20:45:45 PDT
Looks like a test cannot be written, because synthesizeNativeKey quite doesn't. I'll check the patch in anyway.
Comment 26 Ere Maijala (slow) 2008-08-02 23:25:36 PDT
Patch checked in.
Comment 27 Marcia Knous [:marcia - use ni] 2008-08-22 15:45:42 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.