Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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!
Comment | File | Size | Author |
---|---|---|---|
#2 | fb.process.inc_.diff | 1.6 KB | longbao738 |
fb.process.inc_.target.patch | 2.46 KB | neurovation.kiwi |
Comments
Comment #1
Dave Cohen CreditAttribution: Dave Cohen commentedThanks 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.
Comment #2
longbao738 CreditAttribution: longbao738 commentedHow 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.