From 08a141aa219339a8ce79c6969802a8aa7757ae2f Mon Sep 17 00:00:00 2001
From: Sean Lip <sll@google.com>
Date: Tue, 17 Jan 2017 15:18:54 -0800
Subject: [PATCH] Minor refactoring of the modal code (add comments, guard
 against invalid keystrokes, etc.).

---
 accessible/block-options-modal.component.js | 12 ++++++++----
 accessible/block-options-modal.service.js   |  6 +++++-
 accessible/toolbox-modal.component.js       |  9 ++++++++-
 accessible/toolbox-modal.service.js         |  8 ++++++--
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/accessible/block-options-modal.component.js b/accessible/block-options-modal.component.js
index d68a04b70..8f0d532f2 100644
--- a/accessible/block-options-modal.component.js
+++ b/accessible/block-options-modal.component.js
@@ -36,10 +36,10 @@ blocklyApp.BlockOptionsModalComponent = ng.core.Component({
         <h3 id="blockOptionsModalHeading">{{'BLOCK_OPTIONS'|translate}}</h3>
         <div role="document">
           <div class="blocklyModalButtonContainer"
-               *ngFor="#buttonInfo of actionButtonsInfo; #i=index">
-            <button [id]="getOptionId(i)"
+               *ngFor="#buttonInfo of actionButtonsInfo; #buttonIndex=index">
+            <button [id]="getOptionId(buttonIndex)"
                     (click)="buttonInfo.action(); hideModal();"
-                    [ngClass]="{activeButton: activeActionButtonIndex == i}">
+                    [ngClass]="{activeButton: activeActionButtonIndex == buttonIndex}">
               {{buttonInfo.translationIdForText|translate}}
             </button>
           </div>
@@ -110,6 +110,10 @@ blocklyApp.BlockOptionsModalComponent = ng.core.Component({
               evt.preventDefault();
               evt.stopPropagation();
 
+              if (that.activeActionButtonIndex == -1) {
+                return;
+              }
+
               var button = document.getElementById(
                   that.getOptionId(that.activeActionButtonIndex));
               if (that.activeActionButtonIndex <
@@ -150,7 +154,7 @@ blocklyApp.BlockOptionsModalComponent = ng.core.Component({
   },
   // Returns the ID for the corresponding option button.
   getOptionId: function(index) {
-    return 'modal-option-' + index;
+    return 'block-options-modal-option-' + index;
   },
   // Returns the ID for the "cancel" option button.
   getCancelOptionId: function() {
diff --git a/accessible/block-options-modal.service.js b/accessible/block-options-modal.service.js
index 82e425c05..c7b068d42 100644
--- a/accessible/block-options-modal.service.js
+++ b/accessible/block-options-modal.service.js
@@ -26,6 +26,9 @@
 blocklyApp.BlockOptionsModalService = ng.core.Class({
   constructor: [function() {
     this.actionButtonsInfo = [];
+    // The aim of the pre-show hook is to populate the modal component with the
+    // information it needs to display the modal (e.g., which action buttons to
+    // display).
     this.preShowHook = function() {
       throw Error(
           'A pre-show hook must be defined for the block options modal ' +
@@ -35,8 +38,9 @@ blocklyApp.BlockOptionsModalService = ng.core.Class({
     this.onDismissCallback = null;
   }],
   registerPreShowHook: function(preShowHook) {
+    var that = this;
     this.preShowHook = function() {
-      preShowHook(this.actionButtonsInfo, this.onDismissCallback);
+      preShowHook(that.actionButtonsInfo, that.onDismissCallback);
     };
   },
   isModalShown: function() {
diff --git a/accessible/toolbox-modal.component.js b/accessible/toolbox-modal.component.js
index 5e030db59..61dc706de 100644
--- a/accessible/toolbox-modal.component.js
+++ b/accessible/toolbox-modal.component.js
@@ -89,6 +89,8 @@ blocklyApp.ToolboxModalComponent = ng.core.Component({
           that.onSelectBlockCallback = onSelectBlockCallback;
           that.onDismissCallback = onDismissCallback;
 
+          // The indexes of the buttons corresponding to the first block in
+          // each category, as well as the 'cancel' button at the end.
           that.firstBlockIndexes = [];
           that.activeButtonIndex = -1;
           that.totalNumBlocks = 0;
@@ -126,11 +128,16 @@ blocklyApp.ToolboxModalComponent = ng.core.Component({
 
               that.focusOnOption(that.activeButtonIndex);
             },
-            // Enter key: selects an action, performs it, and closes the modal.
+            // Enter key: selects a block (or the 'Cancel' button), and closes
+            // the modal.
             '13': function(evt) {
               evt.preventDefault();
               evt.stopPropagation();
 
+              if (that.activeButtonIndex == -1) {
+                return;
+              }
+
               var button = document.getElementById(
                   that.getOptionId(that.activeButtonIndex));
 
diff --git a/accessible/toolbox-modal.service.js b/accessible/toolbox-modal.service.js
index 1f3dad430..d86ec1087 100644
--- a/accessible/toolbox-modal.service.js
+++ b/accessible/toolbox-modal.service.js
@@ -42,6 +42,9 @@ blocklyApp.ToolboxModalService = ng.core.Class({
       this.selectedToolboxCategories = null;
       this.onSelectBlockCallback = null;
       this.onDismissCallback = null;
+      // The aim of the pre-show hook is to populate the modal component with
+      // the information it needs to display the modal (e.g., which categories
+      // and blocks to display).
       this.preShowHook = function() {
         throw Error(
             'A pre-show hook must be defined for the toolbox modal before it ' +
@@ -106,10 +109,11 @@ blocklyApp.ToolboxModalService = ng.core.Class({
     });
   },
   registerPreShowHook: function(preShowHook) {
+    var that = this;
     this.preShowHook = function() {
       preShowHook(
-          this.selectedToolboxCategories, this.onSelectBlockCallback,
-          this.onDismissCallback);
+          that.selectedToolboxCategories, that.onSelectBlockCallback,
+          that.onDismissCallback);
     };
   },
   isModalShown: function() {