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

Calling sharing_add_footer after (!) wp_print_footer_scripts in wp_footer to ensure jQuery dependancy #1223

Open
kasparsd opened this issue Oct 27, 2014 · 14 comments
Labels
[Feature] Sharing Post sharing, sharing buttons [Pri] Normal [Status] Stale [Type] Bug When a feature is broken and / or not performing as intended

Comments

@kasparsd
Copy link
Contributor

kasparsd commented Oct 27, 2014

Currently sharing_add_footer is being attached to wp_footer with priority 10 which is before WordPress has printed the footer scripts as wp_print_footer_scripts is attached to wp_footer with priority 20.

sharing_add_footer echoes inline Javascript during display_footer() (here is an example) which depends on jQuery being already available. This fails in cases when jQuery is enqueued in the footer.

jquery-error

The solution is to move sharing_add_footer to a later priority:

add_action( 'wp_footer', 'sharing_add_footer', 25 );
@jeherve jeherve added [Feature] Sharing Post sharing, sharing buttons [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Oct 27, 2014
@ronalfy
Copy link

ronalfy commented Oct 28, 2014

+1 for this. Alternative is to remove the footer calls and use the wp_enqueue_scripts hook and do:

global $wp_scripts;
$wp_scripts->add_data( 'jquery', 'data', $jetpack_sharing_scripts );

I can submit a pull request if this would be ideal. A drawback is that the scripts will show up in the header if jQuery is enqueued in the header.

@ronalfy
Copy link

ronalfy commented Oct 28, 2014

An elegant baked-in solution would be nice. Too many hacks in this script to get this to work with enqueuing: https://gist.github.com/ronalfy/3d5de0527095ff00907b

@cfxd
Copy link
Contributor

cfxd commented Feb 19, 2015

@kasparsd @ronalfy

Just wanted to bump this, since it's causing jQuery console errors in themes with jQuery properly enqueued in the footer.

Perhaps we can get a temporary fix as @kasparsd suggested of changing the action priority?

My git is acting funny but a PR would be much appreciated!

samhotchkiss added a commit that referenced this issue Feb 26, 2015
…-priority

Change "sharing_add_footer" priority to temporarily fix jQuery undefined console errors when using ShareDaddy with jQuery enqueued in the footer (#1223)
jeherve added a commit that referenced this issue Mar 2, 2015
Priority higher or equal to 20 causes the sharing library not to be enqueued at all in some cases

See #1223
@samhotchkiss samhotchkiss modified the milestone: Needs Triage Aug 28, 2015
jeherve referenced this issue Jan 7, 2016
…ned console errors when using ShareDaddy with jQuery enqueued in the footer
@jeherve jeherve modified the milestones: Community, Needs Triage Feb 15, 2016
@dboulet
Copy link

dboulet commented Apr 19, 2016

I’m still seeing this issue with Jetpack 3.9.6 and WP 4.5. Changing the action priority of sharing_add_footer() won’t work, because it itself enqueues another script.

In WP 4.5, there is wp_add_inline_script(), which could possibly be used to fix this. Is that doable? Is Jetpack required to support older versions of WP?

https://developer.wordpress.org/reference/functions/wp_add_inline_script/

@jeherve
Copy link
Member

jeherve commented Apr 20, 2016

Is Jetpack required to support older versions of WP?

Yes, we aim for Jetpack to be compatible with the current version and one major version before that, so we'll need to wait for WordPress 4.6 to be released before to require 4.5+.

@jeherve jeherve added [Pri] Normal [Type] Bug When a feature is broken and / or not performing as intended and removed [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Nov 25, 2016
@jeherve jeherve modified the milestones: 4.5, Community Nov 25, 2016
@jeherve
Copy link
Member

jeherve commented Nov 25, 2016

Also reported in 59964-zd-vip

@kasparsd
Copy link
Contributor Author

I updated my original report with a link to the actual script from Share_Email which calls the jQuery directly.

@grappler
Copy link

grappler commented Feb 9, 2017

Yes, we aim for Jetpack to be compatible with the current version and one major version before that, so we'll need to wait for WordPress 4.6 to be released before to require 4.5+.

@jeherve 4.7 has been released so we could create a PR for this?

@kasparsd
Copy link
Contributor Author

kasparsd commented Feb 9, 2017

@jeherve I think the JS document.getElementById('jetpack-source_f_name').value = '' isn't needed anymore since the value of that field is no longer set to hello.

@jeherve
Copy link
Member

jeherve commented Feb 9, 2017

@grappler Yes. It looks like @kasparsd already took care of it (thank you!). You can test #6339 and give your feedback there if you'd like!

dereksmart pushed a commit that referenced this issue Feb 10, 2017
* Remove unused JS logic

Fixes #1223

* Print inline scripts after WP is done with footer scripts

Ensures that dependencies are met.
@jeherve
Copy link
Member

jeherve commented Mar 14, 2017

Reopening as we had to revert this in #6649.

This was explained in #6641.

Next steps to get this fixed once and for all are described here:
#6641 (comment)

@jeherve jeherve reopened this Mar 14, 2017
@jeherve jeherve removed this from the Not Currently Planned milestone Mar 14, 2017
@aheckler
Copy link
Member

3128393-t

@lamdayap
Copy link

related: link

@stale
Copy link

stale bot commented Jul 3, 2018

This issue has been marked as stale. This happened because:

  • It has been inactive in the past 6 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Sharing Post sharing, sharing buttons [Pri] Normal [Status] Stale [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants