? ? Maintainers Checked Pending

User tests: Successful: Unsuccessful:

avatar wilsonge
wilsonge
26 Jun 2022

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

avatar wilsonge wilsonge - open - 26 Jun 2022
avatar wilsonge wilsonge - change - 26 Jun 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Jun 2022
Category JavaScript Repository NPM Change
avatar dgrammatiko
dgrammatiko - comment - 26 Jun 2022

Looking good

avatar richard67 richard67 - test_item - 26 Jun 2022 - Tested successfully
avatar richard67
richard67 - comment - 26 Jun 2022

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.

avatar chmst chmst - test_item - 26 Jun 2022 - Tested successfully
avatar chmst
chmst - comment - 26 Jun 2022

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.

avatar chmst chmst - change - 26 Jun 2022
Status Pending Ready to Commit
avatar chmst
chmst - comment - 26 Jun 2022

RTC


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

avatar dgrammatiko
dgrammatiko - comment - 26 Jun 2022

@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.

avatar richard67
richard67 - comment - 26 Jun 2022

@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.

avatar dgrammatiko
dgrammatiko - comment - 26 Jun 2022

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

The change was done with #30839

avatar wilsonge
wilsonge - comment - 27 Jun 2022

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?

avatar HLeithner HLeithner - change - 27 Jun 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-06-27 15:51:19
Closed_By HLeithner
Labels Added: ? ? Maintainers Checked
avatar HLeithner HLeithner - close - 27 Jun 2022
avatar HLeithner HLeithner - merge - 27 Jun 2022
avatar HLeithner
HLeithner - comment - 27 Jun 2022

thanks

Add a Comment

Login with GitHub to post a comment