Keyboard nav ast node #2391

alschmiedt · 2019-04-18T16:23:46Z

The basics

[] I branched from develop
[] My pull request is against develop
My code follows the style guide

The details

Resolves

Proposed Changes

Adds an AST Node for keyboard navigation. An AST Node will be able to find the next, prev, in, or out node from its current location.

Reason for Changes

To create a better keyboard navigation api.

Test Coverage

Tests are written in mocha.

Tested on:

Additional Information

RoboErikG

Need to refresh to pull in additional changes, so expect further comments.

core/ast_node.js

RoboErikG · 2019-04-18T16:28:50Z

core/ast_node.js

+ * Class for an ASTNode.
+ * @constructor
+ * @param {!String} type This is the type of the location using
+ *     Blockly.ASTNodes.types.


remove s on ASTNodes

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T16:30:43Z

core/ast_node.js

+Blockly.ASTNode = function(type, location, params) {
+
+  /*
+   * The type of the location.


add "One of Blockly.ASTNode.types"

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T16:32:05Z

core/ast_node.js

+/**
+ * Object holding different types for an ASTNode.
+ */
+Blockly.ASTNode.types = {


Move this up to the top of the file. We usually try to declare constants first so they're easy to find.

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T16:36:16Z

core/ast_node.js

+Blockly.ASTNode.createConnectionNode = function(connection) {
+  var astNode;
+  if (!connection){return;}
+  if (connection.type === Blockly.INPUT_VALUE) {


Is there a difference between an input connection node and an input node? Do we want to enforce using Inputs for this case or silently remap it?

RoboErikG · 2019-04-18T21:43:36Z

core/ast_node.js

+ *     block.
+ * @private
+ */
+Blockly.ASTNode.prototype.findOutLocationForBlock_ = function(block) {


Please add some test cases for this method with notes on how out is expected to traverse the tree.

RoboErikG · 2019-04-18T21:47:03Z

core/ast_node.js

+
+  switch (this.type_) {
+    case Blockly.ASTNode.types.WORKSPACE:
+      var newX = this.wsCoordinate_.x + Blockly.ASTNode.wsMove_;


Add a TODO to limit this. The view is bounded to half a screen beyond the furthest blocks, but there's no easy way to translate that to Workspace coordinates right now (hence TODO).

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T21:48:05Z

core/ast_node.js

+/**
+ * Navigate between stacks of blocks on the workspace.
+ * @param {?Boolean} forward True to go forward. False to go backwards.
+ * @return {?Blockly.BlockSvg} The first block of the next stack.


For consistency this should probably return a new ASTNode.

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T21:51:14Z

core/ast_node.js

+ * @return {?Blockly.ASTNode} The ast node holding the next field or connection.
+ * @private
+ */
+Blockly.ASTNode.prototype.findNextForField_ = function(location, parentInput) {


This should be combined with findNextForInput since in both cases you need to iterate through fields and inputs until you find a valid location or hit the end of the block. Right now your code will go from an input to a field, then get stuck on the last field in that input.

You should probably add a test that iterates across a multi-input multi-field block using next and previous to make sure all editable elements are reachable.

RoboErikG · 2019-04-18T21:52:32Z

core/ast_node.js

+ * @return {?Blockly.ASTNode} The ast node holding the previous input or field.
+ * @private
+ */
+Blockly.ASTNode.prototype.findPrevForField_ = function(location, parentInput) {


Ditto about combining this with findPrevForInput_

RoboErikG · 2019-04-18T22:03:11Z

core/ast_node.js

+      break;
+
+    case Blockly.ASTNode.types.BLOCK:
+      var nextConnection = this.location_.nextConnection;


next connection may not exist.

RoboErikG · 2019-04-18T22:04:41Z

core/ast_node.js

+      break;
+
+    case Blockly.ASTNode.types.PREVIOUS:
+      var output = this.location_.outputConnection;


This is a previous, so there will never be an output. Also, this case should be identical to OUTPUT since in both cases next should go to the block itself.

RoboErikG · 2019-04-18T22:09:38Z

core/ast_node.js

+      break;
+
+    case Blockly.ASTNode.types.NEXT:
+      if (this.location_.targetBlock()) {


You can just use this.location_.targetConnection since that will either be null or the previous connection on the next block.

RoboErikG · 2019-04-18T22:12:31Z

core/ast_node.js

+Blockly.ASTNode.prototype.next = function() {
+  var newAstNode = null;
+
+  switch (this.type_) {


For each case in next/prev/in/out please add a comment about what you expect the returned node in the tree to be. For example, "If on a field next should return the next editable field or input connection on the block. It will not move between blocks and it will return null if this is the last valid field or input on the block."

RoboErikG · 2019-04-18T22:13:42Z

core/ast_node.js

+ * Find the element one level below and all the way to the left of the current
+ * location.
+ * @return {?Blockly.ASTNode} An ast node that wraps the next field, connection,
+ * workspace, or block.


Returns null if there is nothing below this node.

Done. Changes can be found in cursor_ast_update branch.

RoboErikG · 2019-04-18T22:22:01Z

core/ast_node.js

+      break;
+
+    case Blockly.ASTNode.types.OUTPUT:
+      var sourceBlock = this.location_.sourceBlock_;


In our current tree diagram there is no previous from an output. you'd have to go up a level.

RoboErikG · 2019-04-18T22:22:32Z

core/ast_node.js

+
+    case Blockly.ASTNode.types.OUTPUT:
+      var sourceBlock = this.location_.sourceBlock_;
+      if (sourceBlock && sourceBlock.previousConnection) {


A block with an output can't have a previous.

RoboErikG · 2019-04-18T22:26:21Z

core/ast_node.js

+      if (output) {
+        newAstNode = Blockly.ASTNode.createConnectionNode(output);
+      } else {
+        var prevConnection = this.location_.previousConnection;


prev may be null.

RoboErikG · 2019-04-18T22:26:41Z

core/ast_node.js

+      break;
+
+    case Blockly.ASTNode.types.PREVIOUS:
+      var prevBlock = this.location_.targetBlock();


location_.targetConnection

RoboErikG · 2019-04-18T22:27:50Z

core/ast_node.js

+ */
+Blockly.ASTNode.prototype.prev = function() {
+  if (!this.location_) {return null;}
+  var newAstNode;


Here and elsewhere you should explicitly initialize this to null. JS initializes things to undefined, which is not what we're promising to return.

alschmiedt added 4 commits April 1, 2019 09:19

Adds astnode and test

d695ce6

Fixes next,prev,out,in functionality for ast node

1626a96

Adds functionality for moving to workspace

ea6b249

Adds ast node functionality

d6b0967

alschmiedt requested a review from rachel-fenichel April 18, 2019 20:11

alschmiedt added 2 commits April 18, 2019 13:31

Added and fixed test case

5fbc88d

Updated and fixed test cases

60b76ad

rachel-fenichel approved these changes Apr 18, 2019

View reviewed changes

alschmiedt merged commit bd05929 into google:keyboard_nav Apr 18, 2019

RoboErikG reviewed Apr 18, 2019

View reviewed changes

alschmiedt mentioned this pull request Apr 25, 2019

Cursor ast update #2409

Merged

3 tasks

rachel-fenichel mentioned this pull request Jul 15, 2019

July 2019 release candidate (Q2 2019) #2644

Merged

alschmiedt deleted the keyboard_nav_astNode branch May 29, 2020 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keyboard nav ast node #2391

Keyboard nav ast node #2391

alschmiedt commented Apr 18, 2019

RoboErikG left a comment

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

alschmiedt Apr 19, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

RoboErikG Apr 18, 2019

Keyboard nav ast node #2391

Keyboard nav ast node #2391

Conversation

alschmiedt commented Apr 18, 2019

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Additional Information

RoboErikG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment