From f0d6fbd192e4f387839b5d41ffcf8e5d977f2d2e Mon Sep 17 00:00:00 2001
From: Beka Westberg <bekawestberg@gmail.com>
Date: Mon, 27 May 2019 14:31:15 -0700
Subject: [PATCH] Reorganized field view initialization.

---
 core/field.js                | 71 +++++++++++++++++++++++++++++-------
 core/field_angle.js          |  5 +--
 core/field_checkbox.js       | 36 +++++++++++-------
 core/field_colour.js         |  5 +--
 core/field_dropdown.js       | 62 ++++++++++++++-----------------
 core/field_image.js          | 37 -------------------
 core/field_label.js          | 30 +--------------
 core/field_textinput.js      |  8 ++--
 core/gesture.js              |  8 ++--
 core/tooltip.js              |  2 +-
 tests/jsunit/gesture_test.js |  2 +
 11 files changed, 123 insertions(+), 143 deletions(-)

diff --git a/core/field.js b/core/field.js
index 70b97f7f0..512f9451b 100644
--- a/core/field.js
+++ b/core/field.js
@@ -235,6 +235,9 @@ Blockly.Field.prototype.init = function() {
   }
   this.sourceBlock_.getSvgRoot().appendChild(this.fieldGroup_);
   this.initView();
+  this.updateEditable();
+  this.setTooltip();
+  this.bindEvents_();
   this.initModel();
 };
 
@@ -243,26 +246,54 @@ Blockly.Field.prototype.init = function() {
  * @package
  */
 Blockly.Field.prototype.initView = function() {
+  this.createBorderRect_();
+  this.createTextElement_();
+};
+
+/**
+ * Create a field border rect element. Not to be overridden by subclasses.
+ * Instead modify the result of the function inside initView, or create a
+ * separate function to call.
+ * @protected
+ */
+Blockly.Field.prototype.createBorderRect_ = function() {
   this.borderRect_ = Blockly.utils.createSvgElement('rect',
       {
         'rx': 4,
         'ry': 4,
         'x': -Blockly.BlockSvg.SEP_SPACE_X / 2,
         'y': 0,
-        'height': 16
+        'height': 16,
+        'width': this.size_.width + Blockly.BlockSvg.SEP_SPACE_X
       }, this.fieldGroup_);
+};
+
+/**
+ * Create a field text element. Not to be overridden by subclasses. Instead
+ * modify the result of the function inside initView, or create a separate
+ * function to call.
+ * @protected
+ */
+Blockly.Field.prototype.createTextElement_ = function() {
   this.textElement_ = Blockly.utils.createSvgElement('text',
-      {'class': 'blocklyText', 'y': this.size_.height - 12.5},
-      this.fieldGroup_);
-  var textNode = document.createTextNode('');
-  this.textElement_.appendChild(textNode);
+      {
+        'class': 'blocklyText',
+        'y': this.size_.height - 12.5
+      }, this.fieldGroup_);
+  this.textContent_ = document.createTextNode('');
+  this.textElement_.appendChild(this.textContent_);
+};
 
-  this.updateEditable();
-
-  this.clickTarget_ = this.getSvgRoot();
+/**
+ * Bind events to the field. Can be overridden by subclasses if they need to do
+ * custom input handling.
+ * @protected
+ */
+Blockly.Field.prototype.bindEvents_ = function() {
+  Blockly.Tooltip.bindMouseEvents(this.getClickTarget_());
   this.mouseDownWrapper_ =
       Blockly.bindEventWithChecks_(
-          this.clickTarget_, 'mousedown', this, this.onMouseDown_);
+          this.getClickTarget_(), 'mousedown', this, this.onMouseDown_);
 };
 
 /**
@@ -334,6 +365,14 @@ Blockly.Field.prototype.updateEditable = function() {
   }
 };
 
+/**
+ * Check whether this field defines the showEditor_ function.
+ * @return {boolean} Whether this field is clickable.
+ */
+Blockly.Field.prototype.isClickable = function() {
+  return !!this.showEditor_ && (typeof this.showEditor_ === 'function');
+};
+
 /**
  * Check whether this field is currently editable.  Some fields are never
  * EDITABLE (e.g. text labels). Other fields may be EDITABLE but may exist on
@@ -481,7 +520,7 @@ Blockly.Field.prototype.updateColour = function() {
  * @protected
  */
 Blockly.Field.prototype.render_ = function() {
-  this.textElement_.textContent = this.getDisplayText_();
+  this.textContent_.nodeValue = this.getDisplayText_();
   this.updateSize_();
 };
 
