Use gtk2 native keybindings for input and textarea

VERIFIED FIXED

Status

()

Core
Layout: Form Controls
VERIFIED FIXED
13 years ago
6 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: Brian Ryner (not reading))

Tracking

({fixed-aviary1.0, fixed1.7.5, relnote})

Trunk
x86
Linux
fixed-aviary1.0, fixed1.7.5, relnote
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0PR +
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
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).
(Assignee)

Updated

13 years ago
Attachment #157388 - Flags: superreview?(roc)
Attachment #157388 - Flags: review?(dbaron)
Attachment #157388 - Flags: review?(dbaron) → review+
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.
(Assignee)

Comment 3

13 years ago
(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.
(Assignee)

Updated

13 years ago
Attachment #157388 - Flags: superreview?(roc)
(Assignee)

Comment 4

13 years ago
Created attachment 157856 [details] [diff] [review]
revised patch

implemented roc's suggestion
Attachment #157388 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #157856 - Flags: superreview?(roc)
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.
(Assignee)

Comment 6

13 years ago
(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?
(Assignee)

Comment 7

13 years ago
(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.
(Assignee)

Updated

13 years ago
Attachment #157856 - Flags: superreview?(roc)
(Assignee)

Comment 8

13 years ago
Created attachment 157901 [details] [diff] [review]
one more time
(Assignee)

Updated

13 years ago
Attachment #157856 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #157901 - Flags: superreview?(roc)
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
Attachment #157901 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 10

13 years ago
(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.
(Assignee)

Comment 11

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

blizzard, does the gtk part of this still look ok?
Attachment #157901 - Flags: review?(blizzard)
fair enough
Attachment #157901 - Flags: review?(blizzard) → review+
We want this for PR1, right?

/be
Flags: blocking-aviary1.0PR+

Updated

13 years ago
Attachment #157901 - Flags: approval1.7.x?
Attachment #157901 - Flags: approval-aviary?

Comment 14

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

a=chofmann for the branches
Attachment #157901 - Flags: approval1.7.x?
Attachment #157901 - Flags: approval1.7.x+
Attachment #157901 - Flags: approval-aviary?
Attachment #157901 - Flags: approval-aviary+
WHY?
(Assignee)

Comment 16

13 years ago
(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)
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
Flags: blocking-aviary1.0PR+ → blocking-aviary1.0+
I think Robert's question was more along the lines of "why are we landing this
sort of thing on stable branches at all?"
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
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.
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
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.
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
"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.
> "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

13 years ago
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.
(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.
(Assignee)

Comment 28

13 years ago
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

13 years ago
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.
Flags: blocking-aviary1.0PR+
Keywords: relnote
(Assignee)

Comment 30

13 years ago
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.
Attachment #158374 - Flags: review?(bzbarsky)
(Assignee)

Comment 31

13 years ago
Created attachment 158383 [details] [diff] [review]
patch merged to aviary branch
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 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
Attachment #158374 - Flags: review?(bzbarsky) → review+
Comment on attachment 158374 [details] [diff] [review]
patch for editor

sr=ben@mozilla.org
Attachment #158374 - Flags: superreview+
Attachment #158374 - Flags: approval-aviary+
It may be worth asserting that the type is keydown if it's neither keyup nor
keypress...
(Assignee)

Comment 36

13 years ago
all checked in.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed-aviary1.0, fixed1.7.x
Resolution: --- → FIXED
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

13 years ago
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?

Updated

13 years ago
Attachment #158441 - Flags: superreview?(bryner)
(Assignee)

Updated

13 years ago
Attachment #158441 - Flags: superreview?(bryner) → superreview+
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

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

Updated

13 years ago
Blocks: 260312

Comment 41

13 years ago
*** Bug 254903 has been marked as a duplicate of this bug. ***
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).
Status: RESOLVED → VERIFIED

Updated

13 years ago
Blocks: 261426

Comment 43

13 years ago
This caused a regression in bug 257523.

Comment 44

13 years ago
This caused a regression in bug 264325

Comment 45

13 years ago
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

13 years ago
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. :)
*** Bug 270001 has been marked as a duplicate of this bug. ***
*** Bug 189615 has been marked as a duplicate of this bug. ***

Comment 49

13 years ago
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?
(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.
You need to log in before you can comment on or make changes to this bug.