Js fixes for key events + ipython notebooks #3965

tacaswell · 2015-01-04T03:05:33Z

First pass at getting key events to work in ipython notebooks

Demo, mouse over the figure to give it focus. Clicking any mouse button or pushing any key should color-cycle the line.

import matplotlib
import itertools
import numpy as np
matplotlib.use('nbagg')
import matplotlib.pyplot as plt
plt.close('all')
fig, ax = plt.subplots()
x = np.linspace(0,10,10000)
y = np.sin(x)
ln, = ax.plot(x,y)
evt = []
colors = iter(itertools.cycle(['r', 'g', 'b', 'k', 'c']))
def on_event(event):
    evt.append(event)
    ln.set_color(next(colors))
    fig.canvas.draw()
    fig.canvas.draw_idle()
fig.canvas.mpl_connect('button_press_event', on_event)
fig.canvas.mpl_connect('key_press_event', on_event)
plt.show()

Remaining issues:

mousing out of the figure area does not release focus, is this the correct behavior?
not sure I injected the extra style at the right place in the DOM or than I used the right pattern to do it

@eyurtsev

Reported by @eyurtsev

It was unhappy without it (no idea why)

- added hooks to inject style onto elements before they are added to the DOM (this might be excessive/an anti pattern, but I have been doing js for < 6 hrs and this worked) - re-ordered to group things in a way that made more sense to me

Changed from the canvas to the canvas div Not strictly sure this is the right thing to do, but it was on the path to get things working.

Don't let the handling keep bubbling up from this function. Again, not sure this is the right thing to do/if it matters.

- need to make the canvas_div 'focusable' - register the element with the keyboard manager

tacaswell · 2015-01-04T03:07:03Z

I broke this up into a bunch of commits as I spent a long time wandering before I sorted out what the problem was. They can be squashed/discarded before merge.

Also just realized that I should have been doing this against 1.4.x.....

blink1073 · 2015-01-04T03:13:54Z

Works for me! Yay!

blink1073 · 2015-01-04T03:15:33Z

I'd say that it should lose focus when the mouse is not there. Also, there is probably a CSS setting to not draw a box around it when it has focus (it interferes with the navigation bar).

blink1073 · 2015-01-04T03:55:22Z

The following diff removes the outline:

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index 1836488..b619018 100644
--- a/lib/matplotlib/backends/web_backend/mpl.js
+++ b/lib/matplotlib/backends/web_backend/mpl.js
@@ -112,8 +112,7 @@ mpl.figure.prototype._init_canvas = function() {
         function(event, ui) { fig.request_resize(ui.size.width, ui.size.height); }
         , 50)});

-    canvas_div.attr('style', 'position: relative; clear: both;');
-
+    canvas_div.attr('style', 'position: relative; clear: both; outline: 0');

     function canvas_keyboard_event(event) {
         return fig.key_event(event, event['data']);

blink1073 · 2015-01-04T04:13:24Z

Diff including mouseout handling:

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index 1836488..2adcb2f 100644
--- a/lib/matplotlib/backends/web_backend/mpl.js
+++ b/lib/matplotlib/backends/web_backend/mpl.js
@@ -112,8 +112,7 @@ mpl.figure.prototype._init_canvas = function() {
         function(event, ui) { fig.request_resize(ui.size.width, ui.size.height); }
         , 50)});

-    canvas_div.attr('style', 'position: relative; clear: both;');
-
+    canvas_div.attr('style', 'position: relative; clear: both; outline: 0');

     function canvas_keyboard_event(event) {
         return fig.key_event(event, event['data']);
@@ -145,6 +144,11 @@ mpl.figure.prototype._init_canvas = function() {
     canvas_div.append(canvas);
     canvas_div.append(rubberband);

+    function mouse_out_fn(event) {
+        canvas_div.blur();
+    }
+    canvas_div.on("mouseout", mouse_out_fn);
+
     this.rubberband = rubberband;
     this.rubberband_canvas = rubberband[0];
     this.rubberband_context = rubberband[0].getContext("2d");

- remove frame around canvas_div - add mouse-out event to release focus (restoring the keyboard manager snarfing of the keyevents) when the mouse leaves the

tacaswell · 2015-01-04T05:59:09Z

@blink1073 added your diff (faking you as the author).

pelson · 2015-01-04T08:52:18Z

lib/matplotlib/backends/web_backend/nbagg_mpl.js

+    // off when our div gets focus
+
+    // location in version 3
+    // IPython.notebook.keyboard_manager.register_events(canvas_div);


Yeah, we're going to have to know the IPython version in our JS aren't we? I wanted to avoid it, but there is no alternative as fas as I can see.

FWIW, they kept IPython.keyboard_manager as an alias for backward compatibility.

pelson · 2015-01-04T09:03:04Z

@tacaswell - great work. Congratulations on digging into such a complex codebase - there is a lot going on here (mpl backends, mpl web agg backend, mpl nbagg backend, IPython, IPython's JS, browser interactions etc etc.) and you seem to have kept the changes in keeping with the existing codebase. 👍

Can you put a UAT in https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/web_backend/nbagg_uat.ipynb which describes and tests the functionality? Sadly, I'm not a sophisticated enough JS developer to have automated testing so I pumped for the manual testing approach instead. Also, could you confirm that you've tested these changes and they don't break the web_agg backend?

As for when the keyboard event should get focus, I'm not the biggest fan of needing the mouse over the figure, but equally, I can't think of a better approach on a page which potentially has multiple figures.

Finally, it would be good to figure out what is needed to get plt.draw to work on nbagg - my own experience is that you need to call draw/draw_idle on the figure (as you have done) but that shouldn't be necessary.

I think this can certainly be targeted at v1.4.x (one could argue it is adding a feature, but nbagg is sufficiently experimental in v1.4 that I'm comfortable with making minor improvements for the benefit of fixing a few compatibility issues).

Other than that, awesome! 👍

tacaswell · 2015-01-04T15:47:25Z

I started to look at the draw/draw_idle issue, but decided to stay focused on getting the keyboard events working.

I am enthusiastic about getting these fixes into 1.4.3 as skimage is looking at using nbagg to replace their qt-based image-viewer code so we should do our best to support that. It also means that nbagg will get it's tires kicked/exposure which is good.

WeatherGod · 2015-01-04T15:58:17Z

I am not an expert in focus issues, but from what I have encountered
exploring the traditional backends, the canvas object was always set up to
acquire focus immediately (particularly, Qt has a "StrongFocus" policy set
for it). Then only the currently active window gets interactive events,
irrespective of where the mouse is. This would seem to be a "focus follows
mouse" policy, which would deviate from the traditional backends.

OTOH, perhaps nbagg is probably more akin to an embedding situation rather
than a stand-alone figure window, in which case there is no manager object
that would set up keymaps and focus policies?

As a side note, if the focus terminates when the mouse leaves the figure,
does it come back when the mouse enters the figure? There is a
mouse_enter_figure event (or something like that, IIRC).

On Sun, Jan 4, 2015 at 10:47 AM, Thomas A Caswell notifications@github.com
wrote:

I started to look at the draw/draw_idle issue, but decided to stay
focused on getting the keyboard events working.

I am enthusiastic about getting these fixes into 1.4.3 as skimage is
looking at using nbagg to replace their qt-based image-viewer code so we
should do our best to support that. It also means that nbagg will get it's
tires kicked/exposure which is good.

—
Reply to this email directly or view it on GitHub
#3965 (comment)
.

blink1073 · 2015-01-04T17:11:13Z

The following diff makes it behave more like a figure in a Qt window (focus on creation and when the user clicks on the figure):

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index d9053d8..7aee81c 100644
--- a/lib/matplotlib/backends/web_backend/mpl.js
+++ b/lib/matplotlib/backends/web_backend/mpl.js
@@ -120,6 +120,7 @@ mpl.figure.prototype._init_canvas = function() {

     canvas_div.keydown('key_press', canvas_keyboard_event);
     canvas_div.keydown('key_release', canvas_keyboard_event);
+
     this.canvas_div = canvas_div
     this._canvas_extra_style(canvas_div)
     this.root.append(canvas_div);
@@ -144,11 +145,6 @@ mpl.figure.prototype._init_canvas = function() {
     canvas_div.append(canvas);
     canvas_div.append(rubberband);

-    function mouse_out_fn(event) {
-        canvas_div.blur();
-    }
-    canvas_div.on("mouseout", mouse_out_fn);
-
     this.rubberband = rubberband;
     this.rubberband_canvas = rubberband[0];
     this.rubberband_context = rubberband[0].getContext("2d");
@@ -412,7 +408,7 @@ mpl.findpos = function(e) {
 mpl.figure.prototype.mouse_event = function(event, name) {
     var canvas_pos = mpl.findpos(event)

-    if (this.focus_on_mouseover && name === 'motion_notify')
+    if (name === 'button_press')
     {
         this.canvas_div.focus();
     }
diff --git a/lib/matplotlib/backends/web_backend/nbagg_mpl.js b/lib/matplotlib/backends/web_backend/nbagg_mpl.js
index 7706891..d08b7ca 100644
--- a/lib/matplotlib/backends/web_backend/nbagg_mpl.js
+++ b/lib/matplotlib/backends/web_backend/nbagg_mpl.js
@@ -49,6 +49,8 @@ mpl.mpl_figure_comm = function(comm, msg) {
         return false;
     });

+    fig.canvas_div.focus();
+
 };

 mpl.figure.prototype.handle_close = function(fig, msg) {

blink1073 · 2015-01-04T17:13:29Z

Better version of the previous (affects webagg as well):

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index d9053d8..54bccdb 100644
--- a/lib/matplotlib/backends/web_backend/mpl.js
+++ b/lib/matplotlib/backends/web_backend/mpl.js
@@ -120,6 +120,7 @@ mpl.figure.prototype._init_canvas = function() {

     canvas_div.keydown('key_press', canvas_keyboard_event);
     canvas_div.keydown('key_release', canvas_keyboard_event);
+
     this.canvas_div = canvas_div
     this._canvas_extra_style(canvas_div)
     this.root.append(canvas_div);
@@ -144,11 +145,6 @@ mpl.figure.prototype._init_canvas = function() {
     canvas_div.append(canvas);
     canvas_div.append(rubberband);

-    function mouse_out_fn(event) {
-        canvas_div.blur();
-    }
-    canvas_div.on("mouseout", mouse_out_fn);
-
     this.rubberband = rubberband;
     this.rubberband_canvas = rubberband[0];
     this.rubberband_context = rubberband[0].getContext("2d");
@@ -170,6 +166,8 @@ mpl.figure.prototype._init_canvas = function() {
     // Set the figure to an initial 600x600px, this will subsequently be updated
     // upon first draw.
     this._resize_canvas(600, 600);
+
+    this.canvas_div.focus();
 }

 mpl.figure.prototype._init_toolbar = function() {
@@ -412,7 +410,7 @@ mpl.findpos = function(e) {
 mpl.figure.prototype.mouse_event = function(event, name) {
     var canvas_pos = mpl.findpos(event)

-    if (this.focus_on_mouseover && name === 'motion_notify')
+    if (name === 'button_press')
     {
         this.canvas_div.focus();
     }

blink1073 · 2015-01-04T17:25:35Z

Sad panda 😦, webagg no longer supports keypress or scroll events with these changes, and nbagg still does not support scroll events. (These changes being the PR without the diff above).

blink1073 · 2015-01-04T17:29:59Z

It looks like webagg needs to focus on canvas, while nbagg needs to focus on canvas_div (from some guess and checking).

blink1073 · 2015-01-04T17:47:26Z

I may have been mistaken about scroll_event working in webagg before. I reverted to master and it is not working.

blink1073 · 2015-01-04T17:56:42Z

Another diff for you, that allows keyboard events in both nbagg and webagg:

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index d9053d8..6ff5f33 100644
--- a/lib/matplotlib/backends/web_backend/mpl.js
+++ b/lib/matplotlib/backends/web_backend/mpl.js
@@ -414,6 +414,7 @@ mpl.figure.prototype.mouse_event = function(event, name) {

     if (this.focus_on_mouseover && name === 'motion_notify')
     {
+        this.canvas.focus();
         this.canvas_div.focus();
     }

WeatherGod · 2015-01-04T19:19:19Z

Steven, it is great that you are helping Thomas out with this, I know I
certainly don't have the mental fortitude to grok javascript (there is a
reason why I work in python...). I don't know if you realize it, but you
can submit PR's against Thomas's branch that can make it very easy for him
to incorporate your efforts (and you get the credit, too!).

On Sun, Jan 4, 2015 at 12:56 PM, Steven Silvester notifications@github.com
wrote:

Another diff for you, that allows keyboard events in both nbagg and webagg
:

diff --git a/lib/matplotlib/backends/web_backend/mpl.js b/lib/matplotlib/backends/web_backend/mpl.js
index d9053d8..6ff5f33 100644--- a/lib/matplotlib/backends/web_backend/mpl.js+++ b/lib/matplotlib/backends/web_backend/mpl.js@@ -414,6 +414,7 @@ mpl.figure.prototype.mouse_event = function(event, name) {
 if (this.focus_on_mouseover && name === 'motion_notify')
 {+        this.canvas.focus();
     this.canvas_div.focus();
 }
—
Reply to this email directly or view it on GitHub
#3965 (comment)
.

blink1073 · 2015-01-04T19:46:13Z

@WeatherGod, I'm more throwing out ideas than anything (shots in the dark), but point taken. I still can't figure out how to get webagg to take focus on creation. canvas.focus() only seems to work in the callback.

pelson · 2015-01-04T19:52:03Z

If I can help with anything specific, just shout - I don't have a huge amount of bandwidth, but I'll happily try to help.

blink1073 · 2015-01-04T21:03:05Z

Victory! (PR sent).

blink1073 · 2015-01-04T21:09:31Z

@tacaswell, we'll have to tackle scroll_event in another issue. We use it to set the thickness of the line tool and the size of the paint tool.

tacaswell · 2015-01-06T12:36:29Z

Right, sorry forgot about that last night.

On Tue, Jan 6, 2015, 02:02 Phil Elson notifications@github.com wrote:

Can you put a UAT in
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/backends/web_backend/nbagg_uat.ipynb
which describes and tests the functionality? Sadly, I'm not a sophisticated
enough JS developer to have automated testing so I pumped for the manual
testing approach instead. Also, could you confirm that you've tested these
changes and they don't break the web_agg backend?

Just this action to do from me.

—
Reply to this email directly or view it on GitHub
#3965 (comment)
.

blink1073 · 2015-01-07T20:50:28Z

lib/matplotlib/backends/web_backend/nbagg_mpl.js

+    // off when our div gets focus
+
+    // location in version 3
+    if (typeof IPython.notebook.keyboard_manager != 'undefined') {


I think the canonical way of doing this is if (IPython.notebook.keyboard_manager).

tacaswell · 2015-01-08T03:11:05Z

@pelson Is that what you had in mind for the UAT?

pelson · 2015-01-08T06:59:27Z

@pelson Is that what you had in mind for the UAT?

Exactly. I didn't run it yet, but the test seems well described and I imagine it is working for you so 👍 from me.

mdboom · 2015-01-08T12:30:32Z

lib/matplotlib/backends/web_backend/mpl.js

@@ -93,6 +94,15 @@ mpl.figure.prototype._init_header = function() {
    this.header = titletext[0];
 }

+mpl.figure.prototype._canvas_extra_style = function(canvas_div){


Please insert a space before the {

mdboom · 2015-01-08T12:31:52Z

Just the one minor style nit. Other than that this seems to "work for me"[TM], so 👍

blink1073 · 2015-01-11T03:52:53Z

@tacaswell, it looks like the error is tangential, perhaps just restart that one job?

tacaswell · 2015-01-11T03:54:56Z

Restarted

pelson · 2015-01-11T21:41:37Z

lib/matplotlib/backends/web_backend/mpl.js

@@ -394,9 +413,10 @@ mpl.findpos = function(e) {
 mpl.figure.prototype.mouse_event = function(event, name) {
    var canvas_pos = mpl.findpos(event)

-    if (this.focus_on_mouseover && name === 'motion_notify')
+    if (name === 'button_press')


I'm not entirely certain why motion notify is no longer important. Would you mind talking me through it? Does the status bar (with the x and y coordinates) still work with this change?

My take: we switched to a more Qt-like focus policy, in that the figure takes focus immediately and upon clicking. When the figure has focus, the status bar receives updates.

I stand corrected, the motion_notify event is triggered regardless of focus, and the coordinates are updated (I just ran the example code).

The mouse-move events fire independent of the focus state of the figure. The reason that we had to go through such contortions to get the key events to work is that IPython has a handler at the top of the document tree that eats all of the keyboard events and uses them for their own shortcuts.

er, sorry, for the repeat, my gh got logged out due to having issues with ipython + js + clearing the cache and didn't see above until I had already posted.

Js fixes for key events + ipython notebooks

pelson · 2015-01-11T21:42:44Z

Thanks @tacaswell. I merged, though I still have one outstanding question (see above re status bar).
Did you mean for this to go against master and not v1.4.x?

tacaswell · 2015-01-11T21:43:56Z

It should have been against 1.4.x, but I screwed up, I will back-port.

tacaswell · 2015-01-11T21:49:15Z

and @blink1073 should get much of the credit here

blink1073 · 2015-01-11T21:50:19Z

Yay! Where do I redeem my bitcoin-XP? 😄

pelson · 2015-01-11T21:52:14Z

Where do I redeem my bitcoin-XP?

A bar in Austin in July? 😄

blink1073 · 2015-01-11T21:52:51Z

Perfect.

WeatherGod · 2015-01-11T21:55:16Z

Say that after experiencing Austin in July...

On Sun, Jan 11, 2015 at 4:52 PM, Steven Silvester notifications@github.com
wrote:

Perfect.

—
Reply to this email directly or view it on GitHub
#3965 (comment)
.

blink1073 · 2015-01-11T21:56:14Z

I was at the conference last year, and being from Florida it was actually quite nice (less humid).

Js fixes for key events + ipython notebooks

tacaswell · 2015-01-11T22:43:44Z

backported as 893352f

tacaswell added 6 commits January 3, 2015 21:45

BUG : fix typo + change default value

db628f3

Reported by @eyurtsev

BUG : added var

465a9bc

It was unhappy without it (no idea why)

MNT : change what element has focus set

7961bad

Changed from the canvas to the canvas div Not strictly sure this is the right thing to do, but it was on the path to get things working.

MNT : terminate key handling

8427606

Don't let the handling keep bubbling up from this function. Again, not sure this is the right thing to do/if it matters.

ENH : enable keyboard events in IPython notebook

98094dd

- need to make the canvas_div 'focusable' - register the element with the keyboard manager

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

ENH : tweaks to nbagg style + mouseout event

73bd850

- remove frame around canvas_div - add mouse-out event to release focus (restoring the keyboard manager snarfing of the keyevents) when the mouse leaves the

pelson reviewed Jan 4, 2015
View reviewed changes

Set focus policy to match Qt, remove outline in webagg

7036ff8

blink1073 reviewed Jan 7, 2015
View reviewed changes

blink1073 mentioned this pull request Jan 7, 2015

Add Save Tool to NbAgg Figure [backport to 1.4.x] #3974

Merged

tacaswell added 2 commits January 7, 2015 22:03

TST : add UAT for key press events

0cdc703

MNT : improve js code

efd0c34

tacaswell added this to the v1.4.3 milestone Jan 8, 2015

mdboom reviewed Jan 8, 2015
View reviewed changes

MNT : js style tweaks

fd4d913

pelson reviewed Jan 11, 2015
View reviewed changes

pelson added a commit that referenced this pull request Jan 11, 2015

Merge pull request #3965 from tacaswell/js_fixes

1a1b82d

Js fixes for key events + ipython notebooks

pelson merged commit 1a1b82d into matplotlib:master Jan 11, 2015

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

tacaswell deleted the js_fixes branch January 11, 2015 21:43

blink1073 mentioned this pull request Jan 11, 2015

Add Support for scroll_event in WebAgg and NbAgg [backport to 1.4.x] #3968

Merged

pelson added a commit that referenced this pull request Jan 11, 2015

Merge pull request #3965 from tacaswell/js_fixes

893352f

Js fixes for key events + ipython notebooks

Uh oh!

Js fixes for key events + ipython notebooks #3965

Js fixes for key events + ipython notebooks #3965

Uh oh!

Conversation

tacaswell commented Jan 4, 2015

Uh oh!

tacaswell commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

tacaswell commented Jan 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pelson commented Jan 4, 2015

Uh oh!

tacaswell commented Jan 4, 2015

Uh oh!

WeatherGod commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

WeatherGod commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

pelson commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

blink1073 commented Jan 4, 2015

Uh oh!

tacaswell commented Jan 6, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Jan 8, 2015

Uh oh!

pelson commented Jan 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdboom commented Jan 8, 2015

Uh oh!

blink1073 commented Jan 11, 2015

Uh oh!

tacaswell commented Jan 11, 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!

pelson commented Jan 11, 2015

Uh oh!

tacaswell commented Jan 11, 2015