Hi folks,

in line 71 of fb.process.inc you state the following:
// Add target=_top so that entire pages do not appear within an iframe.
// TODO: make these pattern replacements more sophisticated, detect whether target is already set.

so attached you find a patch that addresses this issue.

BUT: i could not come up with an regexp that handles this correctly so i used the preg_replace_callback for this special case.

also for the callback to handle the target like defined, i made the $target variable global - perhaps the name should be improved.

please review!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Cohen’s picture

Status: Needs review » Needs work

Thanks for the patch. are you running into an issue with this, or just doing it because you noticed the comment?

One problem is performance. The regexp is already an expensive thing to do, and for most sites it is done on all pages. Your change goes from one preg_replace per page to both one preg_replace and one preg_replace_callback. I think it would be much better to do only one (of either).

Another problem is the global. Is there a way to avoid that entirely? If not, the global should start with _fb, so something like $_fb_process_target.

longbao738’s picture

Version: 6.x-3.0-rc13 » 6.x-3.3
FileSize
1.6 KB

How about add target to the end of 'a' tag?

Currently, the target=_top is added before href
<a target="_top" href="http://www.facebook.com/webio.cc" target="_blank">

Propose to add the target=_top to the end
<a href="http://www.facebook.com/webio.cc" target="_blank" target="_top">

We'd like to have users open our Facebook page in another browser tab or window.
It seems to work.