User tests: Successful: Unsuccessful:
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
Fixes the behaviour of multiselect when not all rows have a checkbox (because of ACL restrictions)
See reproduction steps in linked issue
Multiselect highlights wrong row, when not a super admin in com_users
Multiselect highlights the right row both when super admin with full permissions and as a restricted user.
n/a
Status | New | ⇒ | Pending |
Category | ⇒ | JavaScript Repository NPM Change |
I have tested this item
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
@wilsonge can I ask for some changes?
here's the edited 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!
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.
@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.
@richard67 just fyi the media manager had this improvement as the code is executed immediately
The change was done with #30839
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?
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
|
thanks
Looking good