Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

!node-insert - Script not running on page load for Safari and sometimes Chrome #18

Open
petermonte opened this issue Feb 2, 2021 · 16 comments

Comments

@petermonte
Copy link

Tests:
If you make script run at execution time you will see that Safari ignores all markup on DOM at hard refresh.
If you make script run only on Window Load event Safari ignores all markup on DOM at execution time.
If you make script run only on Document readystatechange event Safari ignores all markup on DOM on hard refresh.
If you make script run only on Window DOMContentLoaded event all 3 browsers ignore markup on DOM at execution time.

Code:
Editor mode - https://codepen.io/petermonte/pen/Vwwqgjv?editors=0010
Debug mode - https://cdpn.io/petermonte/debug/Vwwqgjv/PNrvYXGWZBPM

Specs:

  • Safari - Version 14.0.2 (16610.3.7.1.9)
  • Google Chrome - Version 88.0.4324.96 (Official Build) (x86_64)
  • Firefox - Version 85.0 (64-bit)

Screen recordings:
https://user-images.githubusercontent.com/4997381/106580084-652e3380-6539-11eb-8137-9ffea143c4ec.mov
https://user-images.githubusercontent.com/4997381/106580430-d0780580-6539-11eb-8b80-d9ce14444d18.mov

This is a request by @amorey in #8 (comment)

@petermonte petermonte changed the title !node-insert - Script not running on page load for Safari and sometime Chrome !node-insert - Script not running on page load for Safari and sometimes Chrome Feb 2, 2021
@amorey
Copy link
Member

amorey commented Feb 2, 2021

Have you tried bootstrapping your code using the sentinel-load event described here?

@petermonte
Copy link
Author

https://cdpn.io/petermonte/debug/Vwwqgjv/LDAmdnPpbExr

I've updated the script to make Sentinel only run once document triggers the sentinel-load event - it seems to break in all browser.

...

const _components = [];

function registerComponent(_name, _selector, _callback) {
    _components.push({
        name: _name,
        selector: _selector,
        callback: _callback
    });

    return true;
}

document.addEventListener('sentinel-load', function () {
    sentinel.on('!node-inserted', function (el) {
        var i = 0;
        const lth = _components.length;
        for (i; i < lth; i++) {
            const component = _components[i];
            if (el.matches(component.selector)) {
                component.callback(el);
            }
        }
    });
});

...

@amorey
Copy link
Member

amorey commented Feb 2, 2021

The sentinel-load event gets triggered when the sentinel.js script runs so the event listener in the CodePen example won't get executed.

There's a lot of extra code in the example. Can you isolate the bug with a simpler script? For example, does this work when you run it locally on Safari?

<!doctype html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <style>
      @keyframes changeColor {
        0% {
          color: blue;
          transform: translateX(-10px);
        }

        100% {
          color: black;
          transform: translateX(0);
        }
      }

      .example-cls {
        animation-duration: 1s;
        animation-name: changeColor, node-inserted;
        transform: translateX(0);
      }
    </style>
    <script src="https://cdn.rawgit.com/muicss/sentineljs/0.0.5/dist/sentinel.min.js"></script>
    <script>
      sentinel.on('!node-inserted', function (el) {
        el.innerHTML = "Sentinel is always watching";
      });
    </script>
  </head>
  <body>
    <!-- element to be parsed by sentinel -->
    <div class="example-cls"><div>
  </body>
</html>

What is it that you want to do differently?

@petermonte
Copy link
Author

Here is you're code in a simple way isolate from any other script.
https://cdpn.io/petermonte/debug/XWWKRNv/mWAoNzxpxxWr

  • I still see Safari not working with an hard refresh.
  • Tried sentinel-load didn't seem to work, unless I'm missing something.

What is it that you want to do differently?

Imagine a UI Kit that offers (per example):

  • a tooltip functionality by using title="My tooltip information"
  • a tab item functionality by adding a classname tab
  • a dropdown trigger by using the attribute data-dropdown-trigger="target"

Let say that all 3 features exist isolated or combined like so:

Screenshot 2021-02-02 at 17 14 35

How would you implement such UI Kit given that you need to trigger all existing elements on DOM at page load and any new added to the DOM via script?

@amorey
Copy link
Member

amorey commented Feb 2, 2021

When I click on the codepen link I see a "This debug view expired" message.

From the description of what you're trying to do it sounds like you might be running into a race condition between the page load event and the animationstart event of the elements that are already on the page. You should be able to avoid the race condition by executing sentinel.on() before the dom elements are defined (e.g. in the <head>):

<html>
  <head>
    <script src="/path/to/sentinel.js"></script>
    <script>
      sentinel.on('custom-element', function(el) {
        el.innerHTML = "Sentinel is always watching";
      });
    </script>
  </head>
  <body>
    <custom-element></custom-element>
  </body>
</html>

Alternatively, if you want to bootstrap your code after the DOMContentLoaded event then you should be able to so by processing all of the previously defined DOM elements first and then initializing sentinel afterwards. In that case the javascript can be executed before the closing </body> tag:

<html>
  <head>
    <script src="/path/to/sentinel.js"></script>
  </head>
  <body>
    <custom-element></custom-element>
    <script>
      function modifyElement(el) {
        el.innerHTML = "This element has been modified";
      }

      document.addEventListener('DOMContentLoaded', function(ev) {
        // process previously defined DOM elements
        document.querySelectorAll('custom-element').forEach(modifyElement);
        
        // initialize sentinel
        sentinel.on('custom-element', modifyElement);
      });
    </script>
  </body>
</html>

@petermonte
Copy link
Author

let me put things in a simple way by using your example. Need to slightly change it because you only target one element base on the css animation name. I'm targeting multiple elements, based on multiple kinds of selectors.

Environment:

MacOS Big Sur 11.1
Safari - Version 14.0.2 (16610.3.7.1.9)
Google Chrome - Version 88.0.4324.96 (Official Build) (x86_64)
Firefox - Version 85.0 (64-bit)

This is the markup:

<div class="example-cls" data-attr="abc" title="Title For Tooltip">ON DOM</div>
<!--
I expect a call on Sentinel based on a div tagname
I expect a call on Sentinel based on an attribute named title
I expect a call on Sentinel based on an attribute called data-attr
I expect a call on Sentinel  based on a classname example-css
-->

This is the CSS:

@keyframes change-color {
  0% {
    color: blue;
    transform: translateX(-10px);
  }

  100% {
    color: black;
    transform: translateX(0);
  }
}

div, /* I expect a call on Sentinel based on a tagname */
[title], /* I expect a call on Sentinel based on an attribute named title */
[data-attr], /* I expect a call on Sentinel based on an attribute called data-attr */
.example-cls /* I expect a call on Sentinel based on a classname example-css */
{
  animation-duration: 1s;
  animation-name: change-color, node-inserted;
  transform: translateX(0);
}

This is the JS:

function modifyElement(el) {
    // I increment the innerHTML so that I can count how many times an element is caught by Sentinel
    el.innerHTML += " Sentinel ";
}

window.addEventListener('DOMContentLoaded', function (ev) {
    document.querySelectorAll('[data-attr], .example-cls, [title]').forEach(modifyElement);
    sentinel.on('!node-inserted', modifyElement);
});


// Added a click to generate markup on the fly
document.addEventListener('click', function () {
    const div = document.createElement('div'); // I expect a call on Sentinel based on a tagname Div
    div.textContent = 'ADDED ';
    div.className = 'example-cls'; // I expect a call on Sentinel based on classname example-cls
    div.dataset.attr = 'abc';  // I expect a call on Sentinel based on attibute data-attr
    div.title = 'hello';  // I expect a call on Sentinel based on attibute title
    document.body.append(div);
});

This is what I get (Safari, Firefox, Chrome):

Screenshot 2021-02-02 at 21 10 02

This is what I expected:

sentinel


This is the code editor: https://codepen.io/petermonte/pen/XWWKRNv
This is the code in editor mode, so stripped out of any other script: https://cdpn.io/petermonte/debug/XWWKRNv/nqkwvzGpyEOA

@amorey
Copy link
Member

amorey commented Feb 3, 2021

First, with regards to "ON DOM Sentinel" I think the difference in behavior between the browsers is related to a race condition between DOMContentLoaded and animationstart in Safari. Firefox and Chrome seem to reliably call animationstart after DOMContentLoaded which results in modifyElement() being executed twice. However, sometimes Safari seems to call animationstart before DOMContentLoaded which results in modifyElement() only getting called once. If you want to prevent multiple calls you can use a flag:

function modifyElement(el) {
  (if (!el._upgraded) el.innerHTML != " Sentinel ";
  el._upgraded = true;
}

window.addEventListener("DOMContentLoaded", function(ev) {
  document.querySelectorAll('[data-attr], .example-cls, [title]').forEach(\
modifyElement);
   sentinel.on('!node-inserted', modifyElement);
});

This will result in consistent "ON DOM Sentinel" behavior across browsers.

Second, with regards to "ADDED Sentinel Sentinel Sentinel" it sounds like what you're expecting is for the browser to trigger animationstart 3 times, once each for the "[data-attr]", ".example-css", "[title]" css selectors. However, browsers only trigger the event once per element even if multiple CSS rules match. If you want to execute different code based on the CSS selector that matched you can do some post processing to see which css selectors match.

@petermonte
Copy link
Author

petermonte commented Feb 3, 2021

@amorey

Thanks man. I was almost going crazy trying to figure out what was happening. My purpose with this thread was to understand how sensible Sentinel is with how "each browser deals differently with events for script execution and page load".

  • Would be nice if Sentinel packed a listener to make sure that it ran on script execution.

On another topic but related to this thread I realised that Sentinel when used with the !node-inserted logic doesn't quite help to distinguish what to do on an element on callback. Like in my case a one single element can have randomly up to 3 functionalities and each independent form another.

  • Would be nice that we could simply register a listener with Sentinel and have it check if the animation name !node-inserted needs to be added or pushed to the existing styles of the element/selector.
sentinel.on('.my-target', function(){}, true);
// Last boolean parameter stands for using the generic animation name [node-inserted] to catch any element on DOM
// So if a specific selector has already an animation name declared an additional one will be added: my-animation, node-inserted

This means that we could use the following logic:

// Both these events can occur or not on a same element

sentinel.on('.my-target', function(){
    // My code specifically for any new element added with the classname .my-target
}, true);

sentinel.on('[title]', function(){
    // My code specifically for any new element added that has a title attribute to use has a tooltip
}, true);

In the meantime this is my workaround (Click anywhere to add new elements to DOM):

Edit: Forgot to include the css.

@keyframes my-animation {
  0% {
    color: blue;
  }

  100% {
    color: black;
  }
}

[title],
[data-attr],
.tabs {
  animation-duration: 1s;
  animation-name: my-animation, node-inserted;
}
// Interface to record every single constructor that can be called on an element based on its selector
const components = [];

function registerComponent(_name, _selector, _callback) {
    // Register our component
    components.push({
        name: _name,
        selector: _selector,
        callback: _callback
    });
    return true;
}

function initElement(el) {
    // Run the element through our components and find matches
    components.forEach(component => el.matches(component.selector) ? component.callback(el) : null);
    return true;
}





// Interface to manage elements on DOM and when added to DOM
let sentinelRanAtDomContentLoaded = false;

sentinel.on('!node-inserted', el => {
    if (!sentinelRanAtDomContentLoaded){
        sentinelRanAtDomContentLoaded = true;
    }
    initElement(el);
});

document.addEventListener('DOMContentLoaded', e => {
    if (!sentinelRanAtDomContentLoaded){
        components.forEach(function(component){
            document.querySelectorAll(component.selector).forEach(function(el){
                component.callback(el);
            });
        });
        sentinelRanAtDomContentLoaded = true;
    }
}, true);




/**
 * Usage. Every single selector has its own context
 */
registerComponent(
    'tabs',
    '.tabs',
    function tabs(el) {
        // RUN EVERYTHING NEEDED FOR THE COMPONENT
        return el;
    }
);

registerComponent(
   'title-attr',
   '[title]',
   function attrTitle(el) {
       // RUN EVERYTHING NEEDED FOR THE COMPONENT
       return el;
   }
);

registerComponent(
   'data-attr',
   '[data-attr]',
   function dataAttr(el) {
       // RUN EVERYTHING NEEDED FOR THE COMPONENT
       return el;
   }
);

@amorey
Copy link
Member

amorey commented Feb 4, 2021

No problem. Happy to hear that helped.

Would be nice if Sentinel packed a listener to make sure that it ran on script execution.

Can you describe this in more detail? What do you mean by script execution time? The time when the browser parses and executes the "sentinel.js" script?

Would be nice that we could simply register a listener with Sentinel and have it check if the animation name !node-inserted needs to be added or pushed to the existing styles of the element/selector.

It sounds like what you want is SentinelJS to execute all three callbacks in this example:

<script>
  sentinel.on('div', function callback1() {});
  sentinel.on('.example-cls', function callback2() {});
  sentinel.on('[example-attr]', function callback3() {});
</script>
<div class="example-cls" example-attr="value"></div>

Is that correct?

@petermonte
Copy link
Author

Can you describe this in more detail? What do you mean by script execution time? The time when the browser parses and executes the "sentinel.js" script?

The moment the browser as fully loaded HTML (DOM tree is built and available) yet any external resources like <style> might not be available.

It sounds like what you want is SentinelJS to execute all three callbacks in this example:

Have you tested your script?

@amorey
Copy link
Member

amorey commented Feb 4, 2021

The moment the browser as fully loaded HTML (DOM tree is built and available) yet any external resources like <style> might not be available.

Why can't you use the DOMContentLoaded event?

Have you tested your script?

The example snippet wasn't intended to work, I posed it as a theoretical example to see if that is what you are asking for.

@petermonte
Copy link
Author

The example snippet wasn't intended to work, I posed it as a theoretical example to see if that is what you are asking for.

Ah! sorry, my bad. Yes, that is the idea. To have as many calls I want on a same element.

@petermonte
Copy link
Author

petermonte commented Feb 4, 2021

Why can't you use the DOMContentLoaded event?

Yes, in fact my text practically describes the event DOMContentLoaded. Problem is that it doesn't work.


So to sum things up and for the sake of simplicity two issues are raised here on my comments:

1 - Bug: Safari - Version 14.0.2 (16610.3.7.1.9) on MacOS Big Sur 11.1 Sentinel seems to sometimes not parse elements that are at DOM on page load. Using DOMContentLoaded makes things worst.

2 - Feature: Run separate callbacks with different selectors on a same element.

@amorey
Copy link
Member

amorey commented Feb 4, 2021

Bug: Safari - Version 14.0.2 (16610.3.7.1.9) on MacOS Big Sur 11.1 Sentinel seems to sometimes not parse elements that are at DOM on page load. Using DOMContentLoaded makes things worst.

Sentinel isn't designed to detect elements at DOMContentLoaded, it's designed to detect elements when animationstart is triggered so I don't think this is a bug.

Feature: Run separate callbacks with different selectors on a same element.

This would require calling element.matches() on all elements and might cause a performance hit so I have to think about it. In the mean time you can use the "!node-inserted" custom animation name method and call element.matches() manually if you want to test it out.

@petermonte
Copy link
Author

Sentinel isn't designed to detect elements at DOMContentLoaded, it's designed to detect elements when animationstart is triggered so I don't think this is a bug.

You're right when it comes to the event DOMContentLoaded. Yet with the !node-inserted method, Sentinel (Safari!!) misses elements that already exist in DOM if the page is loading for the first time (hard refresh exemplifies this too). On the other hand if we try to use an event load on window to make sure Sentinel script is loaded first it will also not work with the !node-inserted method and all the existing elements on DOM will remain in silence. So this leaves us with the work around of doing a document.querySelectorAll which ends up being what I ran away from in favour of Sentinel :(

https://cdpn.io/petermonte/debug/LYPKKQQ/yokZEpWRVwyA

<div class="node_inserted">!node-inserted</div>
<div class="class_name">class name</div>
.node_inserted {
  animation-name: node-inserted;
  animation-duration: 0.001s;
}
sentinel.on('!node-inserted', function(el) {
  el.innerHTML += ' <b>Sentinel node-inserted watch!</b>';
});

sentinel.on('.class_name', function(el) {
  el.innerHTML += ' <b>Sentinel class_name watch!</b>';
});
sentinel.mov

This would require calling element.matches() on all elements and might cause a performance hit so I have to think about it. In the mean time you can use the "!node-inserted" custom animation name method and call element.matches() manually if you want to test it out.

Thought so. Possible solution might be a simple wrapper for those who need such functionality. If no element is being targeted for more than a simple selector/callback then definitely there is no need to change anything on Sentinel.

Conclusion:
I don't think there is enough here to justify any change on Sentinel, so if you agree you can close this topic.

Can't thank you enough for your support!

@amorey
Copy link
Member

amorey commented Feb 8, 2021

I tried clicking on the codepen link but it says the "Debug view is expired".

In your previous code example, I think you were adding a Sentinel listener after you defined the DOM elements. However, if you want Sentinel to detect the animationstart event when an element gets added to the DOM then the listener has to be defined first. Here's a canonical example using the "!node-inserted" method:

<!doctype html>
<html>
  <head>
    <script src="//cdn.rawgit.com/muicss/sentineljs/0.0.5/dist/sentinel.min.js"></script>
    <style>
      @keyframes changeColor {
        0% {color: red;}
        100% {color: black;}
      }

      .example-cls {
        animation-duration: 1s;
        animation-name: changeColor, node-inserted;    
      }
    </style>
    <script>
      sentinel.on('!node-inserted', function(el) {
        el.innerHTML = "Sentinel is always watching";
      });
    </script>
  </head>
  <body>
    <div class="example-cls"></div>  
  </body>
</html>

I tested this code on Safari 14.0.3/Big Sur 11.2 repeatedly and didn't see Sentinel miss any animationstart events (see screen recording). Let me know if you see different behavior.

recording.mov

I have some other thoughts to share on the DOMContentLoaded event but first let's establish whether or not there's a bug that's causing Sentinel to miss pre-defined DOM elements in Safari.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants