Last Comment Bug 257405 - Use gtk2 native keybindings for input and textarea
: Use gtk2 native keybindings for input and textarea
Status: VERIFIED FIXED
: fixed-aviary1.0, fixed1.7.5, relnote
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Brian Ryner (not reading)
:
Mentors:
: 189615 254903 270001 (view as bug list)
Depends on:
Blocks: 260312 261426
  Show dependency treegraph
 
Reported: 2004-08-30 02:05 PDT by Brian Ryner (not reading)
Modified: 2011-08-05 21:30 PDT (History)
16 users (show)
chofmann: blocking‑aviary1.0PR+
brendan: blocking‑aviary1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (35.80 KB, patch)
2004-08-30 02:10 PDT, Brian Ryner (not reading)
blizzard: review+
Details | Diff | Review
revised patch (46.97 KB, patch)
2004-09-03 23:38 PDT, Brian Ryner (not reading)
no flags Details | Diff | Review
one more time (44.12 KB, patch)
2004-09-04 12:18 PDT, Brian Ryner (not reading)
blizzard: review+
roc: superreview+
chofmann: approval‑aviary+
chofmann: approval1.7.5+
Details | Diff | Review
patch for editor (80.03 KB, patch)
2004-09-09 15:56 PDT, Brian Ryner (not reading)
bzbarsky: review+
bugs: superreview+
bugs: approval‑aviary+
Details | Diff | Review
patch merged to aviary branch (44.53 KB, patch)
2004-09-09 17:45 PDT, Brian Ryner (not reading)
no flags Details | Diff | Review
patch to make this work for thunderbird (1.14 KB, patch)
2004-09-10 09:24 PDT, Scott MacGregor
bryner: superreview+
Details | Diff | Review

Description Brian Ryner (not reading) 2004-08-30 02:05:25 PDT
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) 2004-08-30 02:10:55 PDT
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) (Exited; email my personal email if necessary) 2004-09-01 18:29:03 PDT
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) 2004-09-02 14:59:23 PDT
(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) 2004-09-03 23:38:14 PDT
Created attachment 157856 [details] [diff] [review]
revised patch

implemented roc's suggestion
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-04 05:49:31 PDT
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) 2004-09-04 11:13:42 PDT
(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) 2004-09-04 11:19:30 PDT
(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) 2004-09-04 12:18:45 PDT
Created attachment 157901 [details] [diff] [review]
one more time
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-04 15:45:48 PDT
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) 2004-09-04 16:05:42 PDT
(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) 2004-09-04 16:07:11 PDT
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) (Exited; email my personal email if necessary) 2004-09-04 21:19:34 PDT
fair enough
Comment 13 Brendan Eich [:brendan] 2004-09-07 11:51:52 PDT
We want this for PR1, right?

/be
Comment 14 chris hofmann 2004-09-07 11:57:38 PDT
Comment on attachment 157901 [details] [diff] [review]
one more time

a=chofmann for the branches
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2004-09-07 12:27:36 PDT
WHY?
Comment 16 Brian Ryner (not reading) 2004-09-07 13:31:06 PDT
(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] 2004-09-07 13:53:06 PDT
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] 2004-09-07 15:15:45 PDT
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] 2004-09-07 18:21:25 PDT
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) (Exited; email my personal email if necessary) 2004-09-07 20:50:42 PDT
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] 2004-09-07 21:37:39 PDT
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) (Exited; email my personal email if necessary) 2004-09-08 05:51:42 PDT
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] 2004-09-08 11:24:15 PDT
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) (Exited; email my personal email if necessary) 2004-09-08 12:20:48 PDT
"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] 2004-09-08 13:47:37 PDT
> "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 2004-09-08 14:00:36 PDT
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) (Exited; email my personal email if necessary) 2004-09-08 14:19:38 PDT
(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) 2004-09-09 15:56:45 PDT
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 2004-09-09 17:04:57 PDT
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) 2004-09-09 17:06:52 PDT
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) 2004-09-09 17:45:21 PDT
Created attachment 158383 [details] [diff] [review]
patch merged to aviary branch
Comment 32 Jeff Walden (gone starting June 8) 2004-09-09 20:12:37 PDT
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] 2004-09-09 20:55:19 PDT
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) 2004-09-09 22:56:30 PDT
Comment on attachment 158374 [details] [diff] [review]
patch for editor

sr=ben@mozilla.org
Comment 35 Boris Zbarsky [:bz] 2004-09-10 01:14:54 PDT
It may be worth asserting that the type is keydown if it's neither keyup nor
keypress...
Comment 36 Brian Ryner (not reading) 2004-09-10 01:15:56 PDT
all checked in.
Comment 37 Boris Zbarsky [:bz] 2004-09-10 08:22:53 PDT
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 2004-09-10 09:24:54 PDT
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] 2004-09-12 13:42:42 PDT
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 2004-09-18 11:33:00 PDT
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 2004-09-23 01:06:02 PDT
*** Bug 254903 has been marked as a duplicate of this bug. ***
Comment 42 sairuh (rarely reading bugmail) 2004-09-23 15:13:10 PDT
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 2004-09-30 01:38:42 PDT
This caused a regression in bug 257523.
Comment 44 Ginn Chen 2004-10-14 02:13:24 PDT
This caused a regression in bug 264325
Comment 45 seth arnold 2004-10-27 11:27:02 PDT
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 2004-10-27 20:19:38 PDT
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) 2004-11-15 10:57:07 PST
*** Bug 270001 has been marked as a duplicate of this bug. ***
Comment 48 :Gavin Sharp [email: gavin@gavinsharp.com] 2004-11-16 17:49:30 PST
*** Bug 189615 has been marked as a duplicate of this bug. ***
Comment 49 Akkana Peck 2004-11-18 22:19:33 PST
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) 2004-11-18 22:42:34 PST
(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.

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