289384 - Midas Editor: Shortcut command + left/right should go to beginning/end of line

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Derek Davenport]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050404 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050404 Firefox/1.0+

In Mac OS X, the home and end keys do not go to the beginning/end of the line as
in Windows. To do that you use command + left/right. In Firefox command +
left/right is the shortcut for going back/forward in the browser history. When
in an input field the shortcut will go to the beginning/end of the line, but in
a midas editor it still controls the history. Note that if the midas editor is
in an iframe the shortcut will control the parent window's history, not the
iframe's.

I think the shortcut should go to the beginning/end of the line.

Reproducible: Always

Steps to Reproduce:
1. Buy a Mac
2. Get Firefox
3. Find a page with a midas editor
4. Type some stuff and then hit command + back

Actual Results:  
Browser goes back in history

Expected Results:  
Goes to beginning of current line in editor

Comment 1

[Derek Davenport]

I should mention that if you don't have a back history command + left will
actually go to the beginning of the line. Likewise if you don't have a forward
history command + right will go to the end of the line.

Comment 2

[Kathleen :Brade]

Given comment 1, it sounds like the command is there and functional.  What is
happening is that the event is first offered to the browser's history which
handles it.  Probably the editor (if it has focus) should be offered the event
first.  This may be a duplicate bug.

Comment 3

[Derek Davenport]

For a while back/forward shortcuts were mapped to Command + [ and ] respectively
and so the problem was fixed. Well, the shortcuts were changed back to
left/right and so this problem is back again.

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This probably worked correctly in Fx in the period when they removed the
cmd-arrow shortcuts (between bug 254225 and bug 291516).

The shortcuts work as expected in midas in Camino 0.9a2+ but go back and forward
in midas in DP a2+; this leads me to guess the "focus" problem is in Fx, not
Editor per se (don't have a Sm build around to test). 

The only editor bug that "seems to me" it could be a possible dupe is bug
292327, and I don't see anything off-hand in Fx (that said, neither are my
specialty).

Comment 5

[Jesse Ruderman]

This is especially nasty when combined with bug 304994, "iframe with designMode=on breaks when you navigate to another loaded page and then back".

Comment 6

[Jesse Ruderman]

Confirmed with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051108 Firefox/1.5.

Comment 7

[Hiro]

WFM.
Mac OS X 10.3.9
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20061012 Minefield/3.0a1

Comment 8

[Phil Crosby]

This is still a problem. To repro, make sure you have something in your history, e.g. navigate to google.com, then http://www.mozilla.org/editor/midasdemo/, then hit command+back.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4

Comment 9

[Phil Crosby]

I noticed something strange: the rich text editor in Gmail does not have this problem. Does anyone know the workaround they used? 

Google Docs, TinyMCE and FCKEditor all have this problem.

Comment 10

[David Caldwell]

