FIX: Allow saving category when color attributes are 3-digits hex (#35119)

This commitreworks the validation and handling of color pickers in
the category edit page so they're simpler and more predictable. In
particular:

* It no longer attempts to "autocomplete" 3-digits hex values to the
equivalent 6-digits format -- we now accept both 3 and 6 digits formats
* It no longer disables the save button if the input color value is
invalid. The save button is always clickable, and if the admin attempts
to save with an invalid color value, the form will display errors
explaining which input is invalid and what's wrong with the color value

Internal topic: t/163062.

app/assets/javascripts/admin/addon/components/color-input.gjs

@@ -32,16 +32,16 @@ export default class ColorInput extends Component {
   }
 
   @computed("hexValueWithFallback")
-  get normalizedValue() {
+  get valueForPicker() {
-    return this.normalize(this.hexValueWithFallback);
+    return this.normalize(this.hexValueWithFallback, { forPicker: true });
   }
 
-  normalize(color) {
+  normalize(color, { forPicker = false } = {}) {
     if (this._valid(color)) {
       if (!color.startsWith("#")) {
         color = "#" + color;
       }
-      if (color.length === 4) {
+      if (color.length === 4 && (!this.skipNormalize || forPicker)) {
         color =
           "#" +
           color
@@ -109,8 +109,8 @@ export default class ColorInput extends Component {
     <input
       class="picker"
       type="color"
-      value={{this.normalizedValue}}
+      value={{this.valueForPicker}}
-      title={{this.normalizedValue}}
+      title={{this.valueForPicker}}
       {{on "input" this.onPickerInput}}
       aria-labelledby={{this.ariaLabelledby}}
     />

app/assets/javascripts/discourse/app/components/edit-category-general.gjs

@@ -129,6 +129,10 @@ export default class EditCategoryGeneral extends Component {
   updateColor(field, newColor) {
     const color = newColor.replace("#", "");
 
+    if (color === field.value) {
+      return;
+    }
+
     if (field.name === "color") {
       const whiteDiff = this.colorDifference(color, CATEGORY_TEXT_COLORS[0]);
       const blackDiff = this.colorDifference(color, CATEGORY_TEXT_COLORS[1]);
@@ -160,6 +164,41 @@ export default class EditCategoryGeneral extends Component {
     return rDiff + gDiff + bDiff;
   }
 
+  @action
+  validateColor(name, color, { addError }) {
+    color = color.trim();
+
+    let title;
+    if (name === "color") {
+      title = i18n("category.background_color");
+    } else if (name === "text_color") {
+      title = i18n("category.foreground_color");
+    } else {
+      throw new Error(`unknown title for category attribute ${name}`);
+    }
+
+    if (!color) {
+      addError(name, {
+        title,
+        message: i18n("category.color_validations.cant_be_empty"),
+      });
+    }
+
+    if (color.length !== 3 && color.length !== 6) {
+      addError(name, {
+        title,
+        message: i18n("category.color_validations.incorrect_length"),
+      });
+    }
+
+    if (!/^[0-9A-Fa-f]+$/.test(color)) {
+      addError(name, {
+        title,
+        message: i18n("category.color_validations.non_hexdecimal"),
+      });
+    }
+  }
+
   get categoryDescription() {
     if (this.args.category.description) {
       return htmlSafe(this.args.category.description);
@@ -318,6 +357,8 @@ export default class EditCategoryGeneral extends Component {
           @name="color"
           @title={{i18n "category.background_color"}}
           @format="full"
+          @validate={{this.validateColor}}
+          @validation="required"
           as |field|
         >
           <field.Custom>
@@ -328,6 +369,7 @@ export default class EditCategoryGeneral extends Component {
                   @valid={{@category.colorValid}}
                   @ariaLabelledby="background-color-label"
                   @onChangeColor={{fn this.updateColor field}}
+                  @skipNormalize={{true}}
                 />
                 <ColorPicker
                   @colors={{this.backgroundColors}}
@@ -345,6 +387,8 @@ export default class EditCategoryGeneral extends Component {
           @name="text_color"
           @title={{i18n "category.foreground_color"}}
           @format="full"
+          @validate={{this.validateColor}}
+          @validation="required"
           as |field|
         >
           <field.Custom>
@@ -353,7 +397,8 @@ export default class EditCategoryGeneral extends Component {
                 <ColorInput
                   @hexValue={{readonly field.value}}
                   @ariaLabelledby="foreground-color-label"
-                  @onBlur={{fn this.updateColor field}}
+                  @onChangeColor={{fn this.updateColor field}}
+                  @skipNormalize={{true}}
                 />
                 <ColorPicker
                   @colors={{CATEGORY_TEXT_COLORS}}

app/assets/javascripts/discourse/app/controllers/edit-category/tabs.js

@@ -61,14 +61,6 @@ export default class EditCategoryTabsController extends Controller {
       return false;
     }
 
-    if (!transientData.color) {
-      return false;
-    }
-
-    if (transientData.text_color.length < 6) {
-      return false;
-    }
-
     if (this.saving || this.deleting) {
       return true;
     }
@@ -120,13 +112,12 @@ export default class EditCategoryTabsController extends Controller {
   }
 
   @action
-  saveCategory(transientData) {
+  saveCategory(data) {
     if (this.validators.some((validator) => validator())) {
       return;
     }
 
-    this.model.setProperties(transientData);
+    this.model.setProperties(data);
-
     this.set("saving", true);
 
     this.model

app/assets/javascripts/discourse/app/form-kit/components/fk/form.gjs

@@ -225,7 +225,7 @@ class FKForm extends Component {
     try {
       this.isSubmitting = true;
 
-      await this.validate(this.fields.values());
+      await this.validate([...this.fields.values()]);
 
       if (this.formData.isValid) {
         this.formData.save();

app/assets/javascripts/discourse/app/templates/edit-category/tabs.gjs

@@ -1,4 +1,3 @@
-import { fn } from "@ember/helper";
 import { getOwner } from "@ember/owner";
 import { htmlSafe } from "@ember/template";
 import RouteTemplate from "ember-route-template";
@@ -93,6 +92,7 @@ export default RouteTemplate(
       <Form
         @data={{@controller.formData}}
         @onDirtyCheck={{@controller.isLeavingForm}}
+        @onSubmit={{@controller.saveCategory}}
         as |form transientData|
       >
         <form.Section
@@ -104,7 +104,7 @@ export default RouteTemplate(
               <Tab
                 @selectedTab={{@controller.selectedTab}}
                 @category={{@controller.model}}
-                @action={{@controller.registerValidator}}
+                @registerValidator={{@controller.registerValidator}}
                 @transientData={{transientData}}
                 @form={{form}}
               />
@@ -119,12 +119,10 @@ export default RouteTemplate(
         {{/if}}
 
         <form.Actions class="edit-category-footer">
-          <form.Button
+          <form.Submit
             @disabled={{not (@controller.canSaveForm transientData)}}
-            @action={{fn @controller.saveCategory transientData}}
             @label={{@controller.saveLabel}}
             id="save-category"
-            class="btn-primary"
           />
 
           {{#if @controller.model.can_delete}}

app/models/category.rb

@@ -104,6 +104,8 @@ class Category < ActiveRecord::Base
             },
             allow_nil: true
   validates :slug, exclusion: { in: RESERVED_SLUGS }
+  validates :color, format: { with: /\A(\h{6}|\h{3})\z/ }
+  validates :text_color, format: { with: /\A(\h{6}|\h{3})\z/ }
 
   after_create :create_category_definition
   after_destroy :trash_category_definition

config/locales/client.en.yml

@@ -4461,6 +4461,10 @@ en:
       style: "Styles"
       background_color: "Color"
       foreground_color: "Text color"
+      color_validations:
+        cant_be_empty: "can't be empty"
+        incorrect_length: "must match the format #RRGGBB or #RGB"
+        non_hexdecimal: "must only contain hexadecimal characters (0-9, A-F)"
       styles:
         type: "Style"
         icon: "Icon"

spec/models/category_spec.rb

@@ -38,6 +38,34 @@ RSpec.describe Category do
     expect(cats.errors[:name]).to be_present
   end
 
+  it "validates format of color" do
+    expect(Fabricate.build(:category, color: "fff", user:)).to be_valid
+    expect(Fabricate.build(:category, color: "1ac", user:)).to be_valid
+    expect(Fabricate.build(:category, color: "fffeee", user:)).to be_valid
+    expect(Fabricate.build(:category, color: "FF11CC", user:)).to be_valid
+    expect(Fabricate.build(:category, color: "ABC", user:)).to be_valid
+
+    expect(Fabricate.build(:category, color: "ffq1e1", user:)).to_not be_valid
+    expect(Fabricate.build(:category, color: "", user:)).to_not be_valid
+    expect(Fabricate.build(:category, color: "f", user:)).to_not be_valid
+    expect(Fabricate.build(:category, color: "21", user:)).to_not be_valid
+    expect(Fabricate.build(:category, color: "XCA", user:)).to_not be_valid
+  end
+
+  it "validates format of text_color" do
+    expect(Fabricate.build(:category, text_color: "fff", user:)).to be_valid
+    expect(Fabricate.build(:category, text_color: "1ac", user:)).to be_valid
+    expect(Fabricate.build(:category, text_color: "fffeee", user:)).to be_valid
+    expect(Fabricate.build(:category, text_color: "FF11CC", user:)).to be_valid
+    expect(Fabricate.build(:category, text_color: "ABC", user:)).to be_valid
+
+    expect(Fabricate.build(:category, text_color: "ffq1e1", user:)).to_not be_valid
+    expect(Fabricate.build(:category, text_color: "", user:)).to_not be_valid
+    expect(Fabricate.build(:category, text_color: "f", user:)).to_not be_valid
+    expect(Fabricate.build(:category, text_color: "21", user:)).to_not be_valid
+    expect(Fabricate.build(:category, text_color: "XCA", user:)).to_not be_valid
+  end
+
   describe "Associations" do
     it { is_expected.to have_one(:category_setting).dependent(:destroy) }
 

spec/system/edit_category_general_spec.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+describe "Edit Category General", type: :system do
+  fab!(:admin)
+  fab!(:category)
+  let(:category_page) { PageObjects::Pages::Category.new }
+  let(:form) { PageObjects::Components::FormKit.new(".form-kit") }
+  before { sign_in(admin) }
+
+  context "when changing background color" do
+    it "displays an error when the hex code is invalid" do
+      category_page.visit_general(category)
+
+      form.field("color").component.find("input.hex-input").fill_in(with: "ABZ")
+      category_page.save_settings
+      expect(form.field("color")).to have_errors(
+        I18n.t("js.category.color_validations.non_hexdecimal"),
+      )
+
+      form.field("color").component.find("input.hex-input").fill_in(with: "")
+      category_page.save_settings
+      expect(form.field("color")).to have_errors(
+        I18n.t("js.category.color_validations.cant_be_empty"),
+      )
+
+      form.field("color").component.find("input.hex-input").fill_in(with: "A")
+      category_page.save_settings
+      expect(form.field("color")).to have_errors(
+        I18n.t("js.category.color_validations.incorrect_length"),
+      )
+    end
+
+    it "saves successfully when the hex code is valid" do
+      category_page.visit_general(category)
+
+      form.field("color").component.find("input.hex-input").fill_in(with: "AB1")
+      category_page.save_settings
+      expect(form.field("color")).to have_no_errors
+    end
+  end
+
+  context "when changing text color" do
+    it "displays an error when the hex code is invalid" do
+      category_page.visit_general(category)
+
+      form.field("text_color").component.find("input.hex-input").fill_in(with: "ABZ")
+      category_page.save_settings
+      expect(form.field("text_color")).to have_errors(
+        I18n.t("js.category.color_validations.non_hexdecimal"),
+      )
+
+      form.field("text_color").component.find("input.hex-input").fill_in(with: "")
+      category_page.save_settings
+      expect(form.field("text_color")).to have_errors(
+        I18n.t("js.category.color_validations.cant_be_empty"),
+      )
+
+      form.field("text_color").component.find("input.hex-input").fill_in(with: "A")
+      category_page.save_settings
+      expect(form.field("text_color")).to have_errors(
+        I18n.t("js.category.color_validations.incorrect_length"),
+      )
+    end
+
+    it "saves successfully when the hex code is valid" do
+      category_page.visit_general(category)
+
+      form.field("text_color").component.find("input.hex-input").fill_in(with: "AB1")
+      category_page.save_settings
+      expect(form.field("text_color")).to have_no_errors
+    end
+  end
+end

spec/system/page_objects/components/form_kit.rb

@@ -80,7 +80,7 @@ module PageObjects
       end
 
       def has_no_errors?
-        !has_css?(".form-kit__errors")
+        has_no_css?(".form-kit__errors")
       end
 
       def control_type

spec/system/page_objects/pages/category.rb

@@ -15,6 +15,11 @@ module PageObjects
         self
       end
 
+      def visit_general(category)
+        page.visit("/c/#{category.slug}/edit/general")
+        self
+      end
+
       def visit_edit_template(category)
         page.visit("/c/#{category.slug}/edit/topic-template")
         self