257405 - Use gtk2 native keybindings for input and textarea

Status: VERIFIED FIXED

Description

[Brian Ryner (not reading)]

There's been a longstanding problem with text keybindings on unix, where certain
shortcuts (ctrl+W, for example) conflict with window-level shortcut keys, such
that those shortcuts don't function when a text input has focus.  As of gtk2,
emacs keybindings are no longer the default (gtk has configurable keybindings).
 Some users would prefer to use the non-emacs/windows-like keybindings, thereby
freeing up the window-level shortcut keys to work no matter where focus is. 
Other users seem to really like the emacs bindings.

The best solution, I think, is for us to honor gtk2's keybinding configuration.
 Users who want to use emacs keybindings can configure that via gtk/gnome
preferences, and we'll behave the same way as other gtk/gnome apps.  I've done
an implementation of this, it seems to work, but probably needs a little more
testing.

Comment 1

[Brian Ryner (not reading)]

Created attachment 157388 [details] [diff] [review]
patch

I should probably know a better way to do it, but the best I could come up with
for diffing a new directory was to make an empty directory and diff -urN it
against the new directory.  This gets the files in the patch, unfortunately,
they come out at the wrong place when the patch is applied.  The new files are
supposed to go in content/xbl/builtin/gtk2.

As far as the actual implementation, I removed all of the bindings for input
and textarea except for the ones gtk doesn't cover at all, and create a dummy
native entry or textview to receive key events.  I then have it responding to
the signals that are generated by translating them to the corresponding XUL
commands.  It's fairly straightforward.  There are a couple of bindings that I
don't think it's possible to support without extending editor (commented in the
code).

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can't nsNativeKeyBindingsGTK.cpp live in widget/src/gtk2?

Can't we share the hidden GTK widget across all nsNativeKeyBindings of a
particular type? I'm a bit afraid of the cost of this for very forms-heavy pages.

Comment 3

[Brian Ryner (not reading)]

(In reply to comment #2)
> Can't nsNativeKeyBindingsGTK.cpp live in widget/src/gtk2?
> 

Perhaps; widget would either need to depend on the DOM event interfaces, or we'd
need to create a separate interface for this and have layout implement the
DOMEventListener and proxy the calls.

> Can't we share the hidden GTK widget across all nsNativeKeyBindings of a
> particular type? I'm a bit afraid of the cost of this for very forms-heavy pages.

Good point.  We should be able to do that.

Comment 4

[Brian Ryner (not reading)]

Created attachment 157856 [details] [diff] [review]
revised patch

implemented roc's suggestion

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 157856 [details] [diff] [review]
revised patch

Looks good but I have one more suggestion: package these parameters:

PRUint32 aDOMKeyCode, PRUint32 aDOMCharCode,
+			      PRBool aAltKey, PRBool aCtrlKey,
+			      PRBool aShiftKey, PRBool aMetaKey,

into a struct to be passed to nsNativeKeyBindings. Then you can share the
DOMevent to native key event conversion code.

Comment 6

[Brian Ryner (not reading)]

(In reply to comment #5)

> into a struct to be passed to nsNativeKeyBindings. Then you can share the
> DOMevent to native key event conversion code.
> 

Share it with what?

Comment 7

[Brian Ryner (not reading)]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > into a struct to be passed to nsNativeKeyBindings. Then you can share the
> > DOMevent to native key event conversion code.
> > 
> 
> Share it with what?
> 

Oh, you probably mean share the code that separates out all of the parameters
between KeyDown, KeyPress, and KeyUp.

Comment 8

[Brian Ryner (not reading)]

Created attachment 157901 [details] [diff] [review]
one more time

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 157901 [details] [diff] [review]
one more time

+  PRBool   altKey;
+  PRBool   ctrlKey;
+  PRBool   shiftKey;
+  PRBool   metaKey;

PRPackedBool

> doCommandCallback

DoComamndCallback

r+ on the layout code, but better get blizzard to recheck the gtk code

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> (From update of attachment 157901 [details] [diff] [review])
> +  PRBool   altKey;
> +  PRBool   ctrlKey;
> +  PRBool   shiftKey;
> +  PRBool   metaKey;
> 
> PRPackedBool

Then I couldn't pass its address directly to the nsIDOMEvent getters and would
have to do an extra copy.  It's just a few extra bytes on the stack, no big deal.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 157901 [details] [diff] [review]
one more time

blizzard, does the gtk part of this still look ok?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

fair enough

Comment 13

[Brendan Eich [:brendan]]

We want this for PR1, right?

/be

Comment 14

[chris hofmann]

Comment on attachment 157901 [details] [diff] [review]
one more time

a=chofmann for the branches

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

WHY?

Comment 16

[Brian Ryner (not reading)]

(In reply to comment #15)
> WHY?

We identified this as a pretty major usability issue on Linux that we'd like to
have fixed for Firefox 1.0.  However, I think this needs to at minimum bake on
the trunk for a few days. (I also need to implement the bindings for 'editor' so
that they work as expected in mail compose)

Comment 17

[Brendan Eich [:brendan]]

So no one answered by "we want this for PR1" question, but based on what bryner
says, I don't think this should block PR1.  It (this patch plus follow-on ones)
could go into the aviary branch after PR1, provided the uber-patch tests well,
and so long as there aren't l10n implications.

/be

Comment 18

[Boris Zbarsky [:bz] (no decent commit message means r-)]

I think Robert's question was more along the lines of "why are we landing this
sort of thing on stable branches at all?"

Comment 19

[Brendan Eich [:brendan]]

This patch is wanted in the aviary branch so we can better integrate with GNOME.
 Since drivers want aviary and 1.7 branch back end code to sync up by aviary
1.0, it's therefore likely to go into the 1.7 branch soon-ish.

We put "bug fixes" that add features or improve OS integration on stable
branches all the time, and have for years, if memory serves.  The question is,
how risky is this patch?  How hard to test its entire surface?  It doesn't seem
that bad to me.

/be

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is a feature, not a bug fix, that we've shipped many releases without, and
I've heard no clamor for. If we backported all features and bug fixes to stable
branches we'd obviously have no need for branches at all, So I'd like to hear
what makes this feature especially worthy.

Comment 21

[Brendan Eich [:brendan]]

GNOME wants you to use its keybindings, which default to Windows-ish but can be
made Emac-ish for old farts like me.  Comment 0 says this all, and we are at a
loss in GNOME-based distributions without a fix here for Firefox 1.0.

If you don't want this on 1.7, then you'll have to live with a difference
between "Gecko" in Firefox 1.0 and in 1.7.x, x >= 4.

I'm sure there are many other *potential* feature-sneaks to worry about
fighting, but I don't think this is the hill on which 1.7.x should make its last
stand.

/be

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That doesn't answer the question. You haven't given me a way to distinguish this
bug from any other major bug we've fixed in 1.8 trunk.

Comment 23

[Brendan Eich [:brendan]]

I made one distinction: this patch is wanted for the aviary branch, in due
course but before 1.0. If that's not enough, then we can go into the patch and
argue about risks and rewards, but do we really need to?

/be

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

"Someone wants it on the Aviary branch" is true for most bugs.

"MF people want this bug on the Aviary branch, but not that one" without further
explanation is arbitrary, and of no help when I deal with people clamouring for
fixes to be ported to Aviary.

Comment 25

[Brendan Eich [:brendan]]

> "Someone wants it on the Aviary branch" is true for most bugs.


Not true of the passive-voice subject I circumlocuted last time ;-).  Aviary
branch management, including yours truly, want this bug, and few others, ported
from the trunk in time for 1.0.

What's arbitrary?  Comment 0 and comment 21 talk about why this matters for
GNOME users, and even moreso for distributors who insist on silly things like
platform look and feel.  Sure, we have other GNOME HIG/L&F bugs, but this one
has a patch, is high-profile, and should be fixed.  We'll consider others only
of same or higher profile.  Oh, now our judgment of what's high profile will be
called arbitrary.

If there were an algorithm for deciding what bugfixes to take, we wouldn't need
people in the loop.

/be

Comment 26

[Pierre Chanial]

roc: few points to try to convince you :)

- in my eyes, the conflict between emacs keybindings and the chrome keybindings
is our number one usability issue on the linux platform.
To say that acknowleges that we've all done a long and good polishing work for
years. We all agree that we need bryner's patch, but I would say we need it for
FF1.0. If we look at linux desktops newly deployed in enterprises, used by
people who would think Emacs is an online junk food provider but that need to
get their work done quickly. Wouldn't they think: "Firefox... yeah... it kinda
works... randomly shortcuts stop working". For some shortkeys, speaking for
myself, I ended by systematically clicking on the content before typing them...
so what's the point of shortkeys if one has to use the mouse?
The linux browser market is a competitive one and there are solutions that just
work and follow the GNOME HIG.

- bryner's patch is gtk2 only. There is no gtk2-enabled tinderbox on the trunk
or 1.7 branch. Embedors can still stick to the fully tested and reliable gtk
builds (as Netscape did for 7.2).

- In the FF perspective, FF 1.0 is sth like a bit more than one month away from
now. That's more than the beta->release span we had with seamonkey and makes
bryner's patch a 'beta stage' one. Well, I've seen larger patches landed in beta
stage.
I know, that's not the 1.7 branch perspective which is not release driven, but
it helps understanding our motivation for pushing this patch in the aviary branch.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #26)
> - in my eyes, the conflict between emacs keybindings and the chrome 
> keybindings is our number one usability issue on the linux platform.
> To say that acknowleges that we've all done a long and good polishing work for
> years. We all agree that we need bryner's patch, but I would say we need it
> for FF1.0. If we look at linux desktops newly deployed in enterprises, used by
> people who would think Emacs is an online junk food provider but that need to
> get their work done quickly. Wouldn't they think: "Firefox... yeah... it kinda
> works... randomly shortcuts stop working". For some shortkeys, speaking for
> myself, I ended by systematically clicking on the content before typing
> them... so what's the point of shortkeys if one has to use the mouse?
> The linux browser market is a competitive one and there are solutions that
> just work and follow the GNOME HIG.

This point is marginal unless you can point me to online blogs, forums, or even
Bugzilla votes that indicate a crowd sees this as a problem. I just haven't ever
seen such feedback.

> - bryner's patch is gtk2 only. There is no gtk2-enabled tinderbox on the trunk
> or 1.7 branch. Embedors can still stick to the fully tested and reliable gtk
> builds (as Netscape did for 7.2).

This is a very good point.

> - In the FF perspective, FF 1.0 is sth like a bit more than one month away
> from now. That's more than the beta->release span we had with seamonkey and
> makes bryner's patch a 'beta stage' one. Well, I've seen larger patches landed
> in beta stage.

This is a really weak point. Releases on *stable* Seamonkey branches have always
been significantly more than one month from beta to release. And "this isn't the
dumbest thing we've ever done" is less than compelling.

Go ahead and check this into the branches, then.

Comment 28

[Brian Ryner (not reading)]

Created attachment 158374 [details] [diff] [review]
patch for editor

This turned out to be a little more involved.  In order to not get duplicate
actions for the handlers defined in htmlBindings.xml, I had to get rid of the
hardcoded stuff in XBL to install editorBase as a handler (the global window
handlers were installed in sequence, rather than having the platform one
inherit like inputFields does, not sure why).  So instead of making it inherit,
I decided to just get rid of htmlBindings.xml, and separate out the common
handlers into files that can be #included into the platform handlers.  So, for
gtk2's editor binding, I just make it _not_ #include editor-base.inc.

Comment 29

[chris hofmann]

decided to get this landed and tested hard with PR release candidate builds to
get some feedback and make a decision on keeping it in the next few days.

bryner to land tonight and asa to help get test coverage and release note update.

Comment 30

[Brian Ryner (not reading)]

Comment on attachment 158374 [details] [diff] [review]
patch for editor

Boris, could you review? Most of this is pretty straightforward.  Instead of
updating the obsolete mac installer list to remove htmlBindings.xml, I just
removed the package file.

Comment 31

[Brian Ryner (not reading)]

Created attachment 158383 [details] [diff] [review]
patch merged to aviary branch

Comment 32

[Jeff Walden (gone starting June 8)]

Exactly what keybindings does this change?  This is a rather huge change for
Help documentation to implement, and I'm pretty sure that I'd miss at least a
few of the changes if I simply downloaded the next nightly and attempted to find
all the changes myself.

I'd like to have a list of the keybindings changed posted here (with the
shortcut keys for each style if someone's willing to make the effort) so I can
get this done before 1.0PR.  It's not a difficult fix to make in the Help code,
but it's not easy to make sure the fix is done fully and correctly.  (Because I
don't think we can detect these keybindings through Help and Javascript, we
might just use the copout method of "GNOME default" or somesuch with "please see
this prefs panel in GNOME for the value" as a footnote, in which case the list
would suffice.)

I've opened bug 258684 for fixing the Help docs.  If you can post the list,
please do so either at this bug or at that bug (your choice) -- I'll be watching
both closely.

Comment 33

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Comment on attachment 158374 [details] [diff] [review]
patch for editor

>Index: content/xbl/src/nsXBLWindowKeyHandler.cpp
>+    } else {
>+      handled = sNativeEditorBindings->KeyUp(nativeEvent,

Why are we doing keyup if we know this is not a keyup event?

With this fixed or clearly documented, r=bzbarsky

Comment 34

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 158374 [details] [diff] [review]
patch for editor

sr=[e-mail]

Comment 35

[Boris Zbarsky [:bz] (no decent commit message means r-)]

It may be worth asserting that the type is keydown if it's neither keyup nor
keypress...

Comment 36

[Brian Ryner (not reading)]

all checked in.

Comment 37

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Also, the Windows build on the SeaMonkey tinderbox page (creature) has been red
ever since it landed.  It's not finding the .inc file.

Comment 38

[Scott MacGregor]

Created attachment 158441 [details] [diff] [review]
patch to make this work for thunderbird

Untested patch to match the packaging changes Brian made for Firefox last
night.

Brian, I added code to the windows installer to remove htmlBindings.xml since
it looks like it's no longer part of the build. Should the Firefox installer be
doing the same?

Comment 39

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Wasn't this patch supposed to only affect GTK2 builds?  Was it even tested with
GTK1 builds?  It breaks emacs keybindings in GTK1 builds....  See bug 259011.

Comment 40

[cls]

This also broke the otaku tinderbox.  Not all versions of mkdir support the -p
option.  We should be using '$(NSINSTALL) -D' instead.

Comment 41

[Steffen Wilberg]

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

Comment 42

[sairuh (rarely reading bugmail)]

vrfy'd fixed by testing the following builds on linux fedora core 2:

a. mozilla 2004092307-trunk (gtk1): old-style (emacs-like) keybindings in
textareas continue to take precendence over application shortcuts.

b. mozilla 2004092301-trunk (gtk2): keybindings obey the gtk2 prefs, which for
me are the gnome style (as opposed to emacs-like). in this case, the application
shortcuts took precendence.

c. firefox 2004092110-0.9+ (branch, gtk2): same results as (b).

Comment 43

[Wladimir Palant (for Adblock Plus info Cc [e-mail])]

This caused a regression in bug 257523.

Comment 44

[Ginn Chen]

This caused a regression in bug 264325

Comment 45

[seth arnold]

Bug #260188 suggested a workaround for this new behaviour, but I was not able to
make it function. Would someone who has worked around this new behaviour please
take a look at my strace log and suggest further courses of action to restore
the old behaviour?

Thanks

https://bugzilla.mozilla.org/show_bug.cgi?id=260188#c14

Comment 46

[seth arnold]

I found the magic lines required to work around this problem. People who came
here looking for a solution, add the following two lines to your ~/.gtkrc-2.0 file:
include "/usr/share/themes/Emacs/gtk-2.0-key/gtkrc"
gtk-key-theme-name = "Emacs"


Make sure the included file exists. It may move around from distribution to
distribution. I found both lines are required.

Restart firefox. Enjoy your old keybindings again. :)

Comment 47

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 48

[:Gavin Sharp [email: [e-mail]]]

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

Comment 49

[Akkana Peck]

I have
gtk-entry-select-on-focus = 0
and it generally works in gtk apps, but clicking in the firefox urlbar now does
a select-all (where it didn't before).

Shouldn't that be part of this bug?  I know it's not customized in the same
place in firefox, but it's related functionality, customized in the same place
in gtk.  Or is there another bug tracking select-on-focus?

Comment 50

[sairuh (rarely reading bugmail)]

(In reply to comment #49)
> I have gtk-entry-select-on-focus = 0
> and it generally works in gtk apps, but clicking in the firefox urlbar now does
> a select-all (where it didn't before).

I think that's covered by bug 172203.