57078 - linux keybindings mimicking windows

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[sujay]

using 10/17 build of netscape

1) launch netscape
2) launch composer
3) enter some text
4) hit ^A

it should take you to home, but it doesn't. instead it selects all.

5) hit ^K

it should delete to the end of the line. but it doesn't. instead it
invokes the spellchecker!

Linux platform only.

Comment 1

[Akkana Peck]

Apparently the order in which keybindings are evaluated changed recently: now xp
bindings take precedence over platform bindings.  I mentioned this to Saari
yesterday, and he thought he might know what changed.

This is a regression which will prevent Unix users from using their customary
editing keys.

Comment 2

[sairuh (rarely reading bugmail)]

hm...now that i think of it...would this be perchance a dup of bug 55408? if
not, then dependent on that bug?

Comment 3

[Akkana Peck]

'Fraid not, though that bug would hide this one for people who used the default.

Comment 4

[Matthew T (active 1999-2002)]

Normally I don't like prefs, but in this case I think we do need one -- a 
choice between Windows-/Mac-style keybindings (so the hordes migrating from 
other OSes can continue using the keybindings they are accustomed to) and the 
traditional emacs keybindings (so Unix users can continue using the keybindings 
they are accustomed to).

Comment 5

[timeless]

Um, that's a packaging nightmare.  And if you do it, you'll really need to be 
fair and let the users of os Y select to use any binding available.  [Some poor 
mac user trapped at a windows box...]

The way i see this happening is by having an empty keybinding file and then 
having an rdf overlay the correct keybindings.  Most likely this will be done 
by marking keybindings as a separate package.

hyatt, ben, dveditz: comments?

Comment 6

[Akkana Peck]

We basically already have this, by switching the pref to use alt keybindings.

What we don't have is a way to conditionally include sets of xbl bindings, to
get around the problem that there are conflicts when the access key is ctrl.  We
need that to un-mess-up the Unix binding situation.

Hyatt told me a while ago shipped that the chrome and resource scheme was being
revised and that it would be possible for users to select different xbl files,
something like the old mechanism that stopped working where an XBL file for
behavior could be specified in userChrome.css.  Hyatt, what's the status on
that, and is there a bug?  Will it eventually be possible for us to switch
between two different platformHTMLBindings.xbl files based on a pref or
userChrome setting or something?

Perhaps a temporary solution would be to put all key bindings in one separate
jar file (or keep them out of jars altogether) which would make it easy to offer
replacement files (users would still have to copy a file to change their
bindings, but it's better than unzipping/editing/rezipping).

Comment 7

[Matthew T (active 1999-2002)]

The pref is bug 49909.

Comment 8

[Akkana Peck]

Yes, that's the bug for adding a pref UI for the pref that switches the accel
key.  Changing individual key bindings is covered by bug 18508 (backend) and bug
57805 (make a UI for key bindings).

This bug is probably a dup of one of those, not sure which since it talks about
several issues.

Comment 9

[sairuh (rarely reading bugmail)]

*** Bug 65538 has been marked as a duplicate of this bug. ***

Comment 10

[Boris Zbarsky [:bzbarsky]]

I think the biggest issue here is consistency.  In textareas and form inputs,
C-a will go to beginning of line, C-h will delete a character.  In mailnews
compose and editor, C-a will select all and C-h will open browser history!

Adding dependencies on the C-a and C-h bugs.

Comment 11

[Akkana Peck]

Are people still seeing this?  I thought about poking around in the XUL key
code, tried changing my accel key back to ctrl and didn't see this problem --
ctrl-A always went to beginning of line, never did select-all.  If this is still
happening, when and where is it seen?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Still an issue for me in mailnews composition with a 2001-02-12 build

Comment 13

[sairuh (rarely reading bugmail)]

*** Bug 69230 has been marked as a duplicate of this bug. ***

Comment 14

[Akkana Peck]

Sure enough, I see it too now, in the editor and mail compose windows, but not
in text controls.  Looks like there's a problem with the way the XBL bindings
are getting registered only in toplevel windows.  I'll see if I can find out more.

Comment 15

[Akkana Peck]

This was previously marked as assigned, but it got changed when I made my last
comment.  Changing it back.

Meanwhile, I've been poking around reordering various lines in
nsXBLWindowHandler.cpp, but haven't had any luck with fixing this.

Comment 16

[Akkana Peck]

On Hyatt's suggestion, I looked at the XUL bindings.  Turns out that when we
switched over to XBL bindings for the editor and browser windows, the XUL
bindings files were never removed from the build, or the tree.

We should remove them -- both browserBindings.xul and editorBindings.xul.  Any
others?  We should remove them from the source tree entirely so they don't
mistakenly get added back in somewhere along the line (which is probably what
happened here).

I'm taking this because someone needs to do it.

Comment 17

[Akkana Peck]

Attachment: Initial diff (breaks some bindings)

Comment 18

[Akkana Peck]

Turns out there are still a lot of XUL key bindings which should be done in XBL.
I've attached a patch which removes all of them.  Now some things don't work:
1. The cut/copy/paste bindings don't show up in the menus any more (even though
they still work correctly).
2. "New blank page" and "New Navigator Window" no longer work -- I guess nobody
ever bothered to add XBL bindings for those functions?