I can confirm that this is happening with TinyMCE on the Firefox 3 beta 4 (it's very frustrating).

Comment 11

[Derek Davenport]

(In reply to comment #9)
> I noticed something strange: the rich text editor in Gmail does not have this
> problem. Does anyone know the workaround they used? 
> 
> Google Docs, TinyMCE and FCKEditor all have this problem.
> 

Without looking at what Gmail does, I know that a listener that prevents the default action for the keydown event would stop it.

Comment 12

[ftolsson]

Although this bug hasn't been added to in a year, it's still a problem in Firefox 3.0.10.

Furthermore, the bug has a sibling:

while cmd+arrow SHOULD make cursor go to beginning or end of line, BUT INSTEAD it changes the page back or forward in history,

cmd+SHIFT+left arrow SHOULD go to beginning of line and select everything from here to there, BUT INSTEAD changes the page to the default start web page.

Both incarnations of the bug are obviously very frustrating, as many webpages won't remember what was entered if you move away and come back, which means editing an email using arrow keys to jump around may suddenly mean you lose everything you wrote...

Comment 13

[matthew arnison]

This bug causes data loss.

As the previous comment mentioned, if you use Cmd-Left to go to the start of the line in any text area or WSYWIG editor in a Firefox page, then instead Firefox takes you to the previous page. For many websites with text area forms, the data is lost. Pressing the forward button gives you a blank form.

This has bitten me several times.

Cmd-Left and Cmd-Right works pretty much everywhere else in Mac OS X.

Comment 14

[(no longer active)]

Attachment: WIP 1

So, what happens here is that nsXBLWindowKeyHandler::EnsureHandlers tries to bind chrome://global/content/platformHTMLBindings.xml#editor to editable documents, but xbl event handlers registered by the browser for example handle the event sooner than we have a chance to do that.

I guess we could bind that binding directly to contenteditable documents.  But with that, some tests in editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html fail.

Masayuki, are these failures benign, or do they actually indicate something being broken?  Here's a list of failures:

782 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | contenteditable="true": Alt+Backspace was prevented on bubbling phase - true should equal false
797 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | contenteditable="true": Shift+Delete was prevented on bubbling phase - true should equal false
807 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | contenteditable="true": Alt+Delete was prevented on bubbling phase - true should equal false
1202 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly HTML editor: Shift+Delete was prevented on bubbling phase - true should equal false
1212 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly HTML editor: Alt+Delete was prevented on bubbling phase - true should equal false
1592 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor: Alt+Backspace was prevented on bubbling phase - true should equal false
1607 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor: Shift+Delete was prevented on bubbling phase - true should equal false
1617 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor: Alt+Delete was prevented on bubbling phase - true should equal false
2012 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly and non-tabbable HTML editor: Shift+Delete was prevented on bubbling phase - true should equal false
2022 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly and non-tabbable HTML editor: Alt+Delete was prevented on bubbling phase - true should equal false
2402 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | HTML editor but plaintext mode: Alt+Backspace was prevented on bubbling phase - true should equal false
2417 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | HTML editor but plaintext mode: Shift+Delete was prevented on bubbling phase - true should equal false
2427 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | HTML editor but plaintext mode: Alt+Delete was prevented on bubbling phase - true should equal false
2807 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor but plaintext mode: Alt+Backspace was prevented on bubbling phase - true should equal false
2822 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor but plaintext mode: Shift+Delete was prevented on bubbling phase - true should equal false
2832 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | non-tabbable HTML editor but plaintext mode: Alt+Delete was prevented on bubbling phase - true should equal false
3227 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly HTML editor but plaintext mode: Shift+Delete was prevented on bubbling phase - true should equal false
3237 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly HTML editor but plaintext mode: Alt+Delete was prevented on bubbling phase - true should equal false
3632 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly and non-tabbable HTML editor but plaintext mode: Shift+Delete was prevented on bubbling phase - true should equal false
3642 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html | readonly and non-tabbable HTML editor but plaintext mode: Alt+Delete was prevented on bubbling phase - true should equal false

Comment 15

[(no longer active)]

Using a testcase like this:

data:text/html,<script>window.addEventListener("keypress", function(e) e.stopPropagation(), false);</script><div contenteditable=true>foo</div>

and typing in the editable area, I verified that the event listeners are added to the system group both before and after the patch.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Looks the failures are not wrong. The shortcut keys are eaten by the key bindings.

You need to change the expected results of each check() in the tests like:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/tests/test_texteditor_keyevent_handling.html?force=1

I think that if your approach is correct, we should remove the editor event handler bindings from nsXBLWindowKeyHandler::EnsureHandlers(). Probably, it causes bug 481626.

Comment 17

[(no longer active)]

So, in addition to the failures I mentioned in comment 14 (which are Mac-only), I get these failures as well in <http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_bug386782.html?force=1>:

232 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Edited contents still correct? - "<p>designModeDocument</p>" should equal "<p>EDITED designModeDocument</p>"
234 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Can we redo? - "<p>designModeDocument</p>" should equal "<p>EDITED designModeDocument</p>"
235 ERROR TEST-UNEXPECTED-FAIL | /tests/docshell/test/navigation/test_bug386782.html | Can we still edit? - "<p>TWICE designModeDocument</p>" should equal "<p>EDITED TWICE designModeDocument</p>"

This is *really* messed up.  If I change the check in <http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_bug386782.html?force=1#74> to get the textContent property instead of innerHTML, the failure message includes "EDITED designModeDocument".  If I check the innerHTML right after that, it's still "<p>designModeDocument</p>"!  If I add anything like a timeout or an alert before getting innerHTML, things work.  And I *see* the edited content on the screen!

So effectively it appears that innerHTML is not updated correctly.  Does anyone know where I should look?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have no idea but sounds similar problem to the second problem of bug 545775 comment 56. The ASUS forum is using innerHTML for getting the content.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Is the caret visible in the editor? If it's not so, nsEditor::InitializeSelection() has been never called. Then, it's same as the second problem of bug 545775 comment 56.

I guess that the binding causes some changes of focus handling.

Comment 20

[(no longer active)]

Hmm, ok, so I was completely mistaken in comment 17!  The code which is failing is actually <http://mxr.mozilla.org/mozilla-central/source/docshell/test/navigation/test_bug386782.html?force=1#92>.  Which means that the edited contents are not restored after going to another page and navigating back.  Investigating...

Comment 21

[(no longer active)]

This is driving me crazy.  Setting a -moz-binding rule on *any* node in the document causes this to fail.  For example, I tried setting the -moz-binding property on :root, html, body, p, it fails for all of them.  When I try setting the -moz-binding rule on input (which is not in the document in question, it doesn't fail).

The curious thing is that setting -moz-binding to anything reproduces the problem.  For example, I tried:

:root {
  -moz-binding: url(ehsan/iamnotxbl);
}

Comment 22

[(no longer active)]

So, when history.back() is called, the window's load event is not triggered.  I worked around this in bug 493381 by calling the next function in a setTimeout(...,0) call, but maybe that's not enough.

Is there an event for me to catch which is fired when the page is "loaded" after back's async stuff is finishes?

Comment 23

[(no longer active)]

Also, Boris, according to mxr, this is the only case where mIsDocumentLoaded is set to true: <http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2477>.  Does this mean that mIsDocumentLoaded is never set to true because the load event never fires?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is there an event for me to catch which is fired when the page is "loaded"
> after back's async stuff is finishes?

pageshow?

> Does this mean that mIsDocumentLoaded is never set to true because the load
> event never fires?

In all cases when the load event doesn't fire, that means we're restoring from bfcache, so mIsDocumentLoaded is already true...

Comment 25

[(no longer active)]

(In reply to comment #24)
> > Is there an event for me to catch which is fired when the page is "loaded"
> > after back's async stuff is finishes?
> 
> pageshow?

The handler for that should be set on the window, right?

This handler never gets called:

 82       gTest.window.addEventListener("pageshow", function() {
 83       checkStillEditable(); }, false);
 84       gTest.window.history.back();

I even tried pressing Cmd+F5, going back or forward manually, and I still couldn't get that handler to call...

Comment 26

[Boris Zbarsky [:bzbarsky]]

> The handler for that should be set on the window, right?

No, since navigating the window blows away event listeners.  It should be set on the <xul:browser> or <xul:iframe> containing the window.... alternately, the window's document could have an in-page handler and then use postMessage to tell whoever is interested or something.

Comment 27

[(no longer active)]

Attachment: Patch (v1)

(Good thing I caught this, since I was under the impression that I had submitted this patch for some reason! ;-) )

Requesting review from Masayuki on the keyhandling test parts, and from bz on the rest.

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 454180 [details] [diff] [review]
Patch (v1)

> +  var isMac = "nsILocalFileMac" in Components.interfaces;
> +  var isWin = "nsILocalFileWin" in Components.interfaces;
> +

You don't need this change, you can use kIsMac and kIsWin which were already defined above.

Otherwise, looks ok for me.

Comment 30

[Jesse Ruderman]

This patch doesn't fix the problem for me on
data:text/html,<body contenteditable>
Cmd+Left still goes back to the previous history entry if there is one.

Comment 31

[(no longer active)]

Attachment: Patch (v2)

Good catch, Jesse!  We actually needed a bit more complex selector to catch contenteditable attributes without values.  This patch fixes that, and also adjusts the keyevent test case to pass on all platforms.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 454360 [details] [diff] [review]
Patch (v2)

This sort of works... but it will apply the binding to conteneditable=" false ", for example. Is that expected?

Comment 33

[(no longer active)]

(In reply to comment #32)
> Comment on attachment 454360 [details] [diff] [review]
> Patch (v2)
> 
> This sort of works... but it will apply the binding to conteneditable=" false
> ", for example. Is that expected?

Hmm, you're right.  I guess I need to do something like [contenteditable=""],[contenteditable="true"] as the selector.  Will land with that change.

Comment 34

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d1dbbaeefe61

Comment 35

[(no longer active)]

Backed out because of crashtest assertion failures:

http://hg.mozilla.org/mozilla-central/rev/520f23ddf196

Example log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1278882300.1278882891.22815.gz

Comment 36

[(no longer active)]

Attachment: Part 2: Fix the first assertion

This patch fixes the "null node passed to nsEditor::Tag()" assertion.

Comment 37

[(no longer active)]

Attachment: Part 3: fix the 2nd assertion

The second assertion was caused because nsHTMLEditor::SelectAll tries to select the native anonymous children of the textarea directly.  This is broken, since the native anonymous children are managed by text control frame's own selection controller.  This bug was uncovered by this patch because we flushed upon setting the contenteditable attribute.

I also added a reftest to make sure that selectall works the same way even if textarea has any non-textnode children.

Comment 38

[(no longer active)]

Attachment: Part 4: Fix one more assertion

The part 3 patch introduced another assertion in <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/crashtests/574558-1.xhtml>.  This was caused because the inserthtml code couldn't handle the case where the current selection could not be deleted.  This can certainly happen, so we need to deal with it.

Comment 39

[(no longer active)]

Attachment: Part 3: fix the 2nd assertion

The old version of this patch caused the selectall command in layout/reftests/bugs/572598-1.html to fail.  The reason was that the selection was inside the input element, which means that there is an ancestor limiter set.  We need to unset it before attempting to select all the children of the body element.

Comment 40

[Kathleen :Brade]

Comment on attachment 456791 [details] [diff] [review]
Part 4: Fix one more assertion

Did you intend to rename the testCollapsed variable?

Comment 41

[(no longer active)]

(In reply to comment #40)
> Comment on attachment 456791 [details] [diff] [review]
> Part 4: Fix one more assertion
> 
> Did you intend to rename the testCollapsed variable?

No, why?

Comment 42

[(no longer active)]

I verified that a try server run with all the patches attached to this bug is green.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> I guess I need to do something like [contenteditable=""],
> [contenteditable="true"] as the selector.

That won't catch contentEditable="foopy", right?  Or contenteditable=" ", etc...

We should really file a style system bug to have some sort of way of matching on canonicalized attr values, not on the raw string.  Or something.  Otherwise anything like this will only be an approximation to what's desired.

Comment 44

[(no longer active)]

(In reply to comment #43)
> > I guess I need to do something like [contenteditable=""],
> > [contenteditable="true"] as the selector.
> 
> That won't catch contentEditable="foopy", right?  Or contenteditable=" ",
> etc...

No, but we don't want it to catch them, right?  The only two values of the contenteditable attribute which makes it actually have an effect are "true" and "".

> We should really file a style system bug to have some sort of way of matching
> on canonicalized attr values, not on the raw string.  Or something.  Otherwise
> anything like this will only be an approximation to what's desired.

Well, I don't see why such a matching operator would be needed in this case.  But yeah, I guess it would be useful elsewhere.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> The only two values of the contenteditable attribute which makes it actually
> have an effect are "true" and "".

Ah, ok.  Great!

Comment 46

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/9a1de8516b43
http://hg.mozilla.org/mozilla-central/rev/dbcb7142daa1
http://hg.mozilla.org/mozilla-central/rev/2a99ed409a38
http://hg.mozilla.org/mozilla-central/rev/1218281a2ead

Comment 47

[Daniel Kabs, reporting bugs since 2002]

Firefox 3.6.12 is still moving away from the editor page (see URL) when pressing cmd + cursor-left on Mac.

Where did this patch land?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Trunk.  You can tell from the "target milestone" field and the lack of branch fixed flags.

So this will be fixed in Firefox 4.

Comment 49

[(no longer active)]

We're going to have to back out the core fix to this bug because of bug 620906.  I will pick it up again post 2.0.

Also, Limi pointed out that the fix doesn't seem to work on Gmail, so it's something for me to keep in mind for the next round.

Comment 50

[(no longer active)]

The code part of this fix was backed out in bug 620906.

Comment 51

[David Gasperoni]

in Firefox 4.0.1 still the problem exists (see bug 341886)

Comment 52

[Sebastian Dahlgren]

This is verified on Fx 4.0.1 and Mac OS 10.6.7. Very annoying bug that really kills usability.

Comment 53

[Dietrich Ayala (:dietrich)]

Ehsan, are you going to be able to pick this up again? If not, let's find a new owner.

Comment 54

[(no longer active)]

(In reply to comment #53)
> Ehsan, are you going to be able to pick this up again? If not, let's find a
> new owner.

I'm not sure what would be a good way of fixing this while not regressing bug 620906.  I'd be happy to pick this up again in case somebody has any ideas.

Comment 55

[Walter]

Wow. It's still alive in Firefox 5...over six years from when it was first reported. Does it really take six years to fix one (seemingly) simple issue? I'll admit, I don't understand the code issues behind this bug (or how it ties into #620906 just mentioned above), but it seems like there ought to be some way to correct it, seeing as how this isn't even an issue in other browsers.

It drives me absolutely nuts when I try to compose a Gmail message in Firefox, for example, or in something else where I think I'm returning to the beginning of the row but instead find myself suddenly at the previous page, at the same time usually losing everything I had written. Because of its impact on usability, it's one of the few Firefox show-stoppers that keeps me going back to Chrome. With more people (especially students) using Mac laptops (or other Macs with no dedicated Home/End keys), this is only going to frustrate more people and drive them to other, better-behaving browsers (like Chrome).

The day this bug is finally slain will be a great day indeed.

Comment 56

[(no longer active)]

Neil, do you know of any way to give a higher priority to the accel+Left handler registered in chrome://global/content/platformHTMLBindings.xml#editor than http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#278?

Comment 57

[[e-mail]]

I'm not sure. I thought that the XBL bindings were registered on the content window and therefore got priority over the XUL bindings. Otherwise the only other idea I have is to register the XBL bindings on the contenteditable element (which was one of my potential ideas for bug 669026, as I recall...)

Comment 58

[(no longer active)]

(In reply to comment #57)
> I'm not sure. I thought that the XBL bindings were registered on the content
> window and therefore got priority over the XUL bindings. Otherwise the only
> other idea I have is to register the XBL bindings on the contenteditable
> element (which was one of my potential ideas for bug 669026, as I recall...)

Well, that doesn't work very well, as we found out the hard way in this bug.  :(

Is there any change that can be done to the browser event handler to make it aware of this XBL keybinding?

Comment 59

[[e-mail]]

(In reply to comment #58)
> (In reply to comment #57)
> > I'm not sure. I thought that the XBL bindings were registered on the content
> > window and therefore got priority over the XUL bindings. Otherwise the only
> > other idea I have is to register the XBL bindings on the contenteditable
> > element (which was one of my potential ideas for bug 669026, as I recall...)
> Well, that doesn't work very well, as we found out the hard way in this bug.
Oops, sorry for not reading the whole bug.

> Is there any change that can be done to the browser event handler to make it
> aware of this XBL keybinding?
Sorry, but I'm not even sure which event target XBL attaches this keybinding to, let alone whether its presence can be queried in JS.

Comment 60

[Cody 'codeman38' Boisclair]

Still present in Firefox 6.0.

As a test case, check out any blog that uses Disqus for its blog comments-- http://www.engadget.com/ for one example.

Comment 61

[(no longer active)]

(In reply to [e-mail] from comment #59)
> > Is there any change that can be done to the browser event handler to make it
> > aware of this XBL keybinding?
> Sorry, but I'm not even sure which event target XBL attaches this keybinding
> to, let alone whether its presence can be queried in JS.

Do you have any idea who I can ask about this, or how I can figure this out on my own?  Seems like we're getting to a dead-end here... :(

Comment 62

[Walter]

Decided to revisit the issue today, since I've been starting to use Firefox more again (now that the memory usage is much better). I've discovered a couple of things.

1. The Firefox extension "Keyconfig", which allows the user to disable the keybindings that attach Cmd + Left/Right to Back/Forward. This liberates them to work properly in previously uncooprative editors such as TinyMCE. (Tested it successfully at http://www.tinymce.com/tryit/full.php). So that was a nice discovery.

2. Implementing the Keyconfig fix STILL doesn't allow normal Mac functionality in the main Gmail text editor. Pages don't go forward or backward (though even without Keyconfig that hasn't been happened in Gmail for a long time), but the cursor still doesn't move obediently to the beginning or the end of the line. Hmm.

3. As an experiment, I plugged in a standard external PC keyboard with real Home and End keys. In the editor I'm typing this in now, those keys sent the view (not the cursor) to the top or the bottom of the text. In Gmail, it moved the view to the top or the bottom of the entire page containing the editor. Somehow, Gmail doesn't seem to be focusing those keys on the editor. When I get a chance, I'll give it a try with a full Mac keyboard with real Home/End keys.

Conclusion: while Firefox has some sort of keybinding funkiness, Gmail is apparently at fault as well. My suggestions: build Keyconfig directly into Firefox to solve most of everyone's problems, and then harass Gmail to get them to fix their editor!

Comment 63

[Alexander Staubo]

A simple workaround for developers is to override the key event and use the selection API to implement the correct behaviour:

element.addEventListener('keydown', function(event) {
  event.preventDefault();
  if (event.metaKey && !(event.shiftKey || event.altKey || event.ctrlKey)) {
    switch (event.which) {
      case 37:
        event.preventDefault();
        window.getSelection().modify("move", "backward", "lineboundary");
        break;
      case 39:
        event.preventDefault();
        window.getSelection().modify("move", "forward", "lineboundary");
        break;
    }
  }
}, false);

Ironically, I discovered this trick while browsing WebKit's unit tests. :-)

Before finding the workaround I spent half an our trying the sensible option, which is to hook the event, stop it from bubbling (to prevent the browser from navigating away from the page), and resend it to the contenteditable element using dispatchEvent(), but Firefox is completely buggy there -- it correctly fires any event listeners on the element, but does not trigger the native behaviour of the editor. Which is pretty inconsistent, since you *can* send normal key events (like characters) to an editor.

Should be possible to write an extension that automatically hooks all contenteditable elements with this workaround.

Comment 64

[miahz]

This is probably the cause (or related to) the arrow keys not working in Chat windows inside Gmail.

When a chat box is focused, simply arrowing left, right, up, or down does not work at all (along with cmd+arrow doing back/forward instead of line start/end). This makes navigating and editing in a Gmail Chat window via keyboard nearly impossible. The only way to move the cursor that i've found is using the option+arrow shortcut which jumps word to word. It drove me crazy for a while and i thought my wireless keyboard was flaking out. But it happens on both my MacBook Pro's built-in keyboard and an external USB wireless keyboard.

Not sure if this is worthy of an additional bug, but i did not find anything similar specific to just the Gmail Chat box.

I'm using 10 beta, and have been seeing this probably at least since the accelerated release cycle started. Don't remember if it's always been there, but seems like at some point in the past keyboard arrows worked properly in the Chat windows at least.

Comment 65

[Eric Case]

Still seeing this issue in Firefox 14.0.1 / Mac OS X 10.7.4.

Comment 66

[mozilla]

Same issue with osx 10.6.8/ff 17
using tinymce editor

Comment 67

[bowdowntonoone]

reported 7.5 years ago, still not fixed
this is getting ridiculous…

in the time being, a good blog post with the state of things around this issue: http://joshrhoderick.wordpress.com/2010/05/05/how-firefoxs-command-key-bug-kills-usability-on-the-mac/

Comment 68

[Kevin Carlson]

Kill editing capabilities for Sharepoint sites on my Mac.  For the first time ever, I have to now try Safari or Chrome.

Comment 69

[Alexey Kinyov]

I'm using Firefox 18.0.2 on MacOS 10.8 - I confirm the bug: in Gmail message field cmd+left and cmd+right just don't work! That's a kind of critical issue.

Comment 70

[Jesse Ruderman]

Neil, can you help Ehsan with comment 61?  This is one of my biggest daily annoyances with using Firefox, and it sucks that it's stuck.

Comment 71

[[e-mail]]

nsGlobalWindow::SetNewDocument looks to see whether it's the top-level window and if so attaches the global event handlers to the window root event handler, which is at the very end of the handler chain.

On the other hand, nsXBLService::AttachGlobalKeyHandler attaches the handlers for a <keyset> element to the document.

We might be able to tweak the event processing order to get the global event handlers to run before the <keyset> handlers. I think textarea key handlers run in the default event group, so perhaps we could make the global event handlers capturing in the system event group?

Comment 72

[(no longer active)]

Attachment: Unbitrotted version of part 1

Comment 73

[(no longer active)]

So, when I applied attachment 718825 [details] [diff] [review] locally I couldn't reproduce bug 620906 any more, so that's one piece of good news.  But the immediate thing that I noticed is that pressing space doesn't do anything...

Comment 74

[(no longer active)]

(In reply to [e-mail] from comment #71)
> nsGlobalWindow::SetNewDocument looks to see whether it's the top-level
> window and if so attaches the global event handlers to the window root event
> handler, which is at the very end of the handler chain.
> 
> On the other hand, nsXBLService::AttachGlobalKeyHandler attaches the
> handlers for a <keyset> element to the document.
> 
> We might be able to tweak the event processing order to get the global event
> handlers to run before the <keyset> handlers. I think textarea key handlers
> run in the default event group, so perhaps we could make the global event
> handlers capturing in the system event group?

This is where the keypress handler responsible to handle typing is registered: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp#122>.

Where is the code responsible for the event handling order where I can make the change in the order?  (Noob disclaimer: I know almost nothing about our event handling code...)

Comment 75

[[e-mail]]

(In reply to Ehsan Akhgari from comment #73)
> So, when I applied attachment 718825 [details] [diff] [review] locally I
> couldn't reproduce bug 620906 any more, so that's one piece of good news. 
> But the immediate thing that I noticed is that pressing space doesn't do
> anything...

If I've understood the patch correctly then you need to do two additional things:
a) tell the XBL window key handler not to bother with global editor bindings
b) remove all the editor bindings that are duplicated in browser (since they will be handled by the global browser bindings)
I'm not sure what happens to the native editor bindings though.

Comment 76

[(no longer active)]

(In reply to comment #75)
> (In reply to Ehsan Akhgari from comment #73)
> > So, when I applied attachment 718825 [details] [diff] [review] locally I
> > couldn't reproduce bug 620906 any more, so that's one piece of good news. 
> > But the immediate thing that I noticed is that pressing space doesn't do
> > anything...
> 
> If I've understood the patch correctly then you need to do two additional
> things:
> a) tell the XBL window key handler not to bother with global editor bindings
> b) remove all the editor bindings that are duplicated in browser (since they
> will be handled by the global browser bindings)
> I'm not sure what happens to the native editor bindings though.

OK, I need to figure out how to do this at some point when I have free time.  If somebody else wants to jump on this in the mean time, please feel free.

Comment 77

[Andy]

Hi all - anyone have thoughts on this?  I love Firefox - but I'm forced to use Chrome as well (which is now freezing up my MBP frequently).  Would love to use Firefox for Gmail but this is such a critical function for me (and all users), that I just can't.  

Thanks in advance

Comment 78

[krizzle]

Is it possible that Google has disabled this shortcut in their new Gmail editor because of previous issues with that key combination being mapped to the back button? Wouldn't it be easier to ask the Gmail team about it?

Comment 79

[(no longer active)]

(In reply to comment #78)
> Is it possible that Google has disabled this shortcut in their new Gmail editor
> because of previous issues with that key combination being mapped to the back
> button? Wouldn't it be easier to ask the Gmail team about it?

It's possible but we need to fix this bug first anyway.

Comment 80

[Andy]

Hi all - this seems to be an issue with every formatted text box that I come across.  Few things are more frustrating than losing changes due to this bug.  

Can anyone help?  

Note that it is in all formatted text boxes, but not unformatted text boxes (i.e. this box does not have the issue).  

Thanks in advance -

Comment 81

[matteo.barnabe]

Any news about this? 
But after all, it has been only EIGHT YEARS since it was first reported, so maybe we should just sit back and wait for another decade or so...

Comment 82

[bowdowntonoone]

ditto that Matteo
this bug is a deal breaker considering browser choice

Comment 83

[Andy]

Agree with Matteo.  Would love to see this fixed.

Comment 84

[[e-mail]]

Firefox 25 on OSX will respect DefaultKeyBinding.dict; I don't know whether that provides those particular shortcuts but you can always add them. This capability was added by bug 282097.

Comment 85

[(no longer active)]

(In reply to comment #84)
> Firefox 25 on OSX will respect DefaultKeyBinding.dict; I don't know whether
> that provides those particular shortcuts but you can always add them. This
> capability was added by bug 282097.

What is DefaultKeyBinding.dict?  I can't find it on mxr...

Comment 86

[[e-mail]]

(In reply to Ehsan Akhgari from comment #85)
> (In reply to comment #84)
> > Firefox 25 on OSX will respect DefaultKeyBinding.dict; I don't know whether
> > that provides those particular shortcuts but you can always add them. This
> > capability was added by bug 282097.
> 
> What is DefaultKeyBinding.dict?  I can't find it on mxr...

Well, if I've understood bug 282097 correctly, it's a file that OSX provides that allows you to customise your keybindings.

Comment 87

[(no longer active)]

(In reply to comment #86)
> (In reply to Ehsan Akhgari from comment #85)
> > (In reply to comment #84)
> > > Firefox 25 on OSX will respect DefaultKeyBinding.dict; I don't know whether
> > > that provides those particular shortcuts but you can always add them. This
> > > capability was added by bug 282097.
> > 
> > What is DefaultKeyBinding.dict?  I can't find it on mxr...
> 
> Well, if I've understood bug 282097 correctly, it's a file that OSX provides
> that allows you to customise your keybindings.

Oh, I thought it's something that lets us fix this bug... :(

Comment 88

[[e-mail]]

I'm sorry, I misunderstood the bug, apologies for the noise.

Comment 89

[Luther Blisset]

(In reply to [e-mail] from comment #88)
> I'm sorry, I misunderstood the bug, apologies for the noise.

Oh, great! 
So this means that after eight years nobody still has any idea on how to solve this serious bug.

Keep up the good work Firefox...

Comment 90

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Isn't this my XXX comment a cause of this bug?
http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLWindowKeyHandler.cpp#351

I feel strange that native keybindings are not preferred than our keybindings.

Comment 91

[(no longer active)]

Attachment: Patch (v1)

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #90)
> Isn't this my XXX comment a cause of this bug?
> http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/
> nsXBLWindowKeyHandler.cpp#351
> 
> I feel strange that native keybindings are not preferred than our
> keybindings.

I tried moving that call like you suggested.  One problem here is that EnsureHandlers() doesn't actually initialize isEditor in a useful way.  By directly calling IsEditor() here, I managed to get a cmd_beginLine over to DoCommandCallback, but DoCommandCallback returns early because it cannot find the window root.  The reason is that mTarget points to a document (the chrome window handler) and a bit of more surgery fixes this bug!

Let's see what the try server thinks.  https://tbpl.mozilla.org/?tree=Try&rev=e174e6002da5

Comment 92

[(no longer active)]

Also, who would be the right reviewer for this patch?

Comment 93

[(no longer active)]

Comment on attachment 8355262 [details] [diff] [review]
Patch (v1)

Picking some usual suspects.  I'll write a test for this as well in another patch.

Comment 94

[(no longer active)]

Attachment: Test case

Comment 95

[[e-mail]]

Comment on attachment 8355262 [details] [diff] [review]
Patch (v1)

> nsresult
>-nsXBLWindowKeyHandler::EnsureHandlers(bool *aIsEditor)
>+nsXBLWindowKeyHandler::EnsureHandlers()
> {
>   nsCOMPtr<Element> el = GetElement();
>   NS_ENSURE_STATE(!mWeakPtrForElement || el);
>   if (el) {
>     // We are actually a XUL <keyset>.
>-    if (aIsEditor)
>-      *aIsEditor = false;
>-
So basically what you've achieved here is that whenever we process events for a XUL <keyset> element you check the native editor key bindings first. (This is why you couldn't find the window root; events for a <keyset> are bound to the XUL document.)

Comment 96

[(no longer active)]

(In reply to comment #95)
> Comment on attachment 8355262 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8355262
> Patch (v1)
> 
> > nsresult
> >-nsXBLWindowKeyHandler::EnsureHandlers(bool *aIsEditor)
> >+nsXBLWindowKeyHandler::EnsureHandlers()
> > {
> >   nsCOMPtr<Element> el = GetElement();
> >   NS_ENSURE_STATE(!mWeakPtrForElement || el);
> >   if (el) {
> >     // We are actually a XUL <keyset>.
> >-    if (aIsEditor)
> >-      *aIsEditor = false;
> >-
> So basically what you've achieved here is that whenever we process events for a
> XUL <keyset> element you check the native editor key bindings first. (This is
> why you couldn't find the window root; events for a <keyset> are bound to the
> XUL document.)

Yes, but only if IsEditable().  I'm not sure if I understand your comment though.  Are you suggesting that this is the wrong thing to do?

Comment 97

[[e-mail]]

Current key event handling in Gecko:

1. <input>s get the inputFields binding, <textarea>s get the textAreas binding via -moz-binding in forms.css, attached to the element itself.
2. <input>s and <textarea>s also get the nsTextInputListener which handles native input and text area bindings.

Both the above only apply when the form field is focused, of course.

3. <keyset> elements in the chrome operate through a virtual binding, via an event handler attached to the XUL document (one for each <keyset>).
4. If all else fails try the browser binding, unless we think that editing is happening, in which case try the editor binding and the native editor bindings. This happens via a handler attached to the window root.

This all happens in the bubbling phase in the system event group.

You have "subverted" this by making all <keyset> elements try the native editor bindings first, as well as the window root handler.

Then again, I don't know a better way of making the native editor bindings run after the input and textareas bindings but before the keyset bindings, unless you can do a trick like the inputs and textareas do so that anything contenteditable or in design mode attaches a listener to handle native editor bindings (but even then <keyset> will still override platformHTMLBindings.xml).

Comment 98

[(no longer active)]

(In reply to comment #97)
> Current key event handling in Gecko:
> 
> 1. <input>s get the inputFields binding, <textarea>s get the textAreas binding
> via -moz-binding in forms.css, attached to the element itself.
> 2. <input>s and <textarea>s also get the nsTextInputListener which handles
> native input and text area bindings.
> 
> Both the above only apply when the form field is focused, of course.
> 
> 3. <keyset> elements in the chrome operate through a virtual binding, via an
> event handler attached to the XUL document (one for each <keyset>).
> 4. If all else fails try the browser binding, unless we think that editing is
> happening, in which case try the editor binding and the native editor bindings.
> This happens via a handler attached to the window root.
> 
> This all happens in the bubbling phase in the system event group.

Thanks for the explanation.  This was never quite obvious to me!

> You have "subverted" this by making all <keyset> elements try the native editor
> bindings first, as well as the window root handler.

I think that should actually be safe since there are a limited and well known number of event types that the native editor binding knows how to handle, and I think we want those to override the <keyset> handlers anyways, to prevent things such as this bug.

> Then again, I don't know a better way of making the native editor bindings run
> after the input and textareas bindings but before the keyset bindings, unless
> you can do a trick like the inputs and textareas do so that anything
> contenteditable or in design mode attaches a listener to handle native editor
> bindings (but even then <keyset> will still override platformHTMLBindings.xml).

That is the approach I was previously exploring, but I don't think there is any way to get it to work (not one that I can think of anyway.)

Comment 99

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8355262 [details] [diff] [review]
Patch (v1)

r=me, I think....

Comment 100

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8355295 [details] [diff] [review]
Test case

r=me

Comment 101

[[e-mail]]

Perhaps the native editor keybindings should be handled by nsEditorEventListener; this should happen before the <keyset> event handler.

Comment 102

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Indeed, sounds reasonable to me.

Comment 103

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Comment 104

[[e-mail]]

Comment on attachment 8356202 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

This looks great from a code inspection POV (I don't have a Mac so I assume it works...) apart from a couple of issues:

>+static void
>+DoCommandCallback(const char *aCommand, void *aData)
>+{
>+  nsCOMPtr<nsPIWindowRoot> root;
>+  nsIDocument* doc = static_cast<nsIDocument*>(aData);
>+  nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
>+  if (win) {
>+    root = win->GetTopWindowRoot();
>+  }
>+  if (!root) {
>+    return;
>+  }
Mix of style here, might as well early return if win is null.

>@@ -420,16 +506,32 @@ nsEditorEventListener::KeyDown(nsIDOMEvent* aKeyEvent)
> 
>   return NS_OK;
> }
> #endif
There's a little problem here; these methods are #ifdef XP_WIN, which is the only major platform that doesn't have native bindings. Now the reason you didn't notice is that neither set of native bindings bothers with keyup/down events...

Comment 105

[[e-mail]]

Comment on attachment 8356202 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

>-  if (isEditor && GetEditorKeyBindings()) {
Also, I guess EnsureHandlers doesn't need to return isEditor any more.

Comment 106

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Comment 107

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=2afcccee20b0

Comment 108

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8356325 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

>@@ -420,16 +475,35 @@ nsEditorEventListener::KeyDown(nsIDOMEvent* aKeyEvent)
> 
>   return NS_OK;
> }
> #endif
> 
> NS_IMETHODIMP
> nsEditorEventListener::KeyPress(nsIDOMEvent* aKeyEvent)
> {
>+  if (GetEditorKeyBindings()) {
>+    // First, ask the native key bindings to handle the event
>+    // XXX Note that we're not passing the keydown/keyup events to the native
>+    // key bindings, which should be OK since those events are only handled on
>+    // Windows for now, where we don't have native key bindings.
>+    WidgetKeyboardEvent* keyEvent =
>+      aKeyEvent->GetInternalNSEvent()->AsKeyboardEvent();
>+    MOZ_ASSERT(keyEvent,
>+               "DOM key event's internal event must be WidgetKeyboardEvent");
>+    nsCOMPtr<nsIDocument> doc = mEditor->GetDocument();
>+    bool handled = sNativeEditorBindings->KeyPress(*keyEvent,
>+                                                   DoCommandCallback,
>+                                                   doc);
>+    if (handled) {
>+      aKeyEvent->PreventDefault();
>+      return NS_OK;
>+    }
>+  }
>+
>   NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
> 
>   if (!mEditor->IsAcceptableInputEvent(aKeyEvent)) {
>     return NS_OK;
>   }

I think that native key bindings should be used only when mEditor->IsAcceptableInputEvent(aKeyEvent) returns true. If it returns false, the editor doesn't have focus. E.g., nsEditorEventListener::KeyPress() may be called twice for a key event. One is on focused <input> or <textarea> element. The other is on HTML editor's if the document has contentediable element. However, the latter shouldn't handle the key event since the former has focus.

Comment 109

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

And also if the key event has already been consumed by a call of preventDefault(), native key bindings shouldn't be used too. Please check defaultPrevented.

Comment 110

[(no longer active)]

You're right on both, fixing that addresses most of the try failures, but not all of them.  Most notably are the test_htmleditor_keyevent_handling.html failures.  Do you have any idea what may be causing those?

Comment 111

[(no longer active)]

Specifically, this is about calling PreventDefault() on the event when the native key bindings handle it.  When I call it, some of the tests break.  When I don't call it, some other tests break.  I'm not really sure what the expected behavior here is, given the fact that PreventDefault() worked fine when I called it from nsXBLWindowKeyHandler...

Comment 112

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Comment 113

[(no longer active)]

A try run with the patch addressing comment 108 and 109: https://tbpl.mozilla.org/?tree=Try&rev=d3de6823f1cd

Comment 114

[(no longer active)]

There are two more failing tests on try...

Comment 115

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> test_htmleditor_keyevent_handling.html

This test checks the behavior of nsEditorEventListener and all failures are "got true, expected false". That means that they do NOT detect regressions, I think. The new failures are caused by some shortcut keys become handled in nsEditorEventListener. So, you should change the last param of:

http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1#176
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1#196
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1#204
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1#192

with kIsMac.

However, I'm concerned about the priority of native keybindings in nsEditorEventListener. It *might* be better to add native keybindings after a call of mEditor->HandleKeyPressEvent(keyEvent). Although, I'm not sure.

> test_bug549262.html

This is odd. The failure is here:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug549262.html?force=1#94
This means that when the <body> element has focus, up-arrow key doesn't cause scroll up. I.e., nsEditorEventListener eats the key event or there is regression in XBL part.

> test_bug289384.html

I think that this test should be performed only on Mac because this bug is only for Mac.

Comment 116

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #115)
> > test_htmleditor_keyevent_handling.html
> 
> This test checks the behavior of nsEditorEventListener and all failures are
> "got true, expected false". That means that they do NOT detect regressions,
> I think. The new failures are caused by some shortcut keys become handled in
> nsEditorEventListener. So, you should change the last param of:
> 
> http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> test_htmleditor_keyevent_handling.html?force=1#176
> http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> test_htmleditor_keyevent_handling.html?force=1#196
> http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> test_htmleditor_keyevent_handling.html?force=1#204
> http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> test_htmleditor_keyevent_handling.html?force=1#192
> 
> with kIsMac.

Hmm yes you're right.  That indeed fixes that test.

> However, I'm concerned about the priority of native keybindings in
> nsEditorEventListener. It *might* be better to add native keybindings after
> a call of mEditor->HandleKeyPressEvent(keyEvent). Although, I'm not sure.

Hmm, better how?  Are native key bindings supposed to handle non-special keyboard events (such as simple text entry for example)?  If not, then I don't see why that would make any difference.

> > test_bug549262.html
> 
> This is odd. The failure is here:
> http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> test_bug549262.html?force=1#94
> This means that when the <body> element has focus, up-arrow key doesn't
> cause scroll up. I.e., nsEditorEventListener eats the key event or there is
> regression in XBL part.

I debugged this (sorry I thought I had mentioned this earlier!) and what happens is that IsAcceptableInputEvent() in nsEditorEventListener::KeyPress() returns false when the contenteditable element does not have the focus (which is what we want), but that skips running the native key bindings which is apparently what handles the up key, so the page remains not-scrolled.  To verify this assumption, I did a hack like this:

diff --git a/editor/libeditor/base/nsEditorEventListener.cpp b/editor/libeditor/base/nsEditorEventListener.cpp
index 3c3b26b..15e379d 100644
--- a/editor/libeditor/base/nsEditorEventListener.cpp
+++ b/editor/libeditor/base/nsEditorEventListener.cpp
@@ -477,33 +477,36 @@ nsEditorEventListener::KeyDown(nsIDOMEvent* aKeyEvent)
 }
 #endif
 
 NS_IMETHODIMP
 nsEditorEventListener::KeyPress(nsIDOMEvent* aKeyEvent)
 {
   NS_ENSURE_TRUE(mEditor, NS_ERROR_NOT_AVAILABLE);
 
-  if (!mEditor->IsAcceptableInputEvent(aKeyEvent)) {
+  uint32_t keyCode;
+  nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
+  aKeyEvent->GetKeyCode(&keyCode);
+  if (keyCode!=nsIDOMKeyEvent::DOM_VK_UP &&
+      !mEditor->IsAcceptableInputEvent(aKeyEvent)) {
     return NS_OK;
   }
 
   // DOM event handling happens in two passes, the client pass and the system
   // pass.  We do all of our processing in the system pass, to allow client
   // handlers the opportunity to cancel events and prevent typing in the editor.
   // If the client pass cancelled the event, defaultPrevented will be true
   // below.
 
   bool defaultPrevented;
   aKeyEvent->GetDefaultPrevented(&defaultPrevented);
   if (defaultPrevented) {
     return NS_OK;
   }
 
-  nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
   if (!keyEvent) {
     //non-key event passed to keypress.  bad things.
     return NS_OK;
   }
 
   if (GetEditorKeyBindings()) {
     // First, ask the native key bindings to handle the event
     // XXX Note that we're not passing the keydown/keyup events to the native

and that works around the first failure.

Now, I would expect the browser binding to be responsible for handling the up and down keys and such (step 4 in comment 97) but apparently that's not the case, which is very surprising!  Any ideas what we can do to fix this?

(Also note that the previous version of this patch which did everything in nsXBLWindowKeyHandler did not have this bug, so there is something actually broken in this new version of the patch -- I just can't put my finger on it...)

> > test_bug289384.html
> 
> I think that this test should be performed only on Mac because this bug is
> only for Mac.

OK, I can do that.

Comment 117

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #116)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #115)
> > However, I'm concerned about the priority of native keybindings in
> > nsEditorEventListener. It *might* be better to add native keybindings after
> > a call of mEditor->HandleKeyPressEvent(keyEvent). Although, I'm not sure.
> 
> Hmm, better how?  Are native key bindings supposed to handle non-special
> keyboard events (such as simple text entry for example)?  If not, then I
> don't see why that would make any difference.

For example, if a key event which may cause something in native key bidnings causes something editing HTML editor. E.g., resizing elements, indenting list items or outdenting list items.

In such case, if we prefer the native key bindings, the HTML editor specific action is never available from keyboard. Otherwise, the native key bindings are ignored.

I'm understanding as that current behavior is the latter. If so, current patch's order may cause something inaccessible regression.

> > > test_bug549262.html
> > 
> > This is odd. The failure is here:
> > http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/
> > test_bug549262.html?force=1#94
> > This means that when the <body> element has focus, up-arrow key doesn't
> > cause scroll up. I.e., nsEditorEventListener eats the key event or there is
> > regression in XBL part.
> 
> I debugged this (sorry I thought I had mentioned this earlier!) and what
> happens is that IsAcceptableInputEvent() in
> nsEditorEventListener::KeyPress() returns false when the contenteditable
> element does not have the focus (which is what we want), but that skips
> running the native key bindings which is apparently what handles the up key,
> so the page remains not-scrolled.

It's strange. Then, how the <body> is scrolled when there is no contenteditable editor? I don't understand why the path doesn't run in this case...

Comment 118

[[e-mail]]

There is a single global XBL window key handler, attached to the window root. Currently this handler looks at the focused document and if it is an editable document it only invokes both the editor bindings and any native editor bindings, otherwise it only invokes the browser bindings.

The problem here is that the window root is last in the event target chain, so that the keyset bindings in the browser chrome override the global bindings.

My suggestion was to move the editor binding processing to the editor key event listener, which is attached to the editable document, and therefore happens before the keyset bindings in the browser chrome.

It doesn't surprise me that some of the tests depended on a subtlety of event processing order, so you are probably right in that the native editor bindings need to be processed after the editor key event handler. However as the editor bindings replace the browser bindings in editable documents you also need to process them for all key events not just editor events.

Comment 119

[(no longer active)]

(In reply to [e-mail] from comment #118)
> There is a single global XBL window key handler, attached to the window
> root. Currently this handler looks at the focused document and if it is an
> editable document it only invokes both the editor bindings and any native
> editor bindings, otherwise it only invokes the browser bindings.

Hmm, actually this is a bug.  We should only invoke the editor binding if the currently focused element is an editable element, because otherwise we will incorrectly skip the browser binding for cases where there is a contenteditable element in the document which is not focused, which is exactly what this test is exercising.  I'll fix that.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #117)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #116)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #115)
> > > However, I'm concerned about the priority of native keybindings in
> > > nsEditorEventListener. It *might* be better to add native keybindings after
> > > a call of mEditor->HandleKeyPressEvent(keyEvent). Although, I'm not sure.
> > 
> > Hmm, better how?  Are native key bindings supposed to handle non-special
> > keyboard events (such as simple text entry for example)?  If not, then I
> > don't see why that would make any difference.
> 
> For example, if a key event which may cause something in native key bidnings
> causes something editing HTML editor. E.g., resizing elements, indenting
> list items or outdenting list items.
> 
> In such case, if we prefer the native key bindings, the HTML editor specific
> action is never available from keyboard. Otherwise, the native key bindings
> are ignored.
> 
> I'm understanding as that current behavior is the latter. If so, current
> patch's order may cause something inaccessible regression.

OK, I'll do that.

Comment 120

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

https://tbpl.mozilla.org/?tree=Try&rev=f531cd9b0da5

Comment 121

[[e-mail]]

Comment on attachment 8359342 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

>+  nsCOMPtr<nsIHTMLEditor> htmlEditor;
>+  if (docShell) {
Nit: Please use the if (!foo) return false; style for consistency.

>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
>+  if (focusedNode) {
>+    // If there is a focused element, make sure it's in the active editing host
>+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
>+    if (!activeEditingHost) {
>+      return false;
>+    }
>+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
>   }
> 
>   return false;
Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?

>+  nsCOMPtr<nsPIWindowRoot> root;
Nit: declare this where you initialise it.

>+  nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
Nit: Could be nsPIDOMWindow*

>+  if (!win) {
>+    return;
>+  }
[Bah, I thought we had an exception for if (...) return; not to need {}s]

>+    // First, ask the native key bindings to handle the event
This comment line looks wrong.

Comment 122

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

(In reply to [e-mail] from comment #121)
> >+  nsCOMPtr<nsIDOMElement> focusedElement;
> >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> >+  if (focusedNode) {
> >+    // If there is a focused element, make sure it's in the active editing host
> >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> >+    if (!activeEditingHost) {
> >+      return false;
> >+    }
> >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> >   }
> > 
> >   return false;
> Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?

Well, we need to ensure that the focused element in a descendant of the active editing host.

> >+  if (!win) {
> >+    return;
> >+  }
> [Bah, I thought we had an exception for if (...) return; not to need {}s]

We don't!

Comment 123

[[e-mail]]

(In reply to Ehsan Akhgari from comment #122)
> (In reply to comment #121)
> > >+  nsCOMPtr<nsIDOMElement> focusedElement;
> > >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> > >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> > >+  if (focusedNode) {
> > >+    // If there is a focused element, make sure it's in the active editing host
> > >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> > >+    if (!activeEditingHost) {
> > >+      return false;
> > >+    }
> > >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> > >   }
> > > 
> > >   return false;
> > Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?
> 
> Well, we need to ensure that the focused element in a descendant of the
> active editing host.

IsAcceptableInputEvent only checks that for mouse events. Would it make sense to expose IsAcceptableInputEvent itself, seeing as that's what you use for native editor bindings?

> > >+  if (!win) {
> > >+    return;
> > >+  }
> > [Bah, I thought we had an exception for if (...) return; not to need {}s]
> 
> We don't!

Well we should :-P

Comment 124

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8359434 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

r=me editor part and its tests.

Comment 125

[(no longer active)]

(In reply to [e-mail] from comment #123)
> (In reply to Ehsan Akhgari from comment #122)
> > (In reply to comment #121)
> > > >+  nsCOMPtr<nsIDOMElement> focusedElement;
> > > >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> > > >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> > > >+  if (focusedNode) {
> > > >+    // If there is a focused element, make sure it's in the active editing host
> > > >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> > > >+    if (!activeEditingHost) {
> > > >+      return false;
> > > >+    }
> > > >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> > > >   }
> > > > 
> > > >   return false;
> > > Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?
> > 
> > Well, we need to ensure that the focused element in a descendant of the
> > active editing host.
> 
> IsAcceptableInputEvent only checks that for mouse events. Would it make
> sense to expose IsAcceptableInputEvent itself, seeing as that's what you use
> for native editor bindings?

No, that was done for other reasons (see bug 674770.)

We need to do this check here to make sure that if the focused element (which is what receives key events) is not in an editable section of the document (for example, outside of a contenteditable) then the key event is correctly passed to the browser bindings.  IOW, this is really what fixes the first paragraph of comment 118.

Comment 126

[[e-mail]]

(In reply to Ehsan Akhgari from comment #125)
> (In reply to comment #123)
> > (In reply to Ehsan Akhgari from comment #122)
> > > (In reply to comment #121)
> > > > Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?
> > > 
> > > Well, we need to ensure that the focused element in a descendant of the
> > > active editing host.
> > 
> > IsAcceptableInputEvent only checks that for mouse events. Would it make
> > sense to expose IsAcceptableInputEvent itself, seeing as that's what you use
> > for native editor bindings?
> 
> We need to do this check

We seem to be at cross purposes here. What I'd like to see is consistency between event handling for the native and XBL editor bindings, but your check appears to resemble the check added in bug 674770 for mouse events.

I might almost go as far as to suggest that you simplify the check to e.g. [pseudocode] (win.focusedNode || win.document).hasFlag(NODE_IS_EDITABLE).

Comment 127

[(no longer active)]

(In reply to [e-mail] from comment #126)
> (In reply to Ehsan Akhgari from comment #125)
> > (In reply to comment #123)
> > > (In reply to Ehsan Akhgari from comment #122)
> > > > (In reply to comment #121)
> > > > > Why does return !!htmlEditor->GetActiveEditingHost(); not suffice here?
> > > > 
> > > > Well, we need to ensure that the focused element in a descendant of the
> > > > active editing host.
> > > 
> > > IsAcceptableInputEvent only checks that for mouse events. Would it make
> > > sense to expose IsAcceptableInputEvent itself, seeing as that's what you use
> > > for native editor bindings?
> > 
> > We need to do this check
> 
> We seem to be at cross purposes here. What I'd like to see is consistency
> between event handling for the native and XBL editor bindings, but your
> check appears to resemble the check added in bug 674770 for mouse events.

Why do you think we need any consistency between the way we handle mouse and keyboard events?  Mouse events can go to any element.  Trusted key events only go to the focused element.

> I might almost go as far as to suggest that you simplify the check to e.g.
> [pseudocode] (win.focusedNode || win.document).hasFlag(NODE_IS_EDITABLE).

Nope, that's not the correct check because it doesn't take into account the fact that we might have multiple contenteditable sections in the document (which doesn't matter here I guess) and that in designMode, the document element is not editable.

But can we take a step back here?  What problem are you trying to solve here Neil?  The check that I wrote is the kosher way to tell whether something falls inside an editable range.  Why are you trying to get me to write this check in some other way?!

Comment 128

[[e-mail]]

(In reply to Ehsan Akhgari from comment #127)
> Why do you think we need any consistency between the way we handle mouse and
> keyboard events?

I don't, which is why I'm surprised you're calling GetActiveEditingHost as it isn't on the code path that you're using for the native bindings.

> > I might almost go as far as to suggest that you simplify the check to e.g.
> > [pseudocode] (win.focusedNode || win.document).hasFlag(NODE_IS_EDITABLE).
> 
> Nope, that's not the correct check because it doesn't take into account the
> fact that we might have multiple contenteditable sections in the document
> (which doesn't matter here I guess) and that in designMode, the document
> element is not editable.

Does it matter as long as the focused one (or the document, if there is no focused node) is editable, because that's the section we're typing into?

Comment 129

[(no longer active)]

(In reply to [e-mail] from comment #128)
> (In reply to Ehsan Akhgari from comment #127)
> > Why do you think we need any consistency between the way we handle mouse and
> > keyboard events?
> 
> I don't, which is why I'm surprised you're calling GetActiveEditingHost as
> it isn't on the code path that you're using for the native bindings.

OK, then are you suggesting I should do that check unconditionally in IsAcceptableInputEvent?  I don't think we should do that, since we have other code in editor/ which prevents things that are not part of the active editing host to be edited as a result of key events, among others.  But if you insist on this, we should do that in a separate bug.  That would be a change which can risk breaking stuff and doesn't have anything to do with what I'm fixing here.

> > > I might almost go as far as to suggest that you simplify the check to e.g.
> > > [pseudocode] (win.focusedNode || win.document).hasFlag(NODE_IS_EDITABLE).
> > 
> > Nope, that's not the correct check because it doesn't take into account the
> > fact that we might have multiple contenteditable sections in the document
> > (which doesn't matter here I guess) and that in designMode, the document
> > element is not editable.
> 
> Does it matter as long as the focused one (or the document, if there is no
> focused node) is editable, because that's the section we're typing into?

It may matter in some edge case that I can't think of right now.  And it's bad coding practice.

Comment 130

[[e-mail]]

Sorry but it's a busy day for me today so I'm not around on IRC. Let's try again.

The current code runs the XBL browser bindings, unless the window is editable in some way, in which case it runs the XBL editor bindings and the native editor bindings. The window commands don't actually care which bindings get run (as long as they are run) since they do their own checks to see whether the selection is in an editable area (design mode document or contenteditable node) or not.

You are moving the native editor bindings to the editor event listener. However, you then want to filter them so that they only apply when a design mode document or editable region has focus. On a side note, bug 674770 added some code to IsAcceptableInputEvent to deal with mouse events, because the rules for mouse events are different to keyboard events.

Because you are now filtering the native editor bindings to apply only to focused editable areas, you currently aren't seeing any bindings being called for uneditable areas in partly editable documents. You plan to change the XBL window key handler to apply the browser bindings on those uneditable areas too, and only apply the editor bindings in those areas that are really editable.

However to my eye the code that you added resembles the mouse event code, since it calls GetActiveEditingHost and ContentIsDescendantOf, which the keyboard event code does not.

tl;dr I would just like the code that applies the XBL editor bindings to match the code that applies the native editor bindings.

Comment 131

[(no longer active)]

(In reply to comment #130)
> Sorry but it's a busy day for me today so I'm not around on IRC. Let's try
> again.

No worries!

> The current code runs the XBL browser bindings, unless the window is editable
> in some way, in which case it runs the XBL editor bindings and the native
> editor bindings. The window commands don't actually care which bindings get run
> (as long as they are run) since they do their own checks to see whether the
> selection is in an editable area (design mode document or contenteditable node)
> or not.
> 
> You are moving the native editor bindings to the editor event listener.
> However, you then want to filter them so that they only apply when a design
> mode document or editable region has focus. On a side note, bug 674770 added
> some code to IsAcceptableInputEvent to deal with mouse events, because the
> rules for mouse events are different to keyboard events.
> 
> Because you are now filtering the native editor bindings to apply only to
> focused editable areas, you currently aren't seeing any bindings being called
> for uneditable areas in partly editable documents.

Well this part is not true with my current patch.  I assume you're talking about the previous version.

> You plan to change the XBL
> window key handler to apply the browser bindings on those uneditable areas too,
> and only apply the editor bindings in those areas that are really editable.

Correct, that's what my latest patch does.

> However to my eye the code that you added resembles the mouse event code, since
> it calls GetActiveEditingHost and ContentIsDescendantOf, which the keyboard
> event code does not.

Right.  That resemblance is only coincidental though, since this is the generic check that we perform anywhere that we need to decide whether we should act on something.  So let's not talk about mouse events any more since I think that's what was confusing me all along!

> tl;dr I would just like the code that applies the XBL editor bindings to match
> the code that applies the native editor bindings.

OK, I'll add a similar check there.

Comment 132

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Comment 133

[[e-mail]]

(In reply to Ehsan Akhgari from comment #131)
> (In reply to comment #130)
> > However to my eye the code that you added resembles the mouse event code, since
> > it calls GetActiveEditingHost and ContentIsDescendantOf, which the keyboard
> > event code does not.
> 
> That resemblance is only coincidental though, since this is the
> generic check that we perform anywhere that we need to decide whether we
> should act on something.

Then why does IsAcceptableInputEvent only do that check for mouse events?

Comment 134

[(no longer active)]

(In reply to comment #133)
> (In reply to Ehsan Akhgari from comment #131)
> > (In reply to comment #130)
> > > However to my eye the code that you added resembles the mouse event code, since
> > > it calls GetActiveEditingHost and ContentIsDescendantOf, which the keyboard
> > > event code does not.
> > 
> > That resemblance is only coincidental though, since this is the
> > generic check that we perform anywhere that we need to decide whether we
> > should act on something.
> 
> Then why does IsAcceptableInputEvent only do that check for mouse events?

Like I said in comment 127, that was done in bug 674770 for other reasons...  Now we're officially circling back! ;-)

Comment 135

[[e-mail]]

Comment on attachment 8359950 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

>+  // Only return true if the target of the event is a desendant of the active
>+  // editing host in order to match the similar decision made in
>+  // nsXBLWindowKeyHandler.
This needs a comment explaining that IsAcceptableInputEvent currently doesn't check the active editing host for key events and this happens to work so far because the editor checks it later anyway, and that check wouldn't otherwise get applied here.

Comment 136

[(no longer active)]

(In reply to comment #135)
> Comment on attachment 8359950 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8359950
> Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil
> 
> >+  // Only return true if the target of the event is a desendant of the active
> >+  // editing host in order to match the similar decision made in
> >+  // nsXBLWindowKeyHandler.
> This needs a comment explaining that IsAcceptableInputEvent currently doesn't
> check the active editing host for key events and this happens to work so far
> because the editor checks it later anyway, and that check wouldn't otherwise
> get applied here.

Sure, will do when landing.

Comment 137

[[e-mail]]

Comment on attachment 8359950 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Sorry for the delay, I had problems updating my build.

>+  nsCOMPtr<nsIEditor> editor;
>+  docShell->GetEditor(getter_AddRefs(editor));
>+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(editor);
>+  if (!htmlEditor) {
>+    return false;
>+  }
This bit turns out to be vitally important to stop the editor bindings applying to textfields.
As it happens our textareas use the same native bindings as our contenteditable.

>+  nsCOMPtr<nsIDOMElement> focusedElement;
>+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
>+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
>+  if (focusedNode) {
>+    // If there is a focused element, make sure it's in the active editing host
>+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
>+    if (!activeEditingHost) {
>+      return false;
>+    }
>+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
GetActiveEditingHost computes the editing host from the selection.
And InitializeSelection limits the selection to the focused node's editing host.
And the focused node's editing host contains the focused node by definition.
So this check can never fail.
Some sort of paranoia comment wouldn't go amiss though.

>+</html>
>+
Nit: unnecessary blank line at the end of your new test.

Comment 138

[(no longer active)]

(In reply to [e-mail] from comment #137)
> Comment on attachment 8359950 [details] [diff] [review]
> Part 1: Run the native key binding handlers from nsEditorEventListener;
> r=Neil
> 
> Sorry for the delay, I had problems updating my build.
> 
> >+  nsCOMPtr<nsIEditor> editor;
> >+  docShell->GetEditor(getter_AddRefs(editor));
> >+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(editor);
> >+  if (!htmlEditor) {
> >+    return false;
> >+  }
> This bit turns out to be vitally important to stop the editor bindings
> applying to textfields.
> As it happens our textareas use the same native bindings as our
> contenteditable.

Yes, I think all of what you said is expected.  (It seems you're just stating facts here and not commenting about something I should change in my patch?)

> >+  nsCOMPtr<nsIDOMElement> focusedElement;
> >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> >+  if (focusedNode) {
> >+    // If there is a focused element, make sure it's in the active editing host
> >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> >+    if (!activeEditingHost) {
> >+      return false;
> >+    }
> >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> GetActiveEditingHost computes the editing host from the selection.
> And InitializeSelection limits the selection to the focused node's editing
> host.
> And the focused node's editing host contains the focused node by definition.
> So this check can never fail.
> Some sort of paranoia comment wouldn't go amiss though.

This is not paranoia, the page can modify the selection to something which doesn't have focus.  Example:

 document.querySelector("#elem1").focus();
 window.getSelection().collapse(document.querySelector("#elem2"), 0);

I will add a comment to make it clearer, but the GetActiveEditingHost check here is indeed required.

Comment 139

[(no longer active)]

Attachment: Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Comment 140

[[e-mail]]

(In reply to Ehsan Akhgari from comment #138)
> (In reply to comment #137)
> > (From update of attachment 8359950 [details] [diff] [review])
> > >+  nsCOMPtr<nsIEditor> editor;
> > >+  docShell->GetEditor(getter_AddRefs(editor));
> > >+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(editor);
> > >+  if (!htmlEditor) {
> > >+    return false;
> > >+  }
> > This bit turns out to be vitally important to stop the editor bindings
> > applying to textfields.
> > As it happens our textareas use the same native bindings as our
> > contenteditable.
> 
> Yes, I think all of what you said is expected.  (It seems you're just stating
> facts here and not commenting about something I should change in my patch?)

I wanted to be sure that these changes didn't break form fields in documents with editable areas. Now I can't remember whether my methodology is correct, but I'll have to wait for my build to finish before I can test it again...

> > >+  nsCOMPtr<nsIDOMElement> focusedElement;
> > >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> > >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> > >+  if (focusedNode) {
> > >+    // If there is a focused element, make sure it's in the active editing host
> > >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> > >+    if (!activeEditingHost) {
> > >+      return false;
> > >+    }
> > >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> > GetActiveEditingHost computes the editing host from the selection.
> > And InitializeSelection limits the selection to the focused node's editing
> > host.
> > And the focused node's editing host contains the focused node by definition.
> > So this check can never fail.
> > Some sort of paranoia comment wouldn't go amiss though.
> 
> This is not paranoia, the page can modify the selection to something which
> doesn't have focus.  Example:
> 
>  document.querySelector("#elem1").focus();
>  window.getSelection().collapse(document.querySelector("#elem2"), 0);

Did you actually try this? When I tried this it returned NS_ERROR_FAILURE.

Comment 141

[(no longer active)]

Attachment: Proof of concept for comment 138

(In reply to [e-mail] from comment #140)
> (In reply to Ehsan Akhgari from comment #138)
> > (In reply to comment #137)
> > > (From update of attachment 8359950 [details] [diff] [review])
> > > >+  nsCOMPtr<nsIEditor> editor;
> > > >+  docShell->GetEditor(getter_AddRefs(editor));
> > > >+  nsCOMPtr<nsIHTMLEditor> htmlEditor = do_QueryInterface(editor);
> > > >+  if (!htmlEditor) {
> > > >+    return false;
> > > >+  }
> > > This bit turns out to be vitally important to stop the editor bindings
> > > applying to textfields.
> > > As it happens our textareas use the same native bindings as our
> > > contenteditable.
> > 
> > Yes, I think all of what you said is expected.  (It seems you're just stating
> > facts here and not commenting about something I should change in my patch?)
> 
> I wanted to be sure that these changes didn't break form fields in documents
> with editable areas. Now I can't remember whether my methodology is correct,
> but I'll have to wait for my build to finish before I can test it again...

OK, let me know if you hit any other problems.

> > > >+  nsCOMPtr<nsIDOMElement> focusedElement;
> > > >+  fm->GetFocusedElement(getter_AddRefs(focusedElement));
> > > >+  nsCOMPtr<nsINode> focusedNode = do_QueryInterface(focusedElement);
> > > >+  if (focusedNode) {
> > > >+    // If there is a focused element, make sure it's in the active editing host
> > > >+    nsCOMPtr<Element> activeEditingHost = htmlEditor->GetActiveEditingHost();
> > > >+    if (!activeEditingHost) {
> > > >+      return false;
> > > >+    }
> > > >+    return nsContentUtils::ContentIsDescendantOf(focusedNode, activeEditingHost);
> > > GetActiveEditingHost computes the editing host from the selection.
> > > And InitializeSelection limits the selection to the focused node's editing
> > > host.
> > > And the focused node's editing host contains the focused node by definition.
> > > So this check can never fail.
> > > Some sort of paranoia comment wouldn't go amiss though.
> > 
> > This is not paranoia, the page can modify the selection to something which
> > doesn't have focus.  Example:
> > 
> >  document.querySelector("#elem1").focus();
> >  window.getSelection().collapse(document.querySelector("#elem2"), 0);
> 
> Did you actually try this? When I tried this it returned NS_ERROR_FAILURE.

Yes of course, there are many many ways in which this can happen, this test case is one such way.  The whole GetActiveEditingHost() machinery was added to deal with these kinds of issues.

Comment 142

[[e-mail]]

Comment on attachment 8362751 [details]
Proof of concept for comment 138

My breakpoint on nsEditorEventListener::KeyPress doesn't get hit in this case.

Comment 143

[(no longer active)]

Attachment: Proof of concept for comment 138

OK, try this test case.  Set a breakpoint on nsEditorEventListener::KeyPress(), load the test case, press tab to focus the div, and then press Cmd+Left.

Comment 144

[[e-mail]]

Comment on attachment 8363038 [details]
Proof of concept for comment 138

This one had me confused, until I realised that it's not reentrant (which makes it harder to debug, but now that I know, that shouldn't be a problem).

Comment 145

[(no longer active)]

(In reply to [e-mail] from comment #144)
> Comment on attachment 8363038 [details]
> Proof of concept for comment 138
> 
> This one had me confused, until I realised that it's not reentrant (which
> makes it harder to debug, but now that I know, that shouldn't be a problem).

I'm not sure if I can parse this.  Did you manage to get to the code in question under the debugger in this test?  (Note that the ContentIsDescendantOf check doesn't fail with this case, the check on editingHost's nullness fails -- but I can come up with another test case to make ContentIsDescendantOf fail with some more effort.  That being said, hopefully this is enough to prove that the check is not entirely paranoid.)

Comment 146

[[e-mail]]

No, I mean that the test only triggers once per load, and it's highly sensitive to focus changes, so I can't just switch to the debugger and back, instead I have to have everything set up just right in the debugger in advance so that it will break on the specific key press in question.

Comment 147

[[e-mail]]

Comment on attachment 8362726 [details] [diff] [review]
Part 1: Run the native key binding handlers from nsEditorEventListener; r=Neil

Wow, that test case is egregious! I managed to "better" it by persuading the selection to span between disparate contenteditable areas. You can't delete any uneditable elements that way, but I'm not sure whether such a selection should count as editable or not (I even managed to arrange for it to pass both the IsAcceptableInputEvent and ShouldHandleNativeKeyBindings tests).

My worries about form fields were baseless as they are considered to be uneditable by virtue of having an independent selection. (The check is buried in GetActiveEditingHost so it wasn't clear before.)

As mentioned on IRC I discovered nsHTMLEditorEventListener which I feel would have resulted in a minor code simplification (but not enough to deny review).

I was unable to find a case where the XBLWindowKeyHandler code tried to use the editor bindings when the editor event listener is ignoring the key event.

Comment 148

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30fefae6278f

Comment 149

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30fefae6278f

Comment 150

[Sören Hentzschel]

Thanks for the fix! Can the fix be uplifted to Aurora? There are so many blogs, cms, forums and so on using WYSIWYG editors such as CKEditor and it's so frustrating to lose all your written text just because you want to jump to the beginning of the line.

Comment 151

[(no longer active)]

(In reply to Sören Hentzschel from comment #150)
> Thanks for the fix! Can the fix be uplifted to Aurora? There are so many
> blogs, cms, forums and so on using WYSIWYG editors such as CKEditor and it's
> so frustrating to lose all your written text just because you want to jump
> to the beginning of the line.

No, this patch is fairly risky, so I would like it to ride the trains as it normally would.  I do sympathize with the pain that this causes you but please let's be patient to make sure that the fix sticks.  If everything goes well, this will be fixed in Firefox 29.

Comment 153

[Maurizio De Magnis]

Given this bug has been labeled as "resolved fixed" what are the steps needed to make it land on Nightly?

Comment 154

[Andrew McCreight [:mccr8]]

Things that land on mozilla-central automatically appear in the Nightly that day or they next day.  It landed on mozilla-central 4 days ago, so it should be in Nightly by now.

Comment 155

[Sören Hentzschel]

Is Thunderbird affected by this change? Because Cmd + left does not work in Thunderbird Daily anymore since 2014-01-24 (it works in new mails but not in replies). I tested different mails and different mail accounts, no problems before this date.

Comment 156

[Maurizio De Magnis]

I've just tried this in Firefox Nightly 29.0a1 and it still doesn't work.
1) Open mail.google.com
2) login if needed
3) click the "compose" button
4) click in the body part of the composer
5) write "hello world, how are you?" (your cursors should now be placed to the right of the sentence)
6) press cmd + left arrow

expected behavior: the cursor is placed to the beginning of the sentence (to the left of the "h" character)

resulting behavior: the cursor doesn't move

additional info: alt + left/right arrow actually moves the cursor to the beginning of the preceding/following word.

Comment 157

[Maurizio De Magnis]

(In reply to Maurizio De Magnis from comment #156)
> I've just tried this in Firefox Nightly 29.0a1 and it still doesn't work.
> 1) Open mail.google.com
> 2) login if needed
> 3) click the "compose" button
> 4) click in the body part of the composer
> 5) write "hello world, how are you?" (your cursors should now be placed to
> the right of the sentence)
> 6) press cmd + left arrow
> 
> expected behavior: the cursor is placed to the beginning of the sentence (to
> the left of the "h" character)
> 
> resulting behavior: the cursor doesn't move
> 
> additional info: alt + left/right arrow actually moves the cursor to the
> beginning of the preceding/following word.

Indeed, the cmd + left/right arrow behavior works correctly in the link attached to this bug (http://www-archive.mozilla.org/editor/midasdemo/). Is the bug I'm describing totally uncorrelated with this one? In that case, I'm sorry for the noise.

Comment 158

[[e-mail]]

(In reply to Maurizio De Magnis from comment #156)
> resulting behavior: the cursor doesn't move

As per comment #78, it looks like those keys were completely disabled by Google to prevent the dataloss behaviour. This is being tracked in bug 341886.

Comment 159

[Maurizio De Magnis]

(In reply to [e-mail] from comment #158)
> (In reply to Maurizio De Magnis from comment #156)
> > resulting behavior: the cursor doesn't move
> 
> As per comment #78, it looks like those keys were completely disabled by
> Google to prevent the dataloss behaviour. This is being tracked in bug
> 341886.

Thanks, I missed that comment :-)

Comment 160

[svansintjan]

This claims to have been fixed, but I can definitely still reproduce it.

For example, using FF 37.0.2, on Mac OS X Yosemite. 
1. go to https://bugzilla.mozilla.org/show_bug.cgi?id=289384
2. click to add a comment
3. Type "hello world"
4. Press cmd + ←. 
5. Cursor doesn't move to start of the line, as it does in all other OS X applications.
6. Press cmd + →.
7. Cursor doesn't move to the end of the line, as it does in all other OS X applications.

This is the one blocker for me using Firefox, as I regularly edit pretty lengthy pieces of text.

It also doesn't work in the Midas editor (http://www-archive.mozilla.org/editor/midasdemo/) listed above.

Is it a different bug? Should I report a new bug?

Comment 161

[Jesse Ruderman]

svansintjan, that sounds like a different bug, so you should report a new one. (fwiw, both of those work fine for me, so there might be a missing step 0; https://support.mozilla.org/en-US/kb/troubleshoot-and-diagnose-firefox-problems might help you figure out what it is.)

Comment 162

[svansintjan]

(In reply to Jesse Ruderman from comment #161)
> svansintjan, that sounds like a different bug, so you should report a new
> one. (fwiw, both of those work fine for me, so there might be a missing step
> 0;
> https://support.mozilla.org/en-US/kb/troubleshoot-and-diagnose-firefox-
> problems might help you figure out what it is.)

Yep, looks like it was. Thanks for the link. Reinstalling Firefox didn't seem to fix this, but refreshing it did.