259011 - Textfield keybindings busted in GTK1 builds

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The patch for bug 257405 broke Emacs keybindings in GTK1 builds.

STEPS TO REPRODUCE:

1)  Load http://bugzilla.mozilla.org/enter_bug.cgi?product=Browser
2)  Type "This is a test" in the textarea
3)  Hit Ctrl-a

EXPECTED RESULTS:  caret goes to beginning of line

ACTUAL RESULTS:  all text is selected.

This is a pretty major usability regression (since there is no way to undo this
change in GTK1 builds, unlike GTK2 ones).

Comment 1

[Boris Zbarsky [:bzbarsky]]

The reason this broke is that xbl/builtin/unix/platformHTMLBindings.xml ends up
with something along the lines of:

  <handler event="keypress" key="a" command="cmd_selectAll" modifiers="accel"/>
  <handler event="keypress" key="a" modifiers="control" command="cmd_beginLine"/>

I suspect XBL ignores the second handler, since there is a handler already
defined for that key/accel combination.

Before, the second handler was defined in a binding that extended the binding
with the first handler, so it overrode the handler from the parent binding... 
We probably need to go back to that setup.

Comment 2

[Pierre Chanial]

Another possibility is to move the "select_all" handlers out of the include
files and add them to the win/mac/unix platform handlers.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Is select_all the only key that the Unix binding overrode?

Comment 4

[Pierre Chanial]

win/mac only, I meant.

bz: yeah, there's not a lot of ctrl-ASCII shortcuts in the .inc files:
ctrl-A,C,V,X,Z,shift-Z. The emacs ctrl-Z (minimize) and ctrl-V (scroll) bindings
have never been implemented in gtk1.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Maybe the right approach is to move the inc to after the handlers of the
"derived" binding.  That way the "derived" binding would win out, right?

Comment 6

[Pierre Chanial]

(In reply to comment #5)
> That way the "derived" binding would win out, right?
Hu, well thought.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch

This actually makes the most sense to me.  Since ctrl+a for select all is not
actually a "common" keybindings, I removed it from the .inc file and added it
to the platform keybindings for mac and windows.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158782 [details] [diff] [review]
patch

Yeah, if this is the only key that was overridden... r+sr=bzbarsky

Comment 9

[Brian Ryner (not reading)]

landed on trunk, aviary, and 1.7