Flash container inserted too soon

bjoerge commented 11 years ago

The element containing the flash embed code is tried appended to document.body at a time when document.body may not be set (depending on when audiojs is initialized).

I.e. the following will break flash fallback:

<html>
<head>
  <script src="/js/audio5.js"></script>
  <script>
    function initAudio () {
      var audio5js = new Audio5js({
        ready: function () {
          this.load('/someaudio.mp3');
          this.play();
        }
     });
    }
    initAudio();
  </script>
  </head>
<body>
(...)

Offending line is this: zohararad/audio5js/src/audio5.js#L209

zohararad commented 11 years ago

Hmm it seems like the wrong place to initialize the code. Why are you not initializing the code at the end of the body?

bjoerge commented 11 years ago

I don't see any good reason for this library to make assumptions about the DOM state. Moreover, this is not a problem in browsers supporting HTML5 audio. As Audio5js is a compatibility layer and the flash fallback should do its best to avoid introducing bugs in environments is providing compatibility for.

The fix is simple, just wrap the document.body.appendChild(d) in a domready event handler.

bjoerge commented 11 years ago

Btw, the code above were just an illustrating example, I'm creating an instance of Audio5js in a script loaded via <script src="the-script.js" async defer></script>. This tag is located in <head> and that makes perfectly sense to me.

zohararad commented 11 years ago

Errr... the library does make the following (justifiable) assumptions:

It's considered good practice to wait for end of or DOM readiness to run your JS code.
You make some sacrifices if you need Flash-based audio because of the nature of Flash in the browser.

I don't think the library should concern itself with DOM readiness (which is in itself incompatible between browsers). If you implement the library, you should be mindful of the DOM manipulation in FF, which requires DOM readiness and implement the code accordingly.

I'm happy to add a note in README at reminds people of this, but I see no reason to change the code to check for DOM readiness.

bjoerge commented 11 years ago

It's considered good practice to wait for end of or DOM readiness to run your JS code.

That is true for the parts of the JS code that interacts with the DOM. There are perfectly good reasons to run javascript that doesn't care about the DOM before the DOM ready (side point: I don't really feel it is in the Audio5js mandate to enforce good practice on its users, it is a compatibility layer after all).

You make some sacrifices if you need Flash-based audio because of the nature of Flash in the browser.

That is true for the unavoidable sacrifices (like flash installed). However, my point is that "wait for dom ready" is a hidden requirement that isn't really necessary and are causing surprising errors. Sure, a note in the README will help those who fine read it, but is in my opinion not a proper fix.

Anyways, I've fixed this by including the domready lib in my fork. I sense that a pull request is not too welcome here, so I'll just keep it to myself for now ;-)

zohararad commented 11 years ago

Hmm, i'm afraid we'll have to agree to disagree. My sentiment is that Audio5js should solve HMTL5 audio compatibility issues and that's all.

Yes, DOM readiness is a hidden requirement, but it's also a hidden requirement for the Facebook JS SDK which doesn't immediately strikes one as DOM-reliant. I don't see why you can't simply use your own DOM readiness mechanism inside the section of your code, since you're aware of that requirement.

In other words, why should Audio5js be responsible for DOM readiness detection, when:

Audio5js is an HTML5 audio compat. layer, rather than a DOM readiness compat. layer
The developer most likely already uses a DOM library and has the facility to detect DOM readiness in a cross platform way.
For the most part Audio5js doesn't manipulate the DOM

zohararad / audio5js

Flash container inserted too soon #17