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

fix duplicate body tags #316

Closed
jodeleeuw opened this issue Jan 4, 2017 · 6 comments · Fixed by #948
Closed

fix duplicate body tags #316

jodeleeuw opened this issue Jan 4, 2017 · 6 comments · Fixed by #948
Labels
Milestone

Comments

@jodeleeuw
Copy link
Member

jsPsych adds a to the DOM if one doesn't exist and the display_element is not set. But Chrome, and maybe other browsers, automatically adds a when the document finishes loading. This causes duplicate tags. Can we get Chrome to recognize that a tag has been added via the script?

@jodeleeuw jodeleeuw modified the milestone: 6.0 Jan 4, 2017
@jodeleeuw
Copy link
Member Author

too much of a hassle with no clear solution. instead, just use a body tag!

@bjoluc
Copy link
Member

bjoluc commented Apr 24, 2020

Stumbled upon this one too. It also happens when jsPsych.init() is called from a script within the head tag, as the body is not set by then. The clear solution for all of these situations would be wrapping the internals of core.init into window.onload = function () { ... }. I validated that this works with a few of my projects, but the change fails 31 test suites which do not mimic browser behavior accordingly.

@jodeleeuw Are you still interested in working on this? I can offer my help in this case.

Edit:
Found the problem with my suggestion: It didn't work when document.readyState was complete already, or if any other script used window.onload.
The proper way to do this is

core.init = function (options) {
  function init() {
    // ...
  }
  if (document.readyState === "complete") {
    init();
  } else {
    window.addEventListener("load", init);
  }
}

This way, the tests pass as well.
@jodeleeuw Would you accept a PR?

@bjoluc
Copy link
Member

bjoluc commented Aug 3, 2020

@jodeleeuw Are you willing to get this fixed? If so, I'm glad to do a PR.

@jodeleeuw
Copy link
Member Author

Yes! Sorry I dropped the ball on this. Getting this into 6.2 would be great.

@becky-gilbert
Copy link
Collaborator

@bjoluc thanks very much for the fix, and sorry about the delay getting to this. You mentioned that you have some tests that cover this - do you have any written in Jest that could be added to jspsych/tests? If not then Josh or I can do it 👍

@bjoluc
Copy link
Member

bjoluc commented Oct 28, 2020

@becky-gilbert Thanks, I added two test cases for my change now (init.test.js)!

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.

3 participants