Fix multiselect when not all rows have checkboxes

[wilsonge]

Pull Request for Issue #38021.

@dgrammatiko would appreciate a review here. Somehow i feel like after this populating this.boxes is now overkill but not really sure of a good way to make that optimization after this. Feels like this is 'good enough' though

Summary of Changes

Fixes the behaviour of multiselect when not all rows have a checkbox (because of ACL restrictions)

Testing Instructions

See reproduction steps in linked issue

Actual result BEFORE applying this Pull Request

Multiselect highlights wrong row, when not a super admin in com_users

Expected result AFTER applying this Pull Request

Multiselect highlights the right row both when super admin with full permissions and as a restricted user.

Documentation Changes Required

n/a

[dgrammatiko]

Looking good

[richard67]

I have tested this item ✅ successfully on 846f1ea

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146.}

[chmst]

I have tested this item ✅ successfully on 846f1ea

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146.}

[chmst]

RTC

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38146.}

[dgrammatiko]

@wilsonge can I ask for some changes?

here's the edited script

Script

/**
 * @copyright   (C) 2018 Open Source Matters, Inc. <https://www.joomla.org>
 * @license     GNU General Public License version 2 or later; see LICENSE.txt
 */
if (!window.Joomla) {
  throw new Error('Joomla API was not properly initialised');
}

/**
 * JavaScript behavior to allow shift select in administrator grids
 */
class JMultiSelect {
  constructor(formElement) {
    this.tableEl = document.querySelector(formElement);

    if (this.tableEl) {
      this.boxes = [].slice.call(this.tableEl.querySelectorAll('td input[type=checkbox]'));
      this.rows = [].slice.call(document.querySelectorAll('tr[class^="row"]'));
      this.checkallToggle = document.querySelector('[name="checkall-toggle"]');

      this.onCheckallToggleClick = this.onCheckallToggleClick.bind(this);
      this.onRowClick = this.onRowClick.bind(this);

      if (this.checkallToggle) {
        this.checkallToggle.addEventListener('click', this.onCheckallToggleClick);
      }

      if (this.rows.length) {
        this.rows.forEach((row) => {
          row.addEventListener('click', this.onRowClick);
        });
      }
    }
  }

  // Changes the background-color on every cell inside a <tr>
  // eslint-disable-next-line class-methods-use-this
  changeBg(row, isChecked) {
    // Check if it should add or remove the background colour
    if (isChecked) {
      [].slice.call(row.querySelectorAll('td, th')).forEach((elementToMark) => {
        elementToMark.classList.add('row-selected');
      });
    } else {
      [].slice.call(row.querySelectorAll('td, th')).forEach((elementToMark) => {
        elementToMark.classList.remove('row-selected');
      });
    }
  }

  onCheckallToggleClick({ target }) {
    const isChecked = target.checked;

    this.rows.forEach((row) => {
      this.changeBg(row, isChecked);
    });
  }

  onRowClick({ target, shiftKey }) {
    // Do not interfere with links or buttons
    if (target.tagName && (target.tagName.toLowerCase() === 'a' || target.tagName.toLowerCase() === 'button')) {
      return;
    }

    if (!this.boxes.length) {
      return;
    }

    const closestRow = target.closest('tr');
    const currentRowNum = this.rows.indexOf(closestRow);
    const currentCheckBox = closestRow.querySelector('td input[type=checkbox]');

    if (currentCheckBox) {
      let isChecked = currentCheckBox.checked;

      if (!(target.id === currentCheckBox.id)) {
        // We will prevent selecting text to prevent artifacts
        if (shiftKey) {
          document.body.style['-webkit-user-select'] = 'none';
          document.body.style['-moz-user-select'] = 'none';
          document.body.style['-ms-user-select'] = 'none';
          document.body.style['user-select'] = 'none';
        }

        currentCheckBox.checked = !currentCheckBox.checked;
        isChecked = currentCheckBox.checked;
        Joomla.isChecked(isChecked, this.tableEl.id);
      }

      this.changeBg(this.rows[currentRowNum], isChecked);

      // Restore normality
      if (shiftKey) {
        document.body.style['-webkit-user-select'] = 'none';
        document.body.style['-moz-user-select'] = 'none';
        document.body.style['-ms-user-select'] = 'none';
        document.body.style['user-select'] = 'none';
      }
    }
  }
}

let formId = '#adminForm';
if (Joomla && Joomla.getOptions('js-multiselect', {}).formName) {
  formId = `#${Joomla.getOptions('js-multiselect', {}).formName}`;
}
// eslint-disable-next-line no-new
new JMultiSelect(formId);

So what's different?

• dropped the DOMContentLoaded event as the script is loaded with a defer attribute. This means that the script executes right before that event just to queue the actual work for the event (that happens few milliseconds later). It's something that comes from the days that Joomla didn't implement the JS as type=module or nomodule or defer and it's pure overhead that delays the responsiveness of the page for no good reason...

• It would be a good perf improvement if all the scripts could remove this execution penalty. The whole procedure is really simple: 1. remove the iife 2. if there's a Joomla dependency make sure the script will throw if the global object is not defined 3. remove the wrapped execution part from the event DOMContentLoaded. FWIW all the core scripts are loaded either as type=module or defer so this is totally safe!

A more generic comment:

Since IE11 is dead, J4 didn't really supported it anyway it would be a nice to have an improvement for the tooling, eg switch the es5 to ES2015. Will cover browsers back to 2015 which obviously is way more than what the limitation imposed from Bootstrap CSS, but also means that the scripts will be leaner and the bundling step of the npm will be waaaaay faster.

[richard67]

@wilsonge can I ask for some changes?

@dgrammatiko Does that also apply go his other PR #38141 ? Does it needs the same kind of changes there, too? Ignore it, silly me. I mixed up PRs.

[dgrammatiko]

@richard67 just fyi the media manager had this improvement as the code is executed immediately

joomla-cms/administrator/components/com_media/resources/scripts/mediamanager.es6.js

Line 64 in 3e649ba

app.mount('#com-media');

The change was done with #30839

[wilsonge]

Let's get this in first with the bug fix - so the two can be separated if there are any issues.

Out of personal interest if you defer all the js files loading - is it still in order? As in how do we know if the Joomla stuff has actually being init'd?

[HLeithner]

thanks