Adding dr -- I think he had something to do with the switch from XUL to XBL --
and Ben for the xul bindings and the menu issue.  Can someone help with getting
the key binding legends in the menus fixed and with deciding where the accel-N
and accel-shift-N XBL bindings should live?

Marking moz 0.9 because users shouldn't have to put up with this any longer.

Comment 19

[Peter "jag" Annema]

The key binding legends issue in menuitems is an interesting one.

The problem is that a <menu(item)/> refers to a <key/> to get the
modifier(s)+key information for the legend. I don't think there's currently a
way to get that information from xbl, so as a quick fix you could leave the
<key/>s around, just remove the oncommand attribute.

Comment 20

[Akkana Peck]

Talked to Hyatt: there is currently no way to update the menus to show what the
current XBL bindings are.  Filed bug 71779 to cover that issue.  Meanwhile, jag
is probably right that we'll need to keep the xul files there to show bindings
in the menus, but remove the part that actually registers a key binding.  Yuck.

Comment 21

[Akkana Peck]

I have a simpler patch, which keeps the <key> nodes but removes their observes
and oncommand attributes so they won't actually fire.

I'd like to get this in for 0.8.1, then address the harder problems and code
cleanup later.

Hyatt, any chance you could look at this and give an sr?  Kathy, could you
review it?  And if jag and/or blake would be willing to look it over as well, it
would help my confidence in this a lot.

Comment 22

[Akkana Peck]

Oh, great timing.  Bugzilla isn't accepting patches for some reason -- the
operation times out.  Until bugzilla is fixed, I've uploaded the latest
(smaller/safer) patch to: http://www.shallowsky.com/bugs/bindings.diff

Comment 23

[John Morrison]

(Actually, there is a mozilla bug in recent builds that prevented the upload of 
patches to bugzilla (if they above a certain size). A fix is forthcoming.)
bug 71346

Comment 24

[David Hyatt]

sr=hyatt.

Comment 25

[rubydoo123]

moving to mozilla0.9

Comment 26

[Akkana Peck]

Attachment: Smaller, simpler patch for 0.8.1

Comment 27

[Akkana Peck]

Attachment: Kathy Brade's cleanup of XBL files

Comment 28

[Akkana Peck]

Back to 0.8.1 (with Beth's okay), since we have a reviewed and sr'ed patch ...
we'll be doing a bit more testing while waiting for a go-ahead to check in.

Comment 29

[Christopher Blizzard (:blizzard)]

Assuming that this is low risk enough for 0.8.1 I'm fine with getting it in (
please! )

Is this something that's going to hurt the mail/news landing?  If so can it wait
until after we actually branch?

Comment 30

[kinmoz]

I added the 03/15/01/ 17:12 and 03/15/01/17:13 patches to my Win32 source tree 
and noticed the following:

Good news:

  Arrow keys and it's variants still work in widgets and Composer.
  Cut/Copy/Paste/Undo/Redo all work in widgets and Composer.
  No more double undos with a single Ctrl-Z anymore.

Bad news:

  SelectAll (Ctrl-A) is busted
  Ctrl-Shift-Home/End are busted in widgets
  PageUp/Down (cmd_movePageUp/Down) and it's variants don't work in widgets

Comment 31

[Kathleen :Brade]

also note: in my cleanup (see attachment), the scrollPageUp and scrollPageDown 
commands should be *move*PageUp and *move*PageDown in the Windows (and presumably 
Linux?) version of platformHTMLBindings.xml.

also, I think that pageup, pagedown isn't handled for textareas and it should be 
(subject of another bug)

Comment 32

[Akkana Peck]

Page up/down in textareas was broken before: bug 65124 covers that -- but we
should fix that with this checkin as long as we're fixing basic keybindings.

