Skip to content

Conversation

@vinespie
Copy link
Contributor

@vinespie vinespie commented May 23, 2016

Pull Request for Issue #9623 .

Summary of Changes

Defer implementation of the script google recaptcha.

Before patch

When you go to single contact page with Internet Explorer, Google Recaptcha does not display all the time.

Testing Instructions

  1. Install joomla 3.5.1 with sample data for testing, apply patch
  2. Publish and configure recaptcha plugin
    2016-05-23 13_41_41-joomla 3 - administration - plug-ins _ captcha - recaptcha
  3. Set "Captcha - ReCaptcha" in general configuration, default captcha
    2016-05-23 13_43_49-joomla 3 - administration - configuration
  4. With Internet Explorer, go to home page and navigate to "Single contact"
    2016-05-23 13_51_26-single contact
  5. Google Recaptcha must be display on contact form after multiple refresh or click on menu.

@piotr-cz
Copy link
Contributor

Which versions of IE are not compatible? I've tested with Edge, v11, v9.
You say that setting the defer attribute solves the problem?

@vinespie
Copy link
Contributor Author

Tested with IE 11 Edge Compatibility View issue #9623

yes, "Defer" attribute solve the problem line 62 :

JFactory::getDocument()->addScript($file, "text/javascript", true);

Generates :
<script src="https://www.google.com/recaptcha/api.js?onload=JoomlaInitReCaptcha2&render=explicit&hl=fr-FR"defer="defer"></script>`

See ReCaptcha documentation
https://developers.google.com/recaptcha/docs/display#explicit_render

@piotr-cz
Copy link
Contributor

piotr-cz commented May 23, 2016

I see. Documentation says that there is another solution: to load the callback first,
so then the code would look like:

JHtml::_('script', 'plg_captcha_recaptcha/recaptcha.min.js', false, true);
JHtml::_('script', $file);

I think I'd prefer this one as the code makes more sense. We could also make an inline comment about script loading order for future reference.
Can test it?

@vinespie
Copy link
Contributor Author

Yes it is possible to do like this.
I use "addScript" because with JHtml script method we can't had "defer" attributes as specified in the recaptcha documentation.
Indeed , I also tested this solution and it fixes the problem.

@piotr-cz
Copy link
Contributor

Okay, I can actually reproduce the bug in Edge. Changing script order fixes the issue

Changing load order and adding a comment.
Proposed by @piotr-cz
@vinespie
Copy link
Contributor Author

@piotr-cz , I 've considered your suggestion. Thanks.

@mawa7
Copy link

mawa7 commented May 23, 2016

Works like a charm in IE11. Thanks a lot!

@cr2a-graphique
Copy link

I have tested this item ✅ successfully on 2eba9fb


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10601.

@brianteeman
Copy link
Contributor

@piotr-cz can you give it one last test please and the I will make this RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10601.

@andrepereiradasilva
Copy link
Contributor

@Fedik since you made the change before. are you ok with this?

@Fedik
Copy link
Member

Fedik commented May 24, 2016

unfortunately I do not have IE to test now, but the changes looks good

@andrepereiradasilva
Copy link
Contributor

ok then. thanks

@piotr-cz
Copy link
Contributor

@brianteeman How do I mark successful Human Test Result?
btw: in comment above I've tested the patch and issue has been fixed


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10601.

@brianteeman
Copy link
Contributor

@piotr-cz you have to do it on the issue tracker not on github

@brianteeman
Copy link
Contributor

Thanks everyone RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10601.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 24, 2016
@brianteeman brianteeman added this to the Joomla 3.6.0 milestone May 24, 2016
@piotr-cz
Copy link
Contributor

I have tested this item ✅ successfully on 2eba9fb

As this is a bugfix and not a feature, can this be included in the current 3.5.x releases and not 3.6?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10601.

@brianteeman
Copy link
Contributor

@piotr_cz the next release is 3.6.0 there wont be another 3.5.x release

On 24 May 2016 at 12:51, Piotr [email protected] wrote:

I have tested this item ✅ successfully on 2eba9fb
2eba9fb

As this is a bugfix and not a feature, can this be included in the current

3.5.x releases and not 3.6?

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/10601
https://issues.joomla.org/tracker/joomla-cms/10601.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#10601 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@zero-24
Copy link
Contributor

zero-24 commented May 24, 2016

@piotr-cz
Copy link
Contributor

@zero-24 Yes, but this is for features that were planned for next minor release; IMHO according to semver, bugfixes should be added to current milestone so released as v3.5.2.

Personally I have more confidence in minimizing B/C issues when upgrading from v3.5.1 to v3.5.2 then to v3.6.0.

@brianteeman
Copy link
Contributor

brianteeman commented May 24, 2016 via email

@mbabker
Copy link
Contributor

mbabker commented May 24, 2016

There isn't a 3.5.2 release. What was going to be 3.5.2 is now 3.6.0.
Short of a major security issue at this point there won't be another 3.5
release.

On Tuesday, May 24, 2016, Piotr [email protected] wrote:

@zero-24 https://github.com/zero-24 Yes, but this is for features
that were planned for next minor release; IMHO according to semver,
bugfixes should be added to current milestone so released as v3.5.2.

Personally I have more confidence in minimizing B/C issues when upgrading
from v3.5.1 to v3.5.2 then to v3.6.0.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#10601 (comment)

@piotr-cz
Copy link
Contributor

okay

@roland-d roland-d merged commit c50837f into joomla:staging May 25, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 25, 2016
@roland-d
Copy link
Contributor

Thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.