vguillou / swipe-action

A Polymer element providing a declarative way to swipe DOM elements with configurable underlays and trigger some action when the swiping is done. Integrates seamlessly with the <iron-list> element.
https://vguillou.github.io/webcomponents/swipe-action
MIT License
15 stars 5 forks source link

activatedActionId bug #7

Open OvermindDL1 opened 7 years ago

OvermindDL1 commented 7 years ago

When activatedActionId is set to a falsey value like "" or 0 then as per the _onActivatedActionIdChange function (line 421 as of the current release version):

      _onActivatedActionIdChange: function(newValue, oldValue) {
        if (!this._activatedActionIdAlreadyChanged) {
          this._mutationObserverPaused = true;
          this._mute = 1;
          if (!newValue) {
            this._mute++;
            this.reset(false);
          } else {
            this.activateAction(newValue, false);
          }
        }
        this._activatedActionIdAlreadyChanged = false;
      },

It is setting this._mute = 1, then since newValue is falsey it then sets this._mute++, which is setting this._mute to be 2, then calls this.reset(false), and reset calls:

      reset: function(animate) {
        // Snap the item back into place
        this._snapBack(animate);
      },

And _snapBack calls:

      _snapBack: function(animate) {
        if (this.isSwiping || this.isSwipedAway) {
          Polymer.dom(this.target).classList.add('snap-back');
        }
        // Animation
        if (animate) {
          Polymer.dom(this.target).classList.add('animated');
        } else {
          this.target.style.transition = '';
          this._cleanupAfterTransition();
        }
      },

Which since animate is false it will call the else branch, which calls this._cleanupAfterTransition(), which is:

      _cleanupAfterTransition: function() {        
        // If item is swiped away, the swipe-action is activated: fire the appropriate event
        if (this._willBeSwipedAway) {
          this._willBeSwipedAway = false;
          this._changeActivatedAction(this._currentAction);
          // Fire activation event
          this._fireEvent('swiped-away');
          return;
        }
        // If the target node won't be swiped away, then it will be snaped back,
        // and the action won't be activated anymore
        else if (Polymer.dom(this.target).classList.contains('snap-back')) {
          this._changeActivatedAction(undefined);
          // Fire cancelation event
          this._fireEvent('swipe-canceled');
        }
        // Cleanup to do AFTER the event is fired
        this._mute--;
      },

Which does a few things, but ending up with this._mute--, which will set this._mute back to 1, so now this._mute is left at 1. Now later when the element is swiped away, which ends up calling this same function but with this._willBeSwipedAway set to true, so this._fireEvent('swiped-away') gets called, and it is:

      _fireEvent: function(eventName) {
        if (!this._mute) {
          this.fire(eventName, {action: this._currentAction, gesture: this._gesture});
        }
      },

Where this._mute is still 1, thus this event is skipped and never fired, ever again. The issue appears resolved by going back to the first _onActivatedActionIdChange function definition of:

      _onActivatedActionIdChange: function(newValue, oldValue) {
        if (!this._activatedActionIdAlreadyChanged) {
          this._mutationObserverPaused = true;
          this._mute = 1;
          if (!newValue) {
            this._mute++;
            this.reset(false);
          } else {
            this.activateAction(newValue, false);
          }
        }
        this._activatedActionIdAlreadyChanged = false;
      },

And just wiping out the this._mute++; line entirely, that will leave this._mute as 1 in the reset call, which ends up setting it back to 0 when this._mute-- is run in the _cleanupAfterTransition function, thus restoring the state.

However, this reveals another bug, this._mute is never initialized unless activatedActionId is set, meaning that it is by default undefined. When _cleanupAfterTransition is called on every swipe it will in turn call this._mute-- on an undefined value, thus setting this._mute to be NaN, which will allow the events to be fired when _fireEvent is called regardless. But by setting activatedActionId it makes this._mute = 0 by the end of its call (or 1 without the above fix in some cases), so when it is swiped or reset it decrements this._mute via the this._mute-- call inside _cleanupAfterTransition, which makes it <0, thus in the _fireEvent function:

      _fireEvent: function(eventName) {
        if (!this._mute) {
          this.fire(eventName, {action: this._currentAction, gesture: this._gesture});
        }
      },

Then !-1 via the !this._mute check (or lower value) will be false and the events stop firing again and never fire again until activatedActionId is set again, of which it will then only fire once more and never again until it is set yet again.

This second issue 'should' be fixed properly by always setting a default value to this._mute on construction and fixing up the code elsewhere, however a quick-fix that will always work (in my testing) is to just change this._mute--; in function _cleanupAfterTransition into being this._mute = this._mute>0 ? this._mute-1 : 0; since it being the only place doing the decrementing means that it needs to make sure it does not go below 0 and it will always default set it to 0 otherwise.

vguillou commented 7 years ago

Wow this is a lot of information. :) Could you explain me how to reproduce this bug please ?

OvermindDL1 commented 7 years ago

Basically I was just setting activated-action-id to a variable that usually contained zero in the <dom-repeat> loop as I was displaying a list of things. At that point on any that were 0 stopped swiping, and any attempts to swipe did nothing. I traced it into the _fireEvent where _mute was NaN and followed it back from there. :-)