Fix DirtySubmit handling of checkbox and radio inputs

Most browsers do not fire the "input" event for checkboxes or radios.
Adds a "change" listener to properly trigger these DirtySubmit updates.

app/assets/javascripts/dirty_submit/dirty_submit_form.js

@@ -25,15 +25,16 @@ class DirtySubmitForm {
       DirtySubmitForm.THROTTLE_DURATION,
     );
     this.form.addEventListener('input', throttledUpdateDirtyInput);
+    this.form.addEventListener('change', throttledUpdateDirtyInput);
     this.form.addEventListener('submit', event => this.formSubmit(event));
   }
 
   updateDirtyInput(event) {
-    const input = event.target;
+    const { target } = event;
 
-    if (!input.dataset.isDirtySubmitInput) return;
+    if (!target.dataset.isDirtySubmitInput) return;
 
-    this.updateDirtyInputs(input);
+    this.updateDirtyInputs(target);
     this.toggleSubmission();
   }
 

changelogs/unreleased/53856-changing-group-visibility-does-not-re-enable-save-button.yml

@@ -0,0 +1,6 @@
+---
+title: Fix suboptimal handling of checkbox and radio input events causing
+  group general settings submit button to stay disabled after changing its visibility
+merge_request: 23022
+author:
+type: fixed

spec/features/groups_spec.rb

@@ -154,7 +154,7 @@ describe 'Group' do
   end
 
   describe 'group edit', :js do
-    let(:group) { create(:group) }
+    let(:group) { create(:group, :public) }
     let(:path)  { edit_group_path(group) }
     let(:new_name) { 'new-name' }
 
@@ -163,6 +163,8 @@ describe 'Group' do
     end
 
     it_behaves_like 'dirty submit form', [{ form: '.js-general-settings-form', input: 'input[name="group[name]"]' },
+                                          { form: '.js-general-settings-form', input: '#group_visibility_level_0' },
+                                          { form: '.js-general-permissions-form', input: '#group_request_access_enabled' },
                                           { form: '.js-general-permissions-form', input: 'input[name="group[two_factor_grace_period]"]' }]
 
     it 'saves new settings' do

spec/javascripts/dirty_submit/dirty_submit_collection_spec.js

@@ -1,5 +1,5 @@
 import DirtySubmitCollection from '~/dirty_submit/dirty_submit_collection';
