Automattic / camptix

Moved to https://github.com/WordPress/wordcamp.org/
177 stars 97 forks source link

camptix.js code not working because of timing condition. (tix-js, has-dynamic-js, wp_footer) #241

Open EliW opened 5 years ago

EliW commented 5 years ago

Hey team. So I recently noticed on a website that the display looked a little different. Specifically the "should the receipt go to this person" radio button text was appearing, which is supposed to be hidden if Javascript is working.

I started looking into it, and it looks like there's a bug in how things are being done, timing wise.

Basically the block of in camptix.js, starting at line 20 in the ($) block, isn't executing properly. This section:

/**
 * CampTix Javascript
 *
 * Hopefully runs during wp_footer.
 */
(function($){

And it's documented exactly why it isn't. That code, as designed, has to be run in the footer, because it expects that the entire page needs to be loaded first. The page isn't, and so basically all that code in that ($) block fails silently.

However, the script being attached in camptix.php, doesn't have the 'in footer' flag turned on. It looks like this starting on line 474:

        wp_register_script(
            'camptix',
            plugins_url( 'camptix.js', __FILE__ ),
            array( 'jquery' ),
            filemtime( __DIR__ . '/camptix.js' )
        );

There are two possible solutions. One is just to update that code to put 'true' in the footer field, ie:

        wp_register_script(
            'camptix',
            plugins_url( 'camptix.js', __FILE__ ),
            array( 'jquery' ),
            filemtime( __DIR__ . '/camptix.js' ),
            true
        );

The other (or in addition to) is to change the (function($) in the javascript, to be a proper document.ready ... so that it doesn't matter where it's included. It won't actually execute until the page is ready. IMO, this is the proper fix regardless of whether the code is included in the footer or header. Because then it doesn't matter.

I'm happy to submit a pull request fixing this. Just would like guidance as to the preferred fix (attach in footer, document.ready, both)

So that I don't submit a PR that gets rejected because the other one is preferred.

EliW commented 5 years ago

Tagging @coreymckrill again, just to make sure this gets seen since it's a real bug w/ consequences for folks at the moment, and a fix might be wanted soon.

coreymckrill commented 5 years ago

@EliW thanks for pinging. This issue was discussed in our bug scrub chat today: https://wordpress.slack.com/archives/C08M59V3P/p1566494342140000

Both changes seem worth doing, although we will need to test to make sure there aren't any negative side effects. I'm not sure why the in_footer flag isn't currently set to true. It could be a mistake, but there might also be a legitimate reason.

Are you still up for submitting a PR for this?