Partial code review of nav (#3091)

* Partial code review of nav

Had a few minutes free.

* Move getTopStackBlock to block

This is a generally useful function.

core/block.js

@@ -616,6 +616,21 @@ Blockly.Block.prototype.getRootBlock = function() {
   return rootBlock;
 };
 
+/**
+ * Walk up from the given block up through the stack of blocks to find
+ * the top block of the sub stack. If we are nested in a statement input only
+ * find the top-most nested block. Do not go all the way to the root block.
+ * @return {!Blockly.Block} The top block in a stack.
+ * @private
+ */
+Blockly.Block.prototype.getTopStackBlock = function() {
+  var block = this;
+  do {
+    var previous = block.getPreviousBlock();
+  } while (previous && previous.getNextBlock() == block && (block = previous));
+  return block;
+};
+
 /**
  * Find all the blocks that are directly nested inside this one.
  * Includes value and statement inputs, as well as any following statement.

core/insertion_marker_manager.js

@@ -494,7 +494,7 @@ Blockly.InsertionMarkerManager.prototype.maybeShowPreview_ = function(candidate)
   // Something went wrong and we're trying to connect to an invalid connection.
   if (closest == this.closestConnection_ ||
       closest.sourceBlock_.isInsertionMarker()) {
-    console.log("trying to connect to an insertion marker");
+    console.log('Trying to connect to an insertion marker');
     return;
   }
   // Add an insertion marker or replacement marker.

core/keyboard_nav/ast_node.js

@@ -31,12 +31,12 @@ goog.provide('Blockly.ASTNode');
  * Class for an AST node.
  * It is recommended that you use one of the createNode methods instead of
  * creating a node directly.
- * @constructor
  * @param {string} type The type of the location.
  *     Must be in Bockly.ASTNode.types.
  * @param {Blockly.Block|Blockly.Connection|Blockly.Field|Blockly.Workspace}
  *     location The position in the AST.
  * @param {!Object=} opt_params Optional dictionary of options.
+ * @constructor
  */
 Blockly.ASTNode = function(type, location, opt_params) {
   if (!location) {
@@ -110,8 +110,8 @@ Blockly.ASTNode.isConnectionType_ = function(type) {
 
 /**
  * Create an AST node pointing to a field.
- * @param {Blockly.Field} field The location of the AST node.
- * @return {Blockly.ASTNode} An AST node pointing to a field.
+ * @param {!Blockly.Field} field The location of the AST node.
+ * @return {!Blockly.ASTNode} An AST node pointing to a field.
  */
 Blockly.ASTNode.createFieldNode = function(field) {
   return new Blockly.ASTNode(Blockly.ASTNode.types.FIELD, field);
@@ -128,16 +128,16 @@ Blockly.ASTNode.createConnectionNode = function(connection) {
   if (!connection) {
     return null;
   }
-  if (connection.type === Blockly.INPUT_VALUE) {
+  if (connection.type == Blockly.INPUT_VALUE) {
     return Blockly.ASTNode.createInputNode(connection.getParentInput());
-  } else if (connection.type === Blockly.NEXT_STATEMENT &&
+  } else if (connection.type == Blockly.NEXT_STATEMENT &&
       connection.getParentInput()) {
     return Blockly.ASTNode.createInputNode(connection.getParentInput());
-  } else if (connection.type === Blockly.NEXT_STATEMENT) {
+  } else if (connection.type == Blockly.NEXT_STATEMENT) {
     return new Blockly.ASTNode(Blockly.ASTNode.types.NEXT, connection);
-  } else if (connection.type === Blockly.OUTPUT_VALUE) {
+  } else if (connection.type == Blockly.OUTPUT_VALUE) {
     return new Blockly.ASTNode(Blockly.ASTNode.types.OUTPUT, connection);
-  } else if (connection.type === Blockly.PREVIOUS_STATEMENT) {
+  } else if (connection.type == Blockly.PREVIOUS_STATEMENT) {
     return new Blockly.ASTNode(Blockly.ASTNode.types.PREVIOUS, connection);
   }
   return null;
@@ -147,7 +147,7 @@ Blockly.ASTNode.createConnectionNode = function(connection) {
  * Creates an AST node pointing to an input. Stores the input connection as the
  *     location.
  * @param {Blockly.Input} input The input used to create an AST node.
- * @return {Blockly.ASTNode} An AST node pointing to a input.
+ * @return {!Blockly.ASTNode} An AST node pointing to a input.
  */
 Blockly.ASTNode.createInputNode = function(input) {
   if (!input) {
@@ -158,8 +158,8 @@ Blockly.ASTNode.createInputNode = function(input) {
 
 /**
  * Creates an AST node pointing to a block.
- * @param {Blockly.Block} block The block used to create an AST node.
- * @return {Blockly.ASTNode} An AST node pointing to a block.
+ * @param {!Blockly.Block} block The block used to create an AST node.
+ * @return {!Blockly.ASTNode} An AST node pointing to a block.
  */
 Blockly.ASTNode.createBlockNode = function(block) {
   return new Blockly.ASTNode(Blockly.ASTNode.types.BLOCK, block);
@@ -168,9 +168,9 @@ Blockly.ASTNode.createBlockNode = function(block) {
 /**
  * Create an AST node of type stack. A stack, represented by its top block, is
  *     the set of all blocks connected to a top block, including the top block.
- * @param {Blockly.Block} topBlock A top block has no parent and can be found
+ * @param {!Blockly.Block} topBlock A top block has no parent and can be found
  *     in the list returned by workspace.getTopBlocks().
- * @return {Blockly.ASTNode} An AST node of type stack that points to the top
+ * @return {!Blockly.ASTNode} An AST node of type stack that points to the top
  *     block on the stack.
  */
 Blockly.ASTNode.createStackNode = function(topBlock) {
@@ -179,15 +179,15 @@ Blockly.ASTNode.createStackNode = function(topBlock) {
 
 /**
  * Creates an AST node pointing to a workspace.
- * @param {Blockly.Workspace} workspace The workspace that we are on.
+ * @param {!Blockly.Workspace} workspace The workspace that we are on.
  * @param {Blockly.utils.Coordinate} wsCoordinate The position on the workspace
  *     for this node.
- * @return {Blockly.ASTNode} An AST node pointing to a workspace and a position
+ * @return {!Blockly.ASTNode} An AST node pointing to a workspace and a position
  *     on the workspace.
  */
 Blockly.ASTNode.createWorkspaceNode = function(workspace, wsCoordinate) {
   var params = {
-    "wsCoordinate": wsCoordinate
+    wsCoordinate: wsCoordinate
   };
   return new Blockly.ASTNode(
       Blockly.ASTNode.types.WORKSPACE, workspace, params);
@@ -202,8 +202,8 @@ Blockly.ASTNode.prototype.processParams_ = function(params) {
   if (!params) {
     return;
   }
-  if (params['wsCoordinate']) {
-    this.wsCoordinate_ = params['wsCoordinate'];
+  if (params.wsCoordinate) {
+    this.wsCoordinate_ = params.wsCoordinate;
   }
 };
 
@@ -262,7 +262,7 @@ Blockly.ASTNode.prototype.findPreviousEditableField_ = function(location,
   var fieldRow = parentInput.fieldRow;
   var fieldIdx = fieldRow.indexOf(location);
   var previousField = null;
-  var startIdx = opt_last ? fieldRow.length - 1 : fieldIdx - 1;
+  var startIdx = (opt_last ? fieldRow.length : fieldIdx) - 1;
   for (var i = startIdx, field; field = fieldRow[i]; i--) {
     if (field.EDITABLE) {
       previousField = field;
@@ -413,8 +413,7 @@ Blockly.ASTNode.prototype.navigateBetweenStacks_ = function(forward) {
       return Blockly.ASTNode.createStackNode(topBlocks[resultIndex]);
     }
   }
-  throw Error('Couldn\'t find ' + (forward ? 'next' : 'previous') +
-      ' stack?!?!?!');
+  throw Error('Couldn\'t find ' + (forward ? 'next' : 'previous') + ' stack?!');
 };
 
 /**
@@ -447,14 +446,10 @@ Blockly.ASTNode.prototype.getOutAstNodeForBlock_ = function(block) {
   if (!block) {
     return null;
   }
-  var topBlock = null;
+  var topBlock;
   // If the block doesn't have a previous connection then it is the top of the
   // substack.
-  if (!block.previousConnection) {
-    topBlock = block;
-  } else {
-    topBlock = this.findTopOfSubStack_(block);
-  }
+  topBlock = block.getTopStackBlock();
   var topConnection = topBlock.previousConnection || topBlock.outputConnection;
   // If the top connection has a parentInput, create an AST node pointing to
   // that input.
@@ -491,26 +486,6 @@ Blockly.ASTNode.prototype.findFirstFieldOrInput_ = function(block) {
   return null;
 };
 
-/**
- * Walk backwards from the given block up through the stack of blocks to find
- * the top block of the sub stack. If we are nested in a statement input only
- * find the top most nested block. Do not go all the way to the top of the
- * stack.
- * @param {!Blockly.Block} sourceBlock A block in the stack.
- * @return {!Blockly.Block} The top block in a stack.
- * @private
- */
-Blockly.ASTNode.prototype.findTopOfSubStack_ = function(sourceBlock) {
-  var topBlock = sourceBlock;
-  while (topBlock && topBlock.previousConnection &&
-      topBlock.previousConnection.targetConnection &&
-      topBlock.previousConnection.targetBlock().nextConnection ==
-      topBlock.previousConnection.targetConnection) {
-    topBlock = topBlock.previousConnection.targetBlock();
-  }
-  return topBlock;
-};
-
 /**
  * Find the element to the right of the current element in the AST.
  * @return {Blockly.ASTNode} An AST node that wraps the next field, connection,

core/keyboard_nav/cursor.js

@@ -88,7 +88,7 @@ Blockly.Cursor.prototype.setCurNode = function(newNode) {
 };
 
 /**
- * Hide the cursor svg.
+ * Hide the cursor SVG.
  */
 Blockly.Cursor.prototype.hide = function() {
   if (this.drawer_) {
@@ -109,8 +109,8 @@ Blockly.Cursor.prototype.next = function() {
 
   var newNode = curNode.next();
   while (newNode && newNode.next() &&
-    (newNode.getType() === Blockly.ASTNode.types.NEXT ||
-    newNode.getType() === Blockly.ASTNode.types.BLOCK)) {
+    (newNode.getType() == Blockly.ASTNode.types.NEXT ||
+    newNode.getType() == Blockly.ASTNode.types.BLOCK)) {
     newNode = newNode.next();
   }
 
@@ -132,13 +132,13 @@ Blockly.Cursor.prototype.in = function() {
   }
   // If we are on a previous or output connection, go to the block level before
   // performing next operation.
-  if (curNode.getType() === Blockly.ASTNode.types.PREVIOUS ||
-    curNode.getType() === Blockly.ASTNode.types.OUTPUT) {
+  if (curNode.getType() == Blockly.ASTNode.types.PREVIOUS ||
+    curNode.getType() == Blockly.ASTNode.types.OUTPUT) {
     curNode = curNode.next();
   }
   var newNode = curNode.in();
 
-  if (newNode && newNode.getType() === Blockly.ASTNode.types.OUTPUT) {
+  if (newNode && newNode.getType() == Blockly.ASTNode.types.OUTPUT) {
     newNode = newNode.next() || newNode;
   }
 
@@ -161,8 +161,8 @@ Blockly.Cursor.prototype.prev = function() {
   var newNode = curNode.prev();
 
   while (newNode && newNode.prev() &&
-    (newNode.getType() === Blockly.ASTNode.types.NEXT ||
-    newNode.getType() === Blockly.ASTNode.types.BLOCK)) {
+    (newNode.getType() == Blockly.ASTNode.types.NEXT ||
+    newNode.getType() == Blockly.ASTNode.types.BLOCK)) {
     newNode = newNode.prev();
   }
 
@@ -183,8 +183,8 @@ Blockly.Cursor.prototype.out = function() {
     return null;
   }
   var newNode = curNode.out();
-  
-  if (newNode && newNode.getType() === Blockly.ASTNode.types.BLOCK) {
+
+  if (newNode && newNode.getType() == Blockly.ASTNode.types.BLOCK) {
     newNode = newNode.prev() || newNode;
   }
 

core/keyboard_nav/cursor_svg.js

@@ -56,7 +56,7 @@ Blockly.CursorSvg = function(workspace, opt_marker) {
   this.isMarker_ = opt_marker;
 
   /**
-   * The workspace, field, or block that the cursor svg element should be
+   * The workspace, field, or block that the cursor SVG element should be
    * attached to.
    * @type {Blockly.WorkspaceSvg|Blockly.Field|Blockly.BlockSvg}
    * @private
@@ -136,13 +136,13 @@ Blockly.CursorSvg.CURSOR_COLOR = '#cc0a0a';
 Blockly.CursorSvg.MARKER_COLOR = '#4286f4';
 
 /**
- * The name of the css class for a cursor.
+ * The name of the CSS class for a cursor.
  * @const {string}
  */
 Blockly.CursorSvg.CURSOR_CLASS = 'blocklyCursor';
 
 /**
- * The name of the css class for a marker.
+ * The name of the CSS class for a marker.
  * @const {string}
  */
 Blockly.CursorSvg.MARKER_CLASS = 'blocklyMarker';
@@ -180,9 +180,9 @@ Blockly.CursorSvg.prototype.createDom = function() {
 };
 
 /**
- * Attaches the svg root of the cursor to the svg group of the parent.
+ * Attaches the SVG root of the cursor to the SVG group of the parent.
  * @param {!Blockly.WorkspaceSvg|!Blockly.Field|!Blockly.BlockSvg} newParent
- *    The workspace, field, or block that the cursor svg element should be
+ *    The workspace, field, or block that the cursor SVG element should be
  *    attached to.
  * @private
  */
@@ -358,9 +358,9 @@ Blockly.CursorSvg.prototype.showCurrent_ = function() {
  */
 Blockly.CursorSvg.prototype.positionBlock_ = function(width, cursorOffset, cursorHeight) {
   var cursorPath = Blockly.utils.svgPaths.moveBy(-1 * cursorOffset, cursorHeight) +
-    Blockly.utils.svgPaths.lineOnAxis('V', -1 * cursorOffset) +
-    Blockly.utils.svgPaths.lineOnAxis('H', width + cursorOffset * 2) +
-    Blockly.utils.svgPaths.lineOnAxis('V', cursorHeight);
+      Blockly.utils.svgPaths.lineOnAxis('V', -1 * cursorOffset) +
+      Blockly.utils.svgPaths.lineOnAxis('H', width + cursorOffset * 2) +
+      Blockly.utils.svgPaths.lineOnAxis('V', cursorHeight);
   this.cursorBlock_.setAttribute('d', cursorPath);
   if (this.workspace_.RTL) {
     this.flipRtl_(this.cursorBlock_);
@@ -426,7 +426,7 @@ Blockly.CursorSvg.prototype.positionOutput_ = function(width, height) {
 /**
  * Position the cursor for a previous connection.
  * Displays a half rectangle with a notch in the top to represent the previous
- * conenction.
+ * connection.
  * @param {number} width The width of the block.
  * @param {number} cursorOffset The offset of the cursor from around the block.
  * @param {number} cursorHeight The height of the cursor.
@@ -434,11 +434,11 @@ Blockly.CursorSvg.prototype.positionOutput_ = function(width, height) {
  */
 Blockly.CursorSvg.prototype.positionPrevious_ = function(width, cursorOffset, cursorHeight) {
   var cursorPath = Blockly.utils.svgPaths.moveBy(-1 * cursorOffset, cursorHeight) +
-    Blockly.utils.svgPaths.lineOnAxis('V', -1 * cursorOffset) +
-    Blockly.utils.svgPaths.lineOnAxis('H', this.constants_.NOTCH_OFFSET_LEFT) +
-    this.constants_.NOTCH.pathLeft +
-    Blockly.utils.svgPaths.lineOnAxis('H', width + cursorOffset * 2) +
-    Blockly.utils.svgPaths.lineOnAxis('V', cursorHeight);
+      Blockly.utils.svgPaths.lineOnAxis('V', -1 * cursorOffset) +
+      Blockly.utils.svgPaths.lineOnAxis('H', this.constants_.NOTCH_OFFSET_LEFT) +
+      this.constants_.NOTCH.pathLeft +
+      Blockly.utils.svgPaths.lineOnAxis('H', width + cursorOffset * 2) +
+      Blockly.utils.svgPaths.lineOnAxis('V', cursorHeight);
   this.cursorBlock_.setAttribute('d', cursorPath);
   if (this.workspace_.RTL) {
     this.flipRtl_(this.cursorBlock_);

tests/mocha/astnode_test.js

@@ -157,12 +157,6 @@ suite('ASTNode', function() {
       var newASTNode = node.navigateBetweenStacks_(false);
       assertEquals(newASTNode.getLocation(), this.blocks.statementInput1);
     });
-    test('findTopOfSubStack_', function() {
-      var node = new Blockly.ASTNode(
-          Blockly.ASTNode.types.BLOCK, this.blocks.statementInput4);
-      var block = node.findTopOfSubStack_(this.blocks.statementInput4);
-      assertEquals(block, this.blocks.statementInput4);
-    });
     test('getOutAstNodeForBlock_', function() {
       var node = new Blockly.ASTNode(
           Blockly.ASTNode.types.BLOCK, this.blocks.statementInput2);