Merge pull request #654 from blink1073/fix-notebook-activate

Clean up notebook focus behavior

src/notebook/index.css

@@ -28,7 +28,6 @@
 .jp-InputArea-prompt {
   flex-grow: 0;
   flex-shrink: 0;
-  padding-top: 8px !important;
 }
 
 

src/notebook/notebook/actions.ts

@@ -335,7 +335,8 @@ namespace NotebookActions {
       promises.push(Private.runCell(widget, child, kernel));
     }
     return Promise.all(promises).then(results => {
-      widget.activate();
+      // Post an update request.
+      widget.update();
       for (let result of results) {
         if (!result) {
           return false;
@@ -370,9 +371,11 @@ namespace NotebookActions {
     if (widget.activeCellIndex === widget.childCount() - 1) {
       let cell = model.factory.createCodeCell();
       model.cells.add(cell);
+      widget.activeCellIndex++;
       widget.mode = 'edit';
+    } else {
+      widget.activeCellIndex++;
     }
-    widget.activeCellIndex++;
     return promise;
   }
 

src/notebook/notebook/widget.ts

@@ -6,7 +6,11 @@ import {
 } from 'jupyter-js-services';
 
 import {
-  Message
+  find
+} from 'phosphor/lib/algorithm/searching';
+
+import {
+  sendMessage, Message
 } from 'phosphor/lib/core/messaging';
 
 import {
@@ -26,7 +30,7 @@ import {
 } from 'phosphor/lib/ui/panel';
 
 import {
-  Widget
+  Widget, WidgetMessage
 } from 'phosphor/lib/ui/widget';
 
 import {
@@ -600,6 +604,8 @@ class Notebook extends StaticNotebook {
     return this._mode;
   }
   set mode(newValue: NotebookMode) {
+    // Always post an update request.
+    this.update();
     if (newValue === this._mode) {
       return;
     }
@@ -617,17 +623,13 @@ class Notebook extends StaticNotebook {
         let widget = layout.widgets.at(i) as BaseCellWidget;
         this.deselect(widget);
       }
-      activeCell.activate();
       if (activeCell instanceof MarkdownCellWidget) {
         activeCell.rendered = false;
       }
-    } else {
-      // Take focus if the active cell is focused.
-      if (activeCell.editor.hasFocus()) {
-        this.node.focus();
+      if (!this._isActive) {
+        sendMessage(this, WidgetMessage.ActivateRequest);
       }
     }
-    this.update();
   }
 
   /**
@@ -643,6 +645,8 @@ class Notebook extends StaticNotebook {
     return this.model.cells.length ? this._activeCellIndex : -1;
   }
   set activeCellIndex(newValue: number) {
+    // Always post an update request.
+    this.update();
     let oldValue = this._activeCellIndex;
     if (!this.model || !this.model.cells.length) {
       newValue = -1;
@@ -659,11 +663,7 @@ class Notebook extends StaticNotebook {
     if (newValue === oldValue) {
       return;
     }
-    if (this.mode === 'edit' && this.activeCell) {
-      this.activeCell.activate();
-    }
     this.stateChanged.emit({ name: 'activeCellIndex', oldValue, newValue });
-    this.update();
   }
 
   /**
@@ -751,6 +751,9 @@ class Notebook extends StaticNotebook {
    * not be called directly by user code.
    */
   handleEvent(event: Event): void {
+    if (!this.model || this.model.readOnly) {
+      return;
+    }
     switch (event.type) {
     case 'mousedown':
       this._evtMouseDown(event as MouseEvent);
@@ -774,7 +777,6 @@ class Notebook extends StaticNotebook {
     this.node.addEventListener('mousedown', this);
     this.node.addEventListener('dblclick', this);
     this.node.addEventListener('focus', this, true);
-    this.update();
   }
 
   /**
@@ -790,7 +792,7 @@ class Notebook extends StaticNotebook {
    * Handle `'activate-request'` messages.
    */
   protected onActivateRequest(msg: Message): void {
-    this.node.focus();
+    this._isActive = true;
     this.update();
   }
 
@@ -798,16 +800,36 @@ class Notebook extends StaticNotebook {
    * Handle `'deactivate-request'` messages.
    */
   protected onDeactivateRequest(msg: Message): void {
+    this._isActive = false;
     this.mode = 'command';
-    this.update();
   }
 
   /**
    * Handle `update-request` messages sent to the widget.
    */
   protected onUpdateRequest(msg: Message): void {
-    // Set the appropriate classes on the cells.
     let activeCell = this.activeCell;
+    // Ensure we have the correct focus.
+    if (this._isActive) {
+      if (this.mode === 'edit' && activeCell) {
+        activeCell.editor.activate();
+      } else {
+        // Focus the node if nothing is focused internally.
+        if (!this.node.contains(document.activeElement)) {
+          this.node.focus();
+        } else {
+          // If an editor currently has focus, focus the node.
+          // Otherwise, another input field has focus and should keep it.
+          let w = find(this.layout, widget => {
+            return (widget as BaseCellWidget).editor.hasFocus();
+          });
+          if (w) {
+            this.node.focus();
+          }
+        }
+      }
+    }
+    // Set the appropriate classes on the cells.
     if (this.mode === 'edit') {
       this.addClass(EDIT_CLASS);
       this.removeClass(COMMAND_CLASS);
@@ -846,7 +868,6 @@ class Notebook extends StaticNotebook {
     cell.editor.edgeRequested.connect(this._onEdgeRequest, this);
     // Trigger an update of the active cell.
     this.activeCellIndex = this.activeCellIndex;
-    this.update();
   }
 
   /**
@@ -867,7 +888,6 @@ class Notebook extends StaticNotebook {
     if (this.isSelected(cell)) {
       this.selectionChanged.emit(void 0);
     }
-    this.update();
   }
 
   /**
@@ -928,8 +948,8 @@ class Notebook extends StaticNotebook {
    * Handle `mousedown` events for the widget.
    */
   private _evtMouseDown(event: MouseEvent): void {
-    if (!this.model || this.model.readOnly) {
-      return;
+    if (!this._isActive) {
+      sendMessage(this, WidgetMessage.ActivateRequest);
     }
     let target = event.target as HTMLElement;
     let i = this._findCell(target);
@@ -940,15 +960,18 @@ class Notebook extends StaticNotebook {
         this.mode = 'command';
       }
       // Set the cell as the active one.
+      // This must be done *after* setting the mode above.
       this.activeCellIndex = i;
     }
-    this.update();
   }
 
   /**
    * Handle `focus` events for the widget.
    */
   private _evtFocus(event: MouseEvent): void {
+    if (!this._isActive) {
+      sendMessage(this, WidgetMessage.ActivateRequest);
+    }
     let target = event.target as HTMLElement;
     let i = this._findCell(target);
     if (i !== -1) {
@@ -961,17 +984,20 @@ class Notebook extends StaticNotebook {
       } else {
         this.mode = 'command';
       }
+      this.activeCellIndex = i;
     } else {
       // No cell has focus, ensure command mode.
       this.mode = 'command';
     }
-    this.update();
   }
 
   /**
    * Handle `dblclick` events for the widget.
    */
   private _evtDblClick(event: MouseEvent): void {
+    if (!this._isActive) {
+      sendMessage(this, WidgetMessage.ActivateRequest);
+    }
     let model = this.model;
     if (!model || model.readOnly) {
       return;
@@ -996,6 +1022,7 @@ class Notebook extends StaticNotebook {
   private _activeCell: BaseCellWidget = null;
   private _inspectionHandler: InspectionHandler = null;
   private _mode: NotebookMode = 'command';
+  private _isActive = false;
 }
 
 

src/notebook/theme.css

@@ -33,7 +33,7 @@
   font-family: monospace;
   padding: 0.4em;
   text-align: right;
-  line-height: 20px;
+  line-height: 1.2em;
   font-size: 14px;
 }
 
@@ -44,12 +44,11 @@
 }
 
 
-.jp-CellEditor {
+.jp-CellEditor.p-Widget {
   border: 1px solid #cfcfcf;
   border-radius: 2px;
   background: #f7f7f7;
   line-height: 1.2em;
-  padding: 4px;
 }
 
 
@@ -59,12 +58,12 @@
 
 
 .jp-MarkdownCell.jp-mod-rendered {
-  padding-left: 100px;
+  padding-left: 90px;
 }
 
 
 .jp-MarkdownCell-renderer {
-  padding-left: 4px;
+  padding: 0.5em 0.5em 0.5em 0.4em;
 }
 
 

test/src/notebook/cells/widget.spec.ts

@@ -803,7 +803,7 @@ describe('notebook/cells/widget', () => {
     describe('#constructor()', () => {
 
       it('should create a raw cell widget', () => {
-        let widget = new RawCellWidget({renderer:CodeMirrorNotebookRenderer.defaultRawCellRenderer});
+        let widget = new RawCellWidget({renderer: CodeMirrorNotebookRenderer.defaultRawCellRenderer});
         expect(widget).to.be.a(RawCellWidget);
       });
 

test/src/notebook/notebook/widget.spec.ts

@@ -709,12 +709,12 @@ describe('notebook/notebook/widget', () => {
         Widget.attach(widget, document.body);
         widget.mode = 'edit';
         let cell = widget.childAt(widget.activeCellIndex);
-        // Notebook activates cell.
+        // Wait for update-request.
         requestAnimationFrame(() => {
-          // Cell activates editor.
+          // Notebook activates the editor.
+          expect(widget.methods).to.contain('onActivateRequest');
           requestAnimationFrame(() => {
             expect(cell.node.contains(document.activeElement)).to.be(true);
-            widget.dispose();
             done();
           });
         });
@@ -1044,13 +1044,16 @@ describe('notebook/notebook/widget', () => {
 
     describe('#onActivateRequest()', () => {
 
-      it('should focus the node', () => {
+      it('should focus the node after an update', (done) => {
         let widget = createActiveWidget();
         Widget.attach(widget, document.body);
         sendMessage(widget, WidgetMessage.ActivateRequest);
         expect(widget.methods).to.contain('onActivateRequest');
-        expect(document.activeElement).to.be(widget.node);
-        widget.dispose();
+        requestAnimationFrame(() => {
+          expect(document.activeElement).to.be(widget.node);
+          widget.dispose();
+          done();
+        });
       });
 
       it('should post an `update-request', (done) => {