Fix key modifier handling in Web backends [backport 1.4.x] #4029

blink1073 · 2015-01-23T03:50:01Z

Addresses #4028.
Fixes an earlier mishandling of the key_release event.
Handles all of the standard keys, including shift modifiers.
Also properly handles shift+enter event in nbagg, moving to the next cell.

Test script:

import matplotlib
matplotlib.use('webagg')
import numpy as np
import matplotlib.pyplot as plt

fig = plt.figure()
ax = fig.add_subplot(111)
ax.plot(np.random.rand(10))


def onkeyevent(event):
    fig.suptitle('%s: %s' % (event.name, event.key))
    fig.canvas.draw()

cid = fig.canvas.mpl_connect('key_press_event', onkeyevent)
cid = fig.canvas.mpl_connect('key_release_event', onkeyevent)
plt.show()

tacaswell · 2015-01-23T04:03:13Z

lib/matplotlib/backends/web_backend/mpl.js

+        value += "shift";
+    }
+    else {
+        value += String.fromCharCode(event.keyCode).toLowerCase();


This does raise the question why we don't pass 'shift+e' and as 'E', but I suspect that there is a good reason for this....(goes to look how qt does it).

AFAIK, Qt just feeds you a key code, and leaves it up to you to capitalize it based on the shift modifier.

Is this behaviour consistent with each of the QtAgg, TkAgg and GtkAgg backends? (with regards to shift?)

The qt backend applies the shift, have not checked the others.

On Fri, Jan 23, 2015, 08:07 Phil Elson notifications@github.com wrote:

In lib/matplotlib/backends/web_backend/mpl.js
#4029 (comment):

value += String.fromCharCode(event.keyCode).toLowerCase();

if (event.shiftKey && !(event.keyCode == 16)) {

value += "shift+";

}

if (event.keyCode == 17) {

value += "control";

}

else if (event.keyCode == 18) {

value += "alt";

}

else if (event.keyCode == 16) {

value += "shift";

}

else {

value += String.fromCharCode(event.keyCode).toLowerCase();

Is this behaviour consistent with each of the QtAgg, TkAgg and GtkAgg
backends? (with regards to shift?)

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4029/files#r23448195.

I think all backends apply the shift. I have keymaps that have capital
letters.
On Jan 23, 2015 8:12 AM, "Thomas A Caswell" notifications@github.com
wrote:

In lib/matplotlib/backends/web_backend/mpl.js
#4029 (comment):

value += String.fromCharCode(event.keyCode).toLowerCase();

if (event.shiftKey && !(event.keyCode == 16)) {

value += "shift+";

}

if (event.keyCode == 17) {

value += "control";

}

else if (event.keyCode == 18) {

value += "alt";

}

else if (event.keyCode == 16) {

value += "shift";

}

else {

value += String.fromCharCode(event.keyCode).toLowerCase();

The qt backend applies the shift, have not checked the others.
… <#14b16ebd01a533d2_>
On Fri, Jan 23, 2015, 08:07 Phil Elson notifications@github.com wrote:
In lib/matplotlib/backends/web_backend/mpl.js <
https://github.com/matplotlib/matplotlib/pull/4029#discussion_r23448195>:

value += String.fromCharCode(event.keyCode).toLowerCase(); > + if
(event.shiftKey && !(event.keyCode == 16)) { > + value += "shift+"; > + } >

if (event.keyCode == 17) { > + value += "control"; > + } > + else if
(event.keyCode == 18) { > + value += "alt"; > + } > + else if
(event.keyCode == 16) { > + value += "shift"; > + } > + else { > + value +=
String.fromCharCode(event.keyCode).toLowerCase(); Is this behaviour
consistent with each of the QtAgg, TkAgg and GtkAgg backends? (with regards
to shift?) — Reply to this email directly or view it on GitHub <
https://github.com/matplotlib/matplotlib/pull/4029/files#r23448195>.

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4029/files#r23448370.

So we want to remove the .toLowerCase() then?

Well, we'd also need to remove "shift" from the value.

I think only if the character can be capitalized.
On Jan 23, 2015 9:48 AM, "Steven Silvester" notifications@github.com
wrote:

In lib/matplotlib/backends/web_backend/mpl.js
#4029 (comment):

value += String.fromCharCode(event.keyCode).toLowerCase();

if (event.shiftKey && !(event.keyCode == 16)) {

value += "shift+";

}

if (event.keyCode == 17) {

value += "control";

}

else if (event.keyCode == 18) {

value += "alt";

}

else if (event.keyCode == 16) {

value += "shift";

}

else {

value += String.fromCharCode(event.keyCode).toLowerCase();

Well, we'd also need to remove "shift" from the value.

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4029/files#r23453341.

I'll play around with it tonight.

tacaswell · 2015-01-23T04:32:41Z

Great! That seems to have fixed it. The only dicey thing is that it seems to generate tons of events if you hold a key down (despite 'keydown' claiming to only fire once).

attn @pelson @mdboom

blink1073 · 2015-01-23T14:46:42Z

@tacaswell, I added a guard against multiple key press events.

blink1073 · 2015-01-23T19:33:09Z

We can now handle shift state and tell a vs A. However, the Javascript key codes do not map to ASCII codes. We'd have to use a LUT to handle keys like Home and the function keys.

blink1073 · 2015-01-23T19:53:40Z

I've got most of the key identifiers working in both Firefox and Chrome. The only one that didn't work was '~' in Chrome.

blink1073 · 2015-01-24T21:21:46Z

I believe I got all key combinations right in both Firefox and Chrome. I also updated the UAT to show the event type and metadata in the figure title.
I tried booting into Windows to try IE, but ran into several problems, then Windows told me to restart, and I rage quit...

blink1073 · 2015-01-25T03:30:11Z

jQuery provides the which attribute, so I used that consistently and pushed the key handling logic into Python. I used a LUT for those values that could not be easily teased out.

blink1073 · 2015-01-25T03:57:23Z

@tonysyu, would you mind testing this with Safari using the UAT notebook (the updated one in this PR)? Basically, just try a bunch of key combinations, shifted and unshifted.

tonysyu · 2015-01-25T22:20:47Z

@blink1073: This seems to behave the same on Safari and Chrome.

One small issue (independent of browser) is that this branch changes the behavior where you can simply advance through the notebook using Shift+Enter: When you hit a cell with a figure, Shift+Enter executes the cell but doesn't advance to the next cell. This might be unavoidable, but I don't know enough about what's going on underneath the hood.

tacaswell · 2015-01-25T22:26:43Z

That is probably because nbagg grabs focus on plot (to match the behavior of the GUI backends) and when it get focus it disables all of IPython's key event handling (so that we can get the key events and pass them down to the python layer).

Maybe we want to special case 'shift-enter' and bounce that back up to IPython?

blink1073 · 2015-01-25T22:29:47Z

@tacaswell, seems a bit magical to me, but I don't feel that strongly.

tacaswell · 2015-01-25T22:42:39Z

From watching people use notebooks being able to 'shift-enter' your way down a page seems to be a major use-case which I would rather not break.

cc @danielballan @rgbkrk am I reading the importance of this use-case right?

blink1073 · 2015-01-25T22:53:02Z

I'm looking into it now, but I'm a bit stuck, see 0efea3b.
Passing the event to the IPython keyboard manager just refreshes the existing output.

blink1073 · 2015-01-25T23:02:28Z

It works now, spoofing a "J" command, which tells IPython to go to the next cell.

rgbkrk · 2015-01-25T23:09:22Z

Yeah, shift-enter is the typical flow for a lot of people (including myself) for running through a notebook.

danielballan · 2015-01-26T00:25:47Z

Yes that is an important use case.

Under the hood, cell advancement is related to the mode (edit or command). That is vague; I'm willing to take a closer look if needed.

blink1073 · 2015-01-26T01:28:15Z

Thanks @danielballan, I added an explicit call to command_mode() prior to handle_keydown()

Fix key modifier handling Remove commented out code Ignore pause/break and caps lock

Prevent repeat key events Add missing brace Use cross-platform event.which and handle shift casing Handle other special keys Add support for firefox Handle modified number keys (like #), pause, and capslock Cleaner handling of shift Style update Style update Handle all key events in chrome Better handling of shift modifier in firefox

Update UAT to display the event type and data in title Add source encoding to file

Move key code handling logic to python Add support for space key Rename keys for consistency with Qt backend Add a link to the key codes Add comments

Partial implementation of shift+enter in nbagg Use the J key command to move to next cell Remove errant change to uat Add a comment and handle IPython 2/3 Move the shift+enter handling to nbagg_mpl Add explicit mode switch for shift+enter Clean up handling of keyboard manager variable Remove extraneous changes Remove unused import Remove newline change

blink1073 · 2015-01-26T01:48:54Z

@tacaswell, I think I'm done monkeying around 😄 Rebased.

danielballan · 2015-01-26T02:40:12Z

@blink1073 Great.

pelson · 2015-01-26T13:07:46Z

lib/matplotlib/backends/web_backend/mpl.js

@@ -458,21 +458,35 @@ mpl.figure.prototype.mouse_event = function(event, name) {
    return false;
 }

+mpl.figure.prototype._key_event_extra = function(event, name) { 


Do we need to start doing JS "docstrings"?

tacaswell · 2015-01-27T02:36:09Z

@blink1073 have a PR against your branch, there is a missing manager = assignment for 'old' IPython versions.

BUG : missing manager assignment

FIX : key modifier handling in Web backends

tacaswell · 2015-01-27T03:18:11Z

backported as 1fa0153

tacaswell added the status: needs review label Jan 23, 2015

tacaswell reviewed Jan 23, 2015
View reviewed changes

tacaswell modified the milestones: v1.5.x, v1.4.3 Jan 23, 2015

tacaswell changed the title ~~Fix key modifier handling in Web backends~~ Fix key modifier handling in Web backends [backport 1.4.x] Jan 23, 2015

blink1073 added 5 commits January 25, 2015 19:46

Fix key modifier handling

70d19e5

Fix key modifier handling Remove commented out code Ignore pause/break and caps lock

Update UAT to display the event type and data in title

11a3010

Update UAT to display the event type and data in title Add source encoding to file

Move key code handling logic to python

5eec284

Move key code handling logic to python Add support for space key Rename keys for consistency with Qt backend Add a link to the key codes Add comments

blink1073 force-pushed the web-key-modifiers branch from 0f955a8 to cef7830 Compare January 26, 2015 01:47

pelson reviewed Jan 26, 2015
View reviewed changes

blink1073 and others added 2 commits January 26, 2015 07:34

Add a 'docstring' to _key_event_extra

31b3f8e

BUG : missing manager assignment

33bffe7

Merge pull request #2 from tacaswell/web-key-modifiers

344bb33

BUG : missing manager assignment

tacaswell added a commit that referenced this pull request Jan 27, 2015

Merge pull request #4029 from blink1073/web-key-modifiers

b90e2b3

FIX : key modifier handling in Web backends

tacaswell merged commit b90e2b3 into matplotlib:master Jan 27, 2015

tacaswell removed the status: needs review label Jan 27, 2015

tacaswell added a commit that referenced this pull request Jan 27, 2015

Merge pull request #4029 from blink1073/web-key-modifiers

1fa0153

FIX : key modifier handling in Web backends

Uh oh!

Fix key modifier handling in Web backends [backport 1.4.x] #4029

Fix key modifier handling in Web backends [backport 1.4.x] #4029

Uh oh!

Conversation

blink1073 commented Jan 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jan 23, 2015

Uh oh!

blink1073 commented Jan 23, 2015

Uh oh!

blink1073 commented Jan 23, 2015

Uh oh!

blink1073 commented Jan 23, 2015

Uh oh!

blink1073 commented Jan 24, 2015

Uh oh!

blink1073 commented Jan 25, 2015

Uh oh!

blink1073 commented Jan 25, 2015

Uh oh!

tonysyu commented Jan 25, 2015

Uh oh!

tacaswell commented Jan 25, 2015

Uh oh!

blink1073 commented Jan 25, 2015

Uh oh!

tacaswell commented Jan 25, 2015

Uh oh!

blink1073 commented Jan 25, 2015

Uh oh!

blink1073 commented Jan 25, 2015

Uh oh!

rgbkrk commented Jan 25, 2015

Uh oh!

danielballan commented Jan 26, 2015

Uh oh!

blink1073 commented Jan 26, 2015

Uh oh!

blink1073 commented Jan 26, 2015

Uh oh!

danielballan commented Jan 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

tacaswell commented Jan 27, 2015

Uh oh!

Uh oh!