Can't call glitch() multiple times with query selectors

ValerioB88 commented 6 months ago

Hi. I believe there is a very nasty bag, difficult to spot, which results are visible only in particular contexts. However, solving this bug allows a wider usecase for powerglitch.

Let's say that we tag one <div> element with class "myglitch", and call PowerGlitch.glitch("myglitch", glitch_initial_opts);. This will look for any element with class="myglitch", and do the following thing: if this is the first time they glitch, it will clone <div> within a grid container. This grid container now contains the original div + any other <div> needed for the glitch. These are div are clone of the original <div>, including the class "myglitch".

Cool. Now let's say we wanna change the options for our <div> and apply the glitch again. We would call stoGlitch(), and then PowerGlitch.glitch(".myglitch", glitch_new_opts). This will cause an error for the following reason:

the glitch function will collect all elements with class "myglitch" in line 554. This include sthe original element and the cloned elements!
For each one of this element, it calls glitchElement (ln 567 in powerglitch.ts).
The first element in this for loop is the original element. glitchElement calls prepareGlitchElement. In line 413, prepareGlitchElement will remove all sibling of this element. Notice: this include elements collected in point 1! Note that these elements are NOT deleted, but they are just disconnected from the parent.
Now the second element of the for loop in glitchElement: this is one of the disconnected children. In line 409 we call layerContainer = element.parentElement: but we did removeChild(), so that the parent of this element is null. Line 413 therefore will throw an error: layersContainer.removeChild(layersContainer.children[1]).

SOLUTION I know that in another discussion @7PH commented that we are not supposed to call glitch multiple times (here, notice that the user is experiencing exactly the error discussed here). So this could not even be considered a bug. However, I think that there are many good usecase that requires calling glitch() more than once. The main one (and the reason I digged deeper into this) is to changing the options of the glitched element repeatedly after some time (e.g in a setInterval loop). I don't think there is a simple way in JS to do that if we cannot call glitch multiple times with new options. Probably there is a way in React, but I don't know React (and I barely know JS). The solution is: when we cycle through all elements with class "myglitch" in line 554 (in the functoin glitch()), we need to avoid the elements that the glitch library itself created (that is, the clones that we use for the glitch itself). In this way, when we can safely remove them (in line 413). This will ensure that we only queue for glitching the element that we really wanna glitch, and not additional cloned element. We can tag the created clone elegemnt with an additional class (I used "pwgel"). Then when we collect all elements with class "glitch", we exclude those with glass "pwgel". That is: line 554:

elements = Array.from(document.querySelectorAll(elOrSelector);

becomes

elements = Array.from(document.querySelectorAll(`${elOrSelector}:not(.pwgel)`));

and line 457-460:

for (let i = 0; i < layers.length - 1; ++ i) {
        const layerDiv = baseLayer.cloneNode(true);
        layersContainer.appendChild(layerDiv);
    }

become:

  for (let i = 0; i < layers.length - 1; ++i) {
    const layerDiv = baseLayer.cloneNode(true);
    if (layerDiv.nodeType === Node.ELEMENT_NODE) {
      layerDiv.classList.add("pwgel");
    }
    layersContainer.appendChild(layerDiv);
  }

This is tested and it works. Now you can call glitch multiple times with different parameters I am not sure if there is a similar bug when the elOrSelector is a list of HTMLElement, a HTMLElement or a NodeList. This is just used for when elOrSelector is a string.

I am not doing a pull request yet as I have just started with web developing and JS, so I might have missed something. I first wanted to show this and have your opinion. :)

7PH commented 6 months ago

Thanks for opening an issue. Indeed, after taking a look, calling multiple times glitch(..) using the same query selector will also try and glitch the glitch layers, which should not be glitched. Only the root layer (= the glitched element) should be "prepared" and glitched again.

I indeed agree being able to glitch multiple times an element, passing different options is a valid use-case that is worth implementing.

Regarding the solution, I would avoid messing with the given selector or using a custom class name, relying on data attributes seems to be a safer option to me.

Taking a quick look, I think it could be implemented like this (very similar to your suggestion):

In the code that generate layers:

// Base layer
const baseLayer = glitched.cloneNode(true) as HTMLElement;
baseLayer.dataset.islayer= 'true';

In glitch(..), we filter out these elements:

const entries = elements
    .filter(element => ! element.dataset.islayer)
    .map((element) =>
        glitchElement(element, layers, options)
    );

This is to be validated and tested, I will take a look in the upcoming days when I get some free time to get back to this project - You are also welcome to create a pull request if you have some time to contribute and want to dig a bit into validating this and adding some tests 😇

ValerioB88 commented 6 months ago

Hi @7PH . Sorry I wrote this yesterday quite late for me and I just saw the post was quite badly formatted - I fixed some of that. Yes, your solution is better! As I said, I am pretty new to JS and web development :) Thank for your amazing tool

7PH commented 5 months ago

Hello @ValerioB88 , thank you for sponsoring me, it means a lot to me. I love the idea that this lib is liked and used around, and getting sponsored for maintaining it makes me real happy, so thanks 😀

I've requalified your issue as bug, and I've opened a pull request that fixes this. I will merge and release a patch version soon. Thanks again for your support.

ValerioB88 commented 5 months ago

Hi Benjamin, it's totally deserved! by the way I am curious about something. WIth that bug, it's not possible to call "glitch" multiple time. That is, is not possible to change the options for a glitch. But then how did you do your playground? That seems to be exactly what the bug should prevent you doing: changing the options and re call glitch.

On Thu, 30 May 2024 at 23:58, Benjamin Raymond @.***> wrote:

Hello @ValerioB88 https://github.com/ValerioB88 , thank you for sponsoring me, it means a lot to me. I love the idea that this lib is liked and used around, and getting sponsored for maintaining it makes me real happy, so thanks 😀

I've requalified your issue as bug, and I've opened a pull request that fixes this. I will merge and release a patch version soon. Thanks again for your support.

— Reply to this email directly, view it on GitHub https://github.com/7PH/powerglitch/issues/26#issuecomment-2140924134, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEDH7BVHFE5TNCNIUDMP36LZE6OHDAVCNFSM6AAAAABIPYEOYKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBQHEZDIMJTGQ . You are receiving this because you were mentioned.Message ID: @.***>

7PH commented 5 months ago

The playground is glitching by reference, it's not using a query selector.

Actually, there was already a unit test ensuring it's possible to glitch an element multiple times, but it was only testing the glitch-by-reference API, it wasn't testing using selectors. I've updated this test to also glitch using query selector.

7PH / powerglitch

Can't call glitch() multiple times with query selectors #26