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

DomReady does not work when it reloads page. #2682

Open
diegoflorez opened this issue Dec 11, 2014 · 12 comments · May be fixed by #2693
Open

DomReady does not work when it reloads page. #2682

diegoflorez opened this issue Dec 11, 2014 · 12 comments · May be fixed by #2693
Labels
Milestone

Comments

@diegoflorez
Copy link

diegoflorez commented Dec 11, 2014

En firefox 34.0.5 when it reloads the page, the code inside

window.addEvent('domready',function(){
   //code inside
})

is never called again, until you close the window/tab and reopen the page.
It's imposible to make a jsfiddle to show this behaviour.
I thinks this is a bug. Because you are trying to reload the page, so the DOM is for a new instance, ready.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/6930694-domready-does-not-work-when-it-reloads-page?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github).
@GCheung55
Copy link
Contributor

In order to properly investigate this issue, the assumed bug needs to be reproducible.

Is there a gist or a page you can create that would reproduce this issue?

Were there any JS errors?

@SergioCrisostomo
Copy link
Member

@diegoflorez I made a simple page myself to check this and it works as It should, no bug.
You can check it here.

If you can put a page online with this assumed bug so we can see/test it would be the best.

Cheers!

@diegoflorez
Copy link
Author

@GCheung55 You're right!, It is what I should do, so that you could fully understand what I was trying to explain.
@SergioCrisostomo Following your example, I made a page that reproduces the error. I forgot above, mentioning that the error occurs when MooTools is charged through RequireJS.
Mootools DomReady RequireJS Test
Thank you both! for your replies.

@ricardograca
Copy link

I think this is an issue with RequireJS interacting in a strange way with Mootools and causing some kind of race condition. Check this out if you need to run scripts after the dom is ready and you're using RequireJS: https://github.com/requirejs/domReady

@SergioCrisostomo
Copy link
Member

@diegoflorez thank you creating the example showing the problem!
@ricardograca nice input! (obrigado!)

Could we fix this by adding if (!ready) check(); in this line? (last line of DOMReady.js, just before the function closure)

@SergioCrisostomo SergioCrisostomo added this to the 1.5.2 milestone Jan 14, 2015
@ibolmo ibolmo added the bug label Jan 14, 2015
@SergioCrisostomo SergioCrisostomo linked a pull request Jan 19, 2015 that will close this issue
@diegoflorez
Copy link
Author

Hey Guys, thanks for all your work.
It works Perfect! for the domready problem.
But now It throws this error: "TypeError: window is null".

@SergioCrisostomo
Copy link
Member

@diegoflorez could you share a jsFiddle reproducing that error and add some Browser info? what line does it point to?

I don't see how window can be null.

@diegoflorez
Copy link
Author

@SergioCrisostomo I have just made the change that you have suggested in fix-2682 to the mootools-core-5.1 no compat. Remove the line 6033 and Add from 6034 to 6038.

The line 6065 throws this error message: "TypeError: window is null" in Firefox 37; and this one: "Uncaught TypeError: Cannot read property 'addEvent' of null" in Chromium 41.
You can see that behaviour here: http://diegoflorez.com/workspace/mootools.test/dom.ready.test.html

The same version of mootools-core-5.1 without the fix, doesn't throw any error message.

Thanks for your work and effort.

@SergioCrisostomo
Copy link
Member

I see now, nice with a demo page. So RequireJS is not providing the window object so MooTools cannot be loaded. I am not so at home with RequireJS but will take look at this again.

Maybe someone else more used to RequireJS will have some feedback soonish...

@diegoflorez
Copy link
Author

[Opening Again]

I have run 2 extra tests to this fix:
I have replaced the Mootools domReady and I have used the requireJS domReady,

  1. when using mootools.core.1.5.1.js it works perfect,
    (as you can see here: http://diegoflorez.com/workspace/mootools.test/dom.ready.test.html)
  2. but when using mootools.core.1.5.1.fix-2682.js throws the error message "TypeError: window is null"
    (as you can see here: http://diegoflorez.com/workspace/mootools.test/dom.ready.fix-2682.test.html)

@SergioCrisostomo
Copy link
Member

Using RequireJS's own domready seems the logical way, and best practise, since its RequireJS the one to load dependencies (MooTools). That said this issue can be closed IMO. I will close it in some days if no one else has input on it.

As a side note I still think the fix is usefull, but that we can discuss in the pull request itself.

Thank you @diegoflorez for nice input and demos to analyse this problem.

@deadmann
Copy link

deadmann commented Jul 4, 2015

i'm not sure if it help or not,

but also i receive error on webstorm intelisense for this part:

line: 5848

// Make sure that domready fires before load
Element.Events.load = {
    base: 'load',
    onAdd: function(fn){
        if (loaded && this == window) fn.call(this);
    },
    condition: function(){
        if (this == window){
            domready(); ------Error is on this line -> invalid call target------
            delete Element.Events.load;
        }
        return true;
    }
};

my version is 1.4.5 (get from nuget)
I don't see this error on minified version

@SergioCrisostomo SergioCrisostomo modified the milestones: 1.5.2, 1.5.3 Sep 14, 2015
@mootools mootools deleted a comment Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants