NPM Resource Changed ? Failure

User tests: Successful: Unsuccessful:

avatar hardik-codes
hardik-codes
24 Mar 2019

Pull Request for Issue #24333

Summary of Changes

removed this.removeEventListener('click'); in build/media_source/system/js/joomla-toolbar-button.w-c.es6.js as suggested in the issue

Testing Instructions

Code Inspection

e3b6052 24 Mar 2019 avatar hardik-codes Fix
avatar hardik-codes hardik-codes - open - 24 Mar 2019
avatar hardik-codes hardik-codes - change - 24 Mar 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Mar 2019
Category JavaScript Repository NPM Change
avatar mbabker
mbabker - comment - 24 Mar 2019

The right fix is to detach the correct event listener, not remove the call to detach the event listener.

avatar hardik-codes
hardik-codes - comment - 24 Mar 2019

The right fix is to detach the correct event listener, not remove the call to detach the event listener.

@mbabker Could you please elaborate ?

avatar Fedik
Fedik - comment - 24 Mar 2019

fix removeEventListener, to work in correct way https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener

instead of removing it

avatar Fedik
Fedik - comment - 24 Mar 2019

but, I think the fix are fine,
a binding happen to current element in a constructor, no need to "unbind" it at disconnectedCallback

avatar hardik-codes
hardik-codes - comment - 24 Mar 2019

but, I think the fix are fine,
a binding happen to current element in a constructor, no need to "unbind" it at disconnectedCallback

@Fedik should I leave it as it is ?

avatar Fedik
Fedik - comment - 24 Mar 2019

yes, leave it as you did

avatar dgrammatiko
dgrammatiko - comment - 24 Mar 2019

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

avatar joeforjoomla
joeforjoomla - comment - 24 Mar 2019

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,

avatar Fedik
Fedik - comment - 24 Mar 2019

You totally need to remove the listener on the disconectCallback lifecycle event.

Check the code, it make no sense, in current case.

avatar dgrammatiko
dgrammatiko - comment - 24 Mar 2019

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

avatar Fedik
Fedik - comment - 25 Mar 2019

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.

avatar dgrammatiko
dgrammatiko - comment - 25 Mar 2019

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

avatar hardik-codes hardik-codes - change - 25 Mar 2019
Labels Added: NPM Resource Changed ?
avatar hardik-codes
hardik-codes - comment - 25 Mar 2019
avatar franz-wohlkoenig franz-wohlkoenig - change - 11 Apr 2019
Category JavaScript Repository NPM Change JavaScript Repository
avatar joeforjoomla
joeforjoomla - comment - 6 Jun 2019

This works!

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 7 Jun 2019
avatar joeforjoomla
joeforjoomla - comment - 7 Jun 2019

I have tested this item successfully on 114cc76


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

avatar joeforjoomla
joeforjoomla - comment - 7 Jun 2019

I have tested this item successfully on 114cc76


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

avatar joeforjoomla joeforjoomla - test_item - 7 Jun 2019 - Tested successfully
avatar joeforjoomla
joeforjoomla - comment - 16 Jul 2019

@wilsonge can this be merged please, as this error keeps popping up in backend... thanks!

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 17 Jul 2019

@joeforjoomla the Pull Request needs a second successfully test from another User before it can be merged.

avatar joeforjoomla
joeforjoomla - comment - 17 Jul 2019

Sorry @franz-wohlkoenig can you do this please?

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 17 Jul 2019

Sorry @franz-wohlkoenig can you do this please?

Screen Shot 2019-07-17 at 09 54 02

avatar wilsonge wilsonge - change - 29 Jul 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-07-29 12:46:41
Closed_By wilsonge
avatar wilsonge wilsonge - close - 29 Jul 2019
avatar wilsonge wilsonge - merge - 29 Jul 2019
avatar wilsonge
wilsonge - comment - 29 Jul 2019

Thanks!

avatar joomla-cms-bot joomla-cms-bot - change - 29 Jul 2019
Category JavaScript Repository JavaScript Repository NPM Change

Add a Comment

Login with GitHub to post a comment