User tests: Successful: Unsuccessful:
Pull Request for Issue #24333
removed this.removeEventListener('click');
in build/media_source/system/js/joomla-toolbar-button.w-c.es6.js
as suggested in the issue
Code Inspection
Status | New | ⇒ | Pending |
Category | ⇒ | JavaScript Repository NPM Change |
fix removeEventListener
, to work in correct way https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener
instead of removing it
but, I think the fix are fine,
a binding happen to current element in a constructor, no need to "unbind" it at disconnectedCallback
yes, leave it as you did
I disagree! You totally need to remove have the listener on the disconectCallback lifecycle event. So the right patch is to make a function onClick
or whatever other name, call that on the addEventListener and remove it properly on removeEventListener...
Guys, whatever decision you like what's important is to fix the error. Add an onClick function and remove it properly or totally remove the removeEventListener usage. IMHO the onClick function should be added ONLY if really needed not just to have removeEventListener properly remove something,
You totally need to remove the listener on the disconectCallback lifecycle event.
Check the code, it make no sense, in current case.
Current state has few problems. Constructor is accessing the DOM. This won’t fly on all possible cases (eg programmatically create an element, insert it in the doc, then try to set some attributes or inner html)... I thought these were fixed in the last Pr but I guess not...
Constructor is accessing the DOM
Where did you seen it? it is not, there only event binding to this
, which is current Element instance.
And it will work fine.
Here is the proper fix here:
/**
* @copyright Copyright (C) 2005 - 2019 Open Source Matters, Inc. All rights reserved.
* @license GNU General Public License version 2 or later; see LICENSE.txt
*/
window.customElements.define('joomla-toolbar-button', class extends HTMLElement {
// Attribute getters
get task() { return this.getAttribute('task'); }
get listSelection() { return this.hasAttribute('list-selection'); }
get form() { return this.getAttribute('form'); }
get formValidation() { return this.hasAttribute('form-validation'); }
get confirmMessage() { return this.getAttribute('confirm-message'); }
/**
* Lifecycle
*/
constructor() {
super();
if (!Joomla) {
throw new Error('Joomla API is not properly initiated');
}
this.onChange = this.onChange.bind(this);
this.executeTask = this.executeTask.bind(this);
}
/**
* Lifecycle
*/
connectedCallback() {
// We need a button to support button behavior,
// because we cannot currently extend HTMLButtonElement
this.buttonElement = this.querySelector('button, a');
this.addEventListener('click', this.executeTask);
// Check whether we have a form
const formSelector = this.form || 'adminForm';
this.formElement = document.getElementById(formSelector);
this.disabled = false;
// If list selection is required, set button to disabled by default
if (this.listSelection) {
this.setDisabled(true);
}
if (this.listSelection) {
if (!this.formElement) {
throw new Error(`The form "${formSelector}" is required to perform the task, but the form was not found on the page.`);
}
// Watch on list selection
this.formElement.boxchecked.addEventListener('change', this.onChange);
}
}
/**
* Lifecycle
*/
disconnectedCallback() {
if (this.formElement.boxchecked) {
this.formElement.boxchecked.removeEventListener('change', this.onChange);
}
this.removeEventListener('click', this.executeTask);
}
onChange(event) {
// Check whether we have selected something
this.setDisabled(event.target.value < 1);
}
setDisabled(disabled) {
// Make sure we have a boolean value
this.disabled = !!disabled;
// Switch attribute for current element
if (this.disabled) {
this.setAttribute('disabled', true);
} else {
this.removeAttribute('disabled');
}
// Switch attribute for native element
// An anchor does not support "disabled" attribute, so use class
if (this.buttonElement) {
if (this.disabled) {
if (this.buttonElement.nodeName === 'BUTTON') {
this.buttonElement.setAttribute('disabled', true);
} else {
this.buttonElement.classList.add('disabled');
}
} else if (this.buttonElement.nodeName === 'BUTTON') {
this.buttonElement.removeAttribute('disabled');
} else {
this.buttonElement.classList.remove('disabled');
}
}
}
executeTask() {
if (this.disabled) {
return false;
}
// eslint-disable-next-line no-restricted-globals
if (this.confirmMessage && !confirm(this.confirmMessage)) {
return false;
}
if (this.task) {
Joomla.submitbutton(this.task, this.form, this.formValidation);
}
return true;
}
});
@hardik-codes please update this PR
Labels |
Added:
NPM Resource Changed
?
|
done @dgrammatiko
Category | JavaScript Repository NPM Change | ⇒ | JavaScript Repository |
This works!
@joeforjoomla please mark your test as successfully > https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results
I have tested this item
I have tested this item
@joeforjoomla the Pull Request needs a second successfully test from another User before it can be merged.
Sorry @franz-wohlkoenig can you do this please?
Sorry @franz-wohlkoenig can you do this please?
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-07-29 12:46:41 |
Closed_By | ⇒ | wilsonge |
Thanks!
Category | JavaScript Repository | ⇒ | JavaScript Repository NPM Change |
The right fix is to detach the correct event listener, not remove the call to detach the event listener.