From 2b7835d02a80388a698c1a14587726d7c26aadd9 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 2 Oct 2025 14:33:30 +0300 Subject: [PATCH] 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. --- .../admin/addon/components/color-input.gjs | 12 +-- .../app/components/edit-category-general.gjs | 47 +++++++++++- .../app/controllers/edit-category/tabs.js | 13 +--- .../app/form-kit/components/fk/form.gjs | 2 +- .../app/templates/edit-category/tabs.gjs | 8 +- app/models/category.rb | 2 + config/locales/client.en.yml | 4 + spec/models/category_spec.rb | 28 +++++++ spec/system/edit_category_general_spec.rb | 73 +++++++++++++++++++ .../page_objects/components/form_kit.rb | 2 +- spec/system/page_objects/pages/category.rb | 5 ++ 11 files changed, 171 insertions(+), 25 deletions(-) create mode 100644 spec/system/edit_category_general_spec.rb diff --git a/app/assets/javascripts/admin/addon/components/color-input.gjs b/app/assets/javascripts/admin/addon/components/color-input.gjs index 0fecbc0cebc..140c47be3c9 100644 --- a/app/assets/javascripts/admin/addon/components/color-input.gjs +++ b/app/assets/javascripts/admin/addon/components/color-input.gjs @@ -32,16 +32,16 @@ export default class ColorInput extends Component { } @computed("hexValueWithFallback") - get normalizedValue() { - return this.normalize(this.hexValueWithFallback); + get valueForPicker() { + 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 { diff --git a/app/assets/javascripts/discourse/app/components/edit-category-general.gjs b/app/assets/javascripts/discourse/app/components/edit-category-general.gjs index 1c62170ba4b..65e3b89b641 100644 --- a/app/assets/javascripts/discourse/app/components/edit-category-general.gjs +++ b/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| > @@ -328,6 +369,7 @@ export default class EditCategoryGeneral extends Component { @valid={{@category.colorValid}} @ariaLabelledby="background-color-label" @onChangeColor={{fn this.updateColor field}} + @skipNormalize={{true}} /> @@ -353,7 +397,8 @@ export default class EditCategoryGeneral extends Component { validator())) { return; } - this.model.setProperties(transientData); - + this.model.setProperties(data); this.set("saving", true); this.model diff --git a/app/assets/javascripts/discourse/app/form-kit/components/fk/form.gjs b/app/assets/javascripts/discourse/app/form-kit/components/fk/form.gjs index 7ca5ebd0b85..fd9b07495e9 100644 --- a/app/assets/javascripts/discourse/app/form-kit/components/fk/form.gjs +++ b/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(); diff --git a/app/assets/javascripts/discourse/app/templates/edit-category/tabs.gjs b/app/assets/javascripts/discourse/app/templates/edit-category/tabs.gjs index e63e4a2f35c..6986a0183f2 100644 --- a/app/assets/javascripts/discourse/app/templates/edit-category/tabs.gjs +++ b/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(
@@ -119,12 +119,10 @@ export default RouteTemplate( {{/if}} - {{#if @controller.model.can_delete}} diff --git a/app/models/category.rb b/app/models/category.rb index f2cb7e81ff7..9d7b54b8ea5 100644 --- a/app/models/category.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index fa0da90369c..be3c31d0bc8 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 271badaf25a..39d979d9c84 100644 --- a/spec/models/category_spec.rb +++ b/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) } diff --git a/spec/system/edit_category_general_spec.rb b/spec/system/edit_category_general_spec.rb new file mode 100644 index 00000000000..764a303ee7e --- /dev/null +++ b/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 diff --git a/spec/system/page_objects/components/form_kit.rb b/spec/system/page_objects/components/form_kit.rb index 96ffe268037..e22a68f04ee 100644 --- a/spec/system/page_objects/components/form_kit.rb +++ b/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 diff --git a/spec/system/page_objects/pages/category.rb b/spec/system/page_objects/pages/category.rb index 5b33da95e48..77244a1fbe9 100644 --- a/spec/system/page_objects/pages/category.rb +++ b/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