SelectAll: turns out there was no selectAll key binding in XBL before (!).
We should be able to add a selectAll binding for each set of bindings in
htmlBindings.xml, then let the unix/platformHTMLBindings.xml override that. 
Note the *should*: it turns out that platform bindings are not currently
overriding xp bindings.  (Full circle back to the original bug!)

So, to fix this on Unix in a way that doesn't remove the binding on windows/mac,
we have two options:
(1) add selectAll bindings to the mac and windows platformHTMLBindings.xml.
(2) add them to the XP bindings file, and fix the problem in XBL that is not
letting platform bindings override xp bindings.

We should fix the problem anyway, of course, but it may be too late to do that
for 0.8.1.  In other cases where two platforms use a binding and the third puts
something else on that key, we have taken approach (1), put the binding in the
platformHTMLBindings for the two platforms and kept it out of the xp bindings. 
(In fact, Kathy's cleanup moves more such bindings out of xp and into duplicate
platform bindings.)  So if we want a fix for 0.8.1, that's what I suggest we do,
consistent with our approach on other bindings.  It's four extra lines in
mac/platformHTMLBindings.xml and win/platformHTMLBindings.xml, and I'll attach a
new diff if anyone's interested.

Comments?

Comment 33

[Akkana Peck]

*** Bug 59404 has been marked as a duplicate of this bug. ***

Comment 34

[Akkana Peck]

Attachment: turn on selectAll for mac and windows

Comment 35

[Akkana Peck]

I've attached a small patch to turn selectAll back on for mac and windows. 
Apply either instead of Kathy's cleanup diff, or on top of it (on top of it will
have different line numbers and might need to be applied by hand).  

Kin's report of ctrl-shift-home being broken in widgets on windows worries me,
though, because those bindings are there and look correct to me.  Maybe it's
another case like bug 65124, paging bindings mysteriously not working.
I did fixed a missing shift in the mac binding for shift-home.

Comment 36

[Akkana Peck]

With help from Hyatt, I determined that the problem was actually that both the
platform and XP bindings were firing (and in the case of selectAll and
beginLine, you don't see the beginLine if there's a selectAll after it).  This
is because nsXBLPrototypeHandler::ExecuteHandler calls PreventDefault, but it
doesn't check GetPreventDefault before firing.  This patch cures that problem
and should make things much nicer for platform and user bindings which override
XP bindings.  (We still need the XUL diffs, though -- XUL bindings will still
win over XBL.)

Index: nsXBLPrototypeHandler.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp,v
retrieving revision 1.17
diff -u -r1.17 nsXBLPrototypeHandler.cpp
--- nsXBLPrototypeHandler.cpp   2001/03/13 01:55:35     1.17
+++ nsXBLPrototypeHandler.cpp   2001/03/17 01:37:00
@@ -189,6 +189,14 @@
   if (!mHandlerElement)
     return NS_ERROR_FAILURE;
 
+  nsCOMPtr<nsIDOMNSUIEvent> uievent = do_QueryInterface(aEvent);
+  if (uievent) {
+    PRBool preventDefault;
+    uievent->GetPreventDefault(&preventDefault);
+    if (preventDefault)
+      return NS_OK;
+  }
+
   // This is a special-case optimization to make command handling fast.
   // It isn't really a part of XBL, but it helps speed things up.
   nsAutoString command;

This came from hyatt and so has sr, but needs an r= from someone else.

Comment 37

[Kathleen :Brade]

r=brade

Comment 38

[Akkana Peck]

Giving up on 0.8.1, this just isn't getting much testing.

Comment 39

[Akkana Peck]

I checked in my part of this a week ago; Kathy checked in her part last week, so
I'm marking this fixed.

Comment 40

[sujay]

Ctrl-A does take you to Home

but

Ctrl-K still brings up the spellchecker...it should delete to the
end of the line right ?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Is the spellchecker in mozilla builds?  Or is this a bugscape issue now?

Comment 42

[timeless]

spellchecker ui is in mozilla, netscape comm builds have an engine. the 
question is does our ui happen reguardless of the presence of an engine.

Comment 43

[Akkana Peck]

Funny, for me in today's build, if I set accel=ctrl, ctrl-K works correctly
(deletes to end of line) but ctrl-H now brings up the history window instead of
deleting backward.

Comment 44

[Akkana Peck]

A few specific keybindings have regressed.  Sujay has filed bug 77976 for those
keybindings, rather than morphing this more general bug.  Re-closing this one,
and we'll look at the ^K/^H issues in that bug.

Comment 45

[sujay]

verified.