@@ -806,11 +845,15 @@ Blockly.Field.prototype.onMouseDown_ = function(e) {
 
 /**
  * Change the tooltip text for this field.
- * @param {string|!Element} _newTip Text for tooltip or a parent element to
- *     link to for its tooltip.
+ * @param {string|function|!Element} newTip Text for tooltip or a parent
+ *    element to link to for its tooltip.
  */
-Blockly.Field.prototype.setTooltip = function(_newTip) {
-  // Non-abstract sub-classes may wish to implement this.  See FieldLabel.
+Blockly.Field.prototype.setTooltip = function(newTip) {
+  if (!newTip && newTip !== '') {  // If null or undefined.
+    this.getClickTarget_().tooltip = this.sourceBlock_;
+  } else {
+    this.getClickTarget_().tooltip = newTip;
+  }
 };
 
 /**
diff --git a/core/field_angle.js b/core/field_angle.js
index f5f189730..ef6f90977 100644
--- a/core/field_angle.js
+++ b/core/field_angle.js
@@ -126,6 +126,7 @@ Blockly.FieldAngle.prototype.initView = function() {
   // Add the degree symbol to the left of the number, even in RTL (issue #2380)
   this.symbol_ = Blockly.utils.createSvgElement('tspan', {}, null);
   this.symbol_.appendChild(document.createTextNode('\u00B0'));
+  this.textElement_.appendChild(this.symbol_);
 };
 
 /**
@@ -133,9 +134,7 @@ Blockly.FieldAngle.prototype.initView = function() {
  * @private
  */
 Blockly.FieldAngle.prototype.render_ = function() {
-  this.textElement_.textContent = this.getDisplayText_();
-  this.textElement_.appendChild(this.symbol_);
-  this.updateSize_();
+  Blockly.FieldAngle.superClass_.render_.call(this);
   this.updateGraph_();
 };
 
diff --git a/core/field_checkbox.js b/core/field_checkbox.js
index 43f3cd6a3..cbb73933c 100644
--- a/core/field_checkbox.js
+++ b/core/field_checkbox.js
@@ -78,6 +78,20 @@ Blockly.FieldCheckbox.WIDTH = 5;
  */
 Blockly.FieldCheckbox.CHECK_CHAR = '\u2713';
 
+/**
+ * Used to correctly position the check mark.
+ * @type {number}
+ * @const
+ */
+Blockly.FieldCheckbox.CHECK_X_OFFSET = -3;
+
+/**
+ * Used to correctly position the check mark.
+ * @type {number}
+ * @const
+ */
+Blockly.FieldCheckbox.CHECK_Y_OFFSET = 14;
+
 /**
  * Serializable fields are saved by the XML renderer, non-serializable fields
  * are not. Editable fields should also be serializable.
@@ -107,19 +121,13 @@ Blockly.FieldCheckbox.prototype.isDirty_ = false;
 Blockly.FieldCheckbox.prototype.initView = function() {
   Blockly.FieldCheckbox.superClass_.initView.call(this);
 
-  // The checkbox doesn't use the inherited text element.
-  // Instead it uses a custom checkmark element that is either visible or not.
-  this.checkElement_ = Blockly.utils.createSvgElement('text',
-      {'class': 'blocklyText blocklyCheckbox', 'x': -3, 'y': 14},
-      this.fieldGroup_);
-  var textNode = document.createTextNode(Blockly.FieldCheckbox.CHECK_CHAR);
-  this.checkElement_.appendChild(textNode);
-  this.checkElement_.style.display = this.value_ ? 'block' : 'none';
+  this.textElement_.setAttribute('x', Blockly.FieldCheckbox.CHECK_X_OFFSET);
+  this.textElement_.setAttribute('y', Blockly.FieldCheckbox.CHECK_Y_OFFSET);
+  Blockly.utils.addClass(this.textElement_, 'blocklyCheckbox');
 
-  if (this.borderRect_) {
-    this.borderRect_.setAttribute('width',
-        this.size_.width + Blockly.BlockSvg.SEP_SPACE_X);
-  }
+  var textNode = document.createTextNode(Blockly.FieldCheckbox.CHECK_CHAR);
+  this.textElement_.appendChild(textNode);
+  this.textElement_.style.display = this.value_ ? 'block' : 'none';
 };
 
 /**
@@ -154,8 +162,8 @@ Blockly.FieldCheckbox.prototype.doClassValidation_ = function(newValue) {
 Blockly.FieldCheckbox.prototype.doValueUpdate_ = function(newValue) {
   this.value_ = this.convertValueToBool_(newValue);
   // Update visual.
-  if (this.checkElement_) {
-    this.checkElement_.style.display = this.value_ ? 'block' : 'none';
+  if (this.textElement_) {
+    this.textElement_.style.display = this.value_ ? 'block' : 'none';
   }
 };
 
diff --git a/core/field_colour.js b/core/field_colour.js
index c01aaafea..c76b5612f 100644
--- a/core/field_colour.js
+++ b/core/field_colour.js
@@ -153,13 +153,10 @@ Blockly.FieldColour.prototype.DROPDOWN_BACKGROUND_COLOUR = 'white';
  * @package
  */
 Blockly.FieldColour.prototype.initView = function() {
-  Blockly.FieldColour.superClass_.initView.call(this);
-
   this.size_ = new goog.math.Size(Blockly.FieldColour.DEFAULT_WIDTH,
       Blockly.FieldColour.DEFAULT_HEIGHT);
+  this.createBorderRect_();
   this.borderRect_.style['fillOpacity'] = 1;
-  this.borderRect_.setAttribute('width',
-      this.size_.width + Blockly.BlockSvg.SEP_SPACE_X);
   this.borderRect_.style.fill = this.value_;
 };
 
diff --git a/core/field_dropdown.js b/core/field_dropdown.js
index 4b9d6f850..c7eed2f96 100644
--- a/core/field_dropdown.js
+++ b/core/field_dropdown.js
@@ -95,6 +95,13 @@ Blockly.FieldDropdown.CHECKMARK_OVERHANG = 25;
  */
 Blockly.FieldDropdown.MAX_MENU_HEIGHT_VH = 0.45;
 
+/**
+ * Used to position the imageElement_ correctly.
+ * @type {number}
+ * @const
+ */
+Blockly.FieldDropdown.IMAGE_Y_OFFSET = 5;
+
 /**
  * Android can't (in 2014) display "▾", so use "▼" instead.
  */
@@ -128,11 +135,19 @@ Blockly.FieldDropdown.prototype.imageJson_ = null;
 Blockly.FieldDropdown.prototype.initView = function() {
   Blockly.FieldDropdown.superClass_.initView.call(this);
 
-  // Add dropdown arrow: "option ▾" (LTR) or "▾ אופציה" (RTL)
-  this.arrow_ = Blockly.utils.createSvgElement('tspan', {}, null);
-  this.arrow_.appendChild(document.createTextNode(this.sourceBlock_.RTL ?
-      Blockly.FieldDropdown.ARROW_CHAR + ' ' :
-      ' ' + Blockly.FieldDropdown.ARROW_CHAR));
+  this.imageElement_ = Blockly.utils.createSvgElement( 'image',
+      {
+        'y': Blockly.FieldDropdown.IMAGE_Y_OFFSET
+      }, this.fieldGroup_);
+
+  this.arrow_ = Blockly.utils.createSvgElement('tspan', {}, this.textElement_);
+  this.arrow_.appendChild(document.createTextNode(
+      Blockly.FieldDropdown.ARROW_CHAR));
+  if (this.sourceBlock_.RTL) {
+    this.textElement_.insertBefore(this.arrow_, this.textContent_);
+  } else {
+    this.textElement_.appendChild(this.arrow_);
+  }
 };
 
 /**
@@ -441,15 +456,6 @@ Blockly.FieldDropdown.prototype.updateColour = function() {
  * @private
  */
 Blockly.FieldDropdown.prototype.render_ = function() {
-  var child;
-  while ((child = this.textElement_.firstChild)) {
-    this.textElement_.removeChild(child);
-  }
-  if (this.imageElement_) {
-    Blockly.utils.removeNode(this.imageElement_);
-    this.imageElement_ = null;
-  }
-
   if (this.imageJson_) {
     this.renderSelectedImage_();
   } else {
@@ -465,20 +471,18 @@ Blockly.FieldDropdown.prototype.render_ = function() {
  * @private
  */
 Blockly.FieldDropdown.prototype.renderSelectedImage_ = function() {
-  // Image option is selected.
-  this.imageElement_ = Blockly.utils.createSvgElement('image',
-      {
-        'y': 5,
-        'height': this.imageJson_.height + 'px',
-        'width': this.imageJson_.width + 'px'
-      }, this.fieldGroup_);
   this.imageElement_.setAttributeNS(
       'http://www.w3.org/1999/xlink', 'xlink:href', this.imageJson_.src);
-  // Insert dropdown arrow.
-  this.textElement_.appendChild(this.arrow_);
+  this.imageElement_.setAttribute('height', this.imageJson_.height);
+  this.imageElement_.setAttribute('width', this.imageJson_.width);
+
   var arrowWidth = Blockly.Field.getCachedWidth(this.arrow_);
+  // TODO: Standardize sizing, need to talk to rachel and abby about rendering
+  //  redux.
+  // I think really this means plus 10?
   this.size_.height = Number(this.imageJson_.height) + 19;
   this.size_.width = Number(this.imageJson_.width) + arrowWidth;
+
   if (this.sourceBlock_.RTL) {
     this.imageElement_.setAttribute('x', arrowWidth);
     this.textElement_.setAttribute('x', -1);
@@ -493,19 +497,9 @@ Blockly.FieldDropdown.prototype.renderSelectedImage_ = function() {
  * @private
  */
 Blockly.FieldDropdown.prototype.renderSelectedText_ = function() {
-  // Text option is selected.
-  // Replace the text.
-  var textNode = document.createTextNode(this.getDisplayText_());
-  this.textElement_.appendChild(textNode);
-  // Insert dropdown arrow.
-  if (this.sourceBlock_.RTL) {
-    this.textElement_.insertBefore(this.arrow_, this.textElement_.firstChild);
-  } else {
-    this.textElement_.appendChild(this.arrow_);
-  }
+  this.textContent_.nodeValue = this.getDisplayText_();
   this.textElement_.setAttribute('text-anchor', 'start');
   this.textElement_.setAttribute('x', 0);
-
   this.size_.height = Blockly.BlockSvg.MIN_BLOCK_Y;
   this.size_.width = Blockly.Field.getCachedWidth(this.textElement_);
 };
diff --git a/core/field_image.js b/core/field_image.js
index 44bc0c2b2..9188935d6 100644
--- a/core/field_image.js
+++ b/core/field_image.js
@@ -66,7 +66,6 @@ Blockly.FieldImage = function(src, width, height,
       this.height_ + 2 * Blockly.BlockSvg.INLINE_PADDING_Y);
 
   this.flipRtl_ = opt_flipRtl;
-  this.tooltip_ = '';
   this.text_ = opt_alt || '';
   this.setValue(src || '');
 
@@ -127,17 +126,6 @@ Blockly.FieldImage.prototype.initView = function() {
       this.fieldGroup_);
   this.imageElement_.setAttributeNS('http://www.w3.org/1999/xlink',
       'xlink:href', this.value_);
-  this.sourceBlock_.getSvgRoot().appendChild(this.fieldGroup_);
-
-  if (this.tooltip_) {
-    this.imageElement_.tooltip = this.tooltip_;
-  } else {
-    // Configure the field to be transparent with respect to tooltips.
-    this.setTooltip(this.sourceBlock_);
-  }
-  Blockly.Tooltip.bindMouseEvents(this.imageElement_);
-
-  this.maybeAddClickHandler_();
 };
 
 /**
@@ -151,31 +139,6 @@ Blockly.FieldImage.prototype.dispose = function() {
   this.imageElement_ = null;
 };
 
-/**
- * Bind events for a mouse down on the image, but only if a click handler has
- * been defined.
- * @private
- */
-Blockly.FieldImage.prototype.maybeAddClickHandler_ = function() {
-  if (this.clickHandler_) {
-    this.mouseDownWrapper_ =
-        Blockly.bindEventWithChecks_(
-            this.fieldGroup_, 'mousedown', this, this.clickHandler_);
-  }
-};
-
-/**
- * Change the tooltip text for this field.
- * @param {string|!Element} newTip Text for tooltip or a parent element to
- *     link to for its tooltip.
- */
-Blockly.FieldImage.prototype.setTooltip = function(newTip) {
-  this.tooltip_ = newTip;
-  if (this.imageElement_) {
-    this.imageElement_.tooltip = newTip;
-  }
-};
-
 /**
  * Ensure that the input value (the source URL) is a string.
  * @param {string=} newValue The input value
diff --git a/core/field_label.js b/core/field_label.js
index f287964e0..b752f74bb 100644
--- a/core/field_label.js
+++ b/core/field_label.js
@@ -50,7 +50,6 @@ Blockly.FieldLabel = function(opt_value, opt_class) {
     opt_value = '';
   }
   this.setValue(opt_value);
-  this.tooltip_ = '';
 };
 goog.inherits(Blockly.FieldLabel, Blockly.Field);
 
@@ -80,24 +79,11 @@ Blockly.FieldLabel.prototype.EDITABLE = false;
  * @package
  */
 Blockly.FieldLabel.prototype.initView = function() {
-  this.textElement_ = Blockly.utils.createSvgElement('text',
-      {
-        'class': 'blocklyText',
-        'y': this.size_.height - 5
-      }, this.fieldGroup_);
-  var textNode = document.createTextNode('');
-  this.textElement_.appendChild(textNode);
+  this.createTextElement_();
+  this.textElement_.setAttribute('y', this.size_.height - 5);
   if (this.class_) {
     Blockly.utils.addClass(this.textElement_, this.class_);
   }
-
-  if (this.tooltip_) {
-    this.textElement_.tooltip = this.tooltip_;
-  } else {
-    // Configure the field to be transparent with respect to tooltips.
-    this.textElement_.tooltip = this.sourceBlock_;
-  }
-  Blockly.Tooltip.bindMouseEvents(this.textElement_);
 };
 
 /**
@@ -123,16 +109,4 @@ Blockly.FieldLabel.prototype.doClassValidation_ = function(newValue) {
   return String(newValue);
 };
 
-/**
- * Change the tooltip text for this field.
- * @param {string|!Element} newTip Text for tooltip or a parent element to
- *     link to for its tooltip.
- */
-Blockly.FieldLabel.prototype.setTooltip = function(newTip) {
-  this.tooltip_ = newTip;
-  if (this.textElement_) {
-    this.textElement_.tooltip = newTip;
-  }
-};
-
 Blockly.Field.register('field_label', Blockly.FieldLabel);
diff --git a/core/field_textinput.js b/core/field_textinput.js
index 7e3e8e65f..116309bb6 100644
--- a/core/field_textinput.js
+++ b/core/field_textinput.js
@@ -263,7 +263,7 @@ Blockly.FieldTextInput.prototype.showInlineEditor_ = function(quietInput) {
   }
 
   // Bind events.
-  this.bindEvents_(htmlInput);
+  this.bindInputEvents_(htmlInput);
 
   // Save it.
   Blockly.FieldTextInput.htmlInput_ = htmlInput;
@@ -275,7 +275,7 @@ Blockly.FieldTextInput.prototype.showInlineEditor_ = function(quietInput) {
  *     which event handlers will be bound.
  * @private
  */
-Blockly.FieldTextInput.prototype.bindEvents_ = function(htmlInput) {
+Blockly.FieldTextInput.prototype.bindInputEvents_ = function(htmlInput) {
   // Bind to keydown -- trap Enter without IME and Esc to hide.
   htmlInput.onKeyDownWrapper_ =
       Blockly.bindEventWithChecks_(
@@ -297,7 +297,7 @@ Blockly.FieldTextInput.prototype.bindEvents_ = function(htmlInput) {
  * @param {!HTMLInputElement} htmlInput The html for this text input.
  * @private
  */
-Blockly.FieldTextInput.prototype.unbindEvents_ = function(htmlInput) {
+Blockly.FieldTextInput.prototype.unbindInputEvents_ = function(htmlInput) {
   Blockly.unbindEvent_(htmlInput.onKeyDownWrapper_);
   Blockly.unbindEvent_(htmlInput.onKeyUpWrapper_);
   Blockly.unbindEvent_(htmlInput.onKeyPressWrapper_);
@@ -413,7 +413,7 @@ Blockly.FieldTextInput.prototype.widgetDispose_ = function() {
     }
 
     // Remove htmlInput.
-    thisField.unbindEvents_(htmlInput);
+    thisField.unbindInputEvents_(htmlInput);
     Blockly.FieldTextInput.htmlInput_ = null;
 
     // Delete style properties.
diff --git a/core/gesture.js b/core/gesture.js
index 990667e22..ebf67584a 100644
--- a/core/gesture.js
+++ b/core/gesture.js
@@ -899,10 +899,10 @@ Blockly.Gesture.prototype.isBlockClick_ = function() {
  * @private
  */
 Blockly.Gesture.prototype.isFieldClick_ = function() {
-  var fieldEditable = this.startField_ ?
-      this.startField_.isCurrentlyEditable() : false;
-  return fieldEditable && !this.hasExceededDragRadius_ && (!this.flyout_ ||
-    !this.flyout_.autoClose);
+  var fieldClickable = this.startField_ ?
+      this.startField_.isClickable() : false;
+  return fieldClickable && !this.hasExceededDragRadius_ &&
+      (!this.flyout_ || !this.flyout_.autoClose);
 };
 
 /**
diff --git a/core/tooltip.js b/core/tooltip.js
index a423f31dc..06d22f85f 100644
--- a/core/tooltip.js
+++ b/core/tooltip.js
@@ -165,7 +165,7 @@ Blockly.Tooltip.onMouseOver_ = function(e) {
   }
   // If the tooltip is an object, treat it as a pointer to the next object in
   // the chain to look at.  Terminate when a string or function is found.
-  var element = e.target;
+  var element = e.currentTarget;
   while ((typeof element.tooltip != 'string') &&
          (typeof element.tooltip != 'function')) {
     element = element.tooltip;
diff --git a/tests/jsunit/gesture_test.js b/tests/jsunit/gesture_test.js
index 19ba1864e..5642c4e40 100644
--- a/tests/jsunit/gesture_test.js
+++ b/tests/jsunit/gesture_test.js
@@ -49,6 +49,7 @@ function test_gestureIsField_ClickInWorkspace() {
   var block = new Blockly.Block(workspace);
   var field = new Blockly.Field();
   field.setSourceBlock(block);
+  field.showEditor_ = function() {};
   var gesture = new Blockly.Gesture(e, workspace);
   gesture.setStartField(field);
 
@@ -64,6 +65,7 @@ function gestureIsFieldClick_InFlyoutHelper(flyout, expectedResult){
   var block = new Blockly.Block(workspace);
   var field = new Blockly.Field();
   field.setSourceBlock(block);
+  field.showEditor_ = function() {};
   // Create gesture from the flyout
   var gesture = new Blockly.Gesture(e, workspace.flyout_);
   // Populate gesture with click start information