Audit fields' behavior when setting invalid values

[RoboErikG]

Problem statement

In general, trying to set a field with an invalid value should not change the current value of the field. There are several fields that instead fall back to the default if they can't interpret the value passed in.

There are some fields where invalid values should cause errors, such as field images. Please add comments to this issue to clarify any field specific bad value handling.

Fields to review:

• angle
• checkbox
• colour
• date
• dropdown
• image
• label
• label serializable
• number
• text input
• variable

[RoboErikG]

Image specific behavior

Images should throw an error if the width or height are invalid. The src is allowed to be invalid as the browser will handle it as a broken image.

[BeksOmega]

So after #2441 all fields should work, but I think the code can be reorganized to make it better.

Almost all field constructors look like this:

  opt_value = this.doClassValidation_(opt_value);
  if (opt_value === null) {
    opt_value = 0;
  }
  Blockly.FieldAngle.superClass_.constructor.call(
      this, opt_value, opt_validator);

The problem with this is that the super-constructor calls setValue which calls doClassValidation_ which means doClassValidation_ is getting called twice.

Proposal

Inside setValue, if doClassValidation_ returns null, and the current value is also null, use a getClassDefaultValue function to set the value. This way (almost) all field constructors would just look like this:

  Blockly.FieldAngle.superClass_.constructor.call(
      this, opt_value, opt_validator);

Making the code more organized for external developers, and more efficient.

Blockly.Field.prototype.setValue = function(newValue) {
  var oldValue = this.getValue();
  if (newValue === null) {
    // Not a valid value to check.
    return;
  }

  newValue = this.doClassValidation_(newValue);
  if (newValue === null) {
    if (oldValue === null) {
      // The field's start value isn't valid, use the default.
      newValue = this.getClassDefaultValue();
    } else {
      this.doValueInvalid_();
      if (this.isDirty_) {
        this.forceRerender();
      }
      return;
    }
  }
  // etc...
}

Oh and also any newValue === null checks could be removed from class validators.

Additional Information

This may be overkill hehe.

[rachel-fenichel]

@BeksOmega can you check off any of these that you've already fixed, and comment on any that you know have weird behaviour?

[BeksOmega]

@rachel-fenichel As far as I know, all fields are functioning correctly. Each field type has a test suite for "Value -> New Value" which covers this issue (I went back and checked). All of those tests pass.

Sadly I cannot check off the boxes because I do not have perms :/