-import { setInput, createForm } from './helper';
+import { setInputValue, createForm } from './helper';
 
 describe('DirtySubmitCollection', () => {
   it('disables submits until there are changes', done => {
@@ -14,11 +14,11 @@ describe('DirtySubmitCollection', () => {
 
       expect(submit.disabled).toBe(true);
 
-      return setInput(input, `${originalValue} changes`)
+      return setInputValue(input, `${originalValue} changes`)
         .then(() => {
           expect(submit.disabled).toBe(false);
         })
-        .then(() => setInput(input, originalValue))
+        .then(() => setInputValue(input, originalValue))
         .then(() => {
           expect(submit.disabled).toBe(true);
         })

spec/javascripts/dirty_submit/dirty_submit_form_spec.js

@@ -1,14 +1,14 @@
 import DirtySubmitForm from '~/dirty_submit/dirty_submit_form';
-import { setInput, createForm } from './helper';
+import { getInputValue, setInputValue, createForm } from './helper';
 
 function expectToToggleDisableOnDirtyUpdate(submit, input) {
-  const originalValue = input.value;
+  const originalValue = getInputValue(input);
 
   expect(submit.disabled).toBe(true);
 
-  return setInput(input, `${originalValue} changes`)
+  return setInputValue(input, `${originalValue} changes`)
     .then(() => expect(submit.disabled).toBe(false))
-    .then(() => setInput(input, originalValue))
+    .then(() => setInputValue(input, originalValue))
     .then(() => expect(submit.disabled).toBe(true));
 }
 
@@ -33,4 +33,24 @@ describe('DirtySubmitForm', () => {
       .then(done)
       .catch(done.fail);
   });
+
+  it('disables submit until there are changes for radio inputs', done => {
+    const { form, input, submit } = createForm('radio');
+
+    new DirtySubmitForm(form); // eslint-disable-line no-new
+
+    return expectToToggleDisableOnDirtyUpdate(submit, input)
+      .then(done)
+      .catch(done.fail);
+  });
+
+  it('disables submit until there are changes for checkbox inputs', done => {
+    const { form, input, submit } = createForm('checkbox');
+
+    new DirtySubmitForm(form); // eslint-disable-line no-new
+
+    return expectToToggleDisableOnDirtyUpdate(submit, input)
+      .then(done)
+      .catch(done.fail);
+  });
 });

spec/javascripts/dirty_submit/helper.js

@@ -1,25 +1,42 @@
 import DirtySubmitForm from '~/dirty_submit/dirty_submit_form';
 import setTimeoutPromiseHelper from '../helpers/set_timeout_promise_helper';
 
-export function setInput(element, value) {
+function isCheckableType(type) {
+  return /^(radio|checkbox)$/.test(type);
+}
+
+export function setInputValue(element, value) {
+  const { type } = element;
+  let eventType;
+
+  if (isCheckableType(type)) {
+    element.checked = !element.checked;
+    eventType = 'change';
+  } else {
     element.value = value;
+    eventType = 'input';
+  }
 
   element.dispatchEvent(
-    new Event('input', {
+    new Event(eventType, {
       bubbles: true,
-      cancelable: true,
     }),
   );
 
   return setTimeoutPromiseHelper(DirtySubmitForm.THROTTLE_DURATION);
 }
 
-export function createForm() {
+export function getInputValue(input) {
+  return isCheckableType(input.type) ? input.checked : input.value;
+}
+
+export function createForm(type = 'text') {
   const form = document.createElement('form');
   form.innerHTML = `
-    <input type="text" value="original" class="js-input" name="input" />
+    <input type="${type}" name="${type}" class="js-input"/>
     <button type="submit" class="js-dirty-submit"></button>
   `;
+
   const input = form.querySelector('.js-input');
   const submit = form.querySelector('.js-dirty-submit');
 

spec/support/shared_examples/dirty_submit_form_shared_examples.rb

@@ -1,24 +1,36 @@
 shared_examples 'dirty submit form' do |selector_args|
   selectors = selector_args.is_a?(Array) ? selector_args : [selector_args]
 
+  def expect_disabled_state(form, submit, is_disabled = true)
+    disabled_selector = is_disabled == true ? '[disabled]' : ':not([disabled])'
+
+    form.find(".js-dirty-submit#{disabled_selector}", match: :first)
+
+    expect(submit.disabled?).to be is_disabled
+  end
+
   selectors.each do |selector|
-    it "disables #{selector[:form]} submit until there are changes", :js do
+    it "disables #{selector[:form]} submit until there are changes on #{selector[:input]}", :js do
       form = find(selector[:form])
       submit = form.first('.js-dirty-submit')
       input = form.first(selector[:input])
+      is_radio = input[:type] == 'radio'
+      is_checkbox = input[:type] == 'checkbox'
+      is_checkable = is_radio || is_checkbox
       original_value = input.value
+      original_checkable = form.find("input[name='#{input[:name]}'][checked]") if is_radio
+      original_checkable = input if is_checkbox
 
       expect(submit.disabled?).to be true
+      expect(input.checked?).to be false
 
-      input.set("#{original_value} changes")
+      is_checkable ? input.click : input.set("#{original_value} changes")
 
-      form.find('.js-dirty-submit:not([disabled])', match: :first)
-      expect(submit.disabled?).to be false
+      expect_disabled_state(form, submit, false)
 
-      input.set(original_value)
+      is_checkable ? original_checkable.click : input.set(original_value)
 
-      form.find('.js-dirty-submit[disabled]', match: :first)
-      expect(submit.disabled?).to be true
+      expect_disabled_state(form, submit)
     end
   end
 end