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

turbo isn't working with adsense #834

Closed
eladmarg opened this issue Dec 30, 2022 · 7 comments · Fixed by #835
Closed

turbo isn't working with adsense #834

eladmarg opened this issue Dec 30, 2022 · 7 comments · Fixed by #835

Comments

@eladmarg
Copy link

eladmarg commented Dec 30, 2022

I didn't find the root cause for this issue, but if adsense on page, links aren't working with turbo.
(every link will trigger full page reload)

this start happen after 7.1 version (on 7.1 works ok, on 7.2-beta-1 or higher the problem exists).

if adsense isn't presenting, everything works as expected.

@eladmarg
Copy link
Author

update - found the issue, the function doesNotTargetIFrame(anchor)
is getting as parameter empty string (because the link target is empty)
this cause the selector to get all iframes in, and then it's not handled by turbo.

@eladmarg
Copy link
Author

the fix is simple,
just add this as another check.
if someone can create a PR, it would be great.

function doesNotTargetIFrame(anchor: HTMLAnchorElement): boolean {
if (anchor.target === "")
return true;

@seanpdoyle
Copy link
Contributor

@eladmarg thank you for opening this issue.

Making an implementation change seems straightforward, but making that change without a test case wouldn't prevent future changes to re-introduce a regression.

Could you share some more information like an error stack trace or a JSFiddle with a minimum snippet of code to reproduce the behavior so that we can also include test coverage in the fix?

@eladmarg
Copy link
Author

sure,
very simple,
when you queryselector by empty name, you can get iframes if exists on the page (document.getElementsByTagName(""))

so simply add to the page an iframe of google ads

this for instance:

<div id="google_ads_iframe_/4744526/ads_cat_top_desktop_0__container__" style="border: 0pt none; display: inline-block; width: 728px; height: 90px;"><iframe frameborder="0" src="https://5642dc6e614dd1d0e97dec6070467bae.safeframe.googlesyndication.com/safeframe/1-0-40/html/container.html" id="google_ads_iframe_/4744526/ads_cat_top_desktop_0" title="3rd party ad content" name="" scrolling="no" marginwidth="0" marginheight="0" width="728" height="90" data-is-safeframe="true" sandbox="allow-forms allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts allow-top-navigation-by-user-activation" role="region" aria-label="Advertisement" tabindex="0" data-google-container-id="1" style="border: 0px; vertical-align: bottom;" data-load-complete="true"></iframe></div>

then you'll see that its get selected when you click on another anchor element.

@seanpdoyle
Copy link
Contributor

Thank you for sharing that snippet!

I've extracted the <iframe> element and broken it across several lines to make reading it easier:

<iframe
  frameborder="0" 
  src="https://5642dc6e614dd1d0e97dec6070467bae.safeframe.googlesyndication.com/safeframe/1-0-40/html/container.html"
  id="google_ads_iframe_/4744526/ads_cat_top_desktop_0"
  title="3rd party ad content"
  name=""
  scrolling="no"
  marginwidth="0"
  marginheight="0"
  width="728" height="90"
  data-is-safeframe="true"
  sandbox="allow-forms allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts allow-top-navigation-by-user-activation"
  role="region"
  aria-label="Advertisement"
  tabindex="0"
  data-google-container-id="1"
  style="border: 0px; vertical-align: bottom;"
  data-load-complete="true"></iframe>

Note the name="" attribute. The presence of that attribute is at the root of the issue.

I'll incorporate some tests into a pull request.

@eladmarg
Copy link
Author

awesome! thank you very much

seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Dec 30, 2022
Closes [hotwired#834][]

When applications embed [Google Adsense][]-powered `<iframe>` elements,
the snippets they provide render them **with** a `[name]` attribute
that's set to the empty string `""`:

```html
<iframe
  frameborder="0"
  src="REDACTED"
  id="google_ads_iframe_/REDACTED"
  title="3rd party ad content"
  name=""
  scrolling="no"
  marginwidth="0"
  marginheight="0"
  width="728" height="90"
  data-is-safeframe="true"
  sandbox="allow-forms allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts allow-top-navigation-by-user-activation"
  role="region"
  aria-label="Advertisement"
  tabindex="0"
  data-google-container-id="1"
  style="border: 0px; vertical-align: bottom;"
  data-load-complete="true"></iframe>
```

Note the `[name=""]` in the snippet.

The guard clauses in the `FormSubmitObserver` and `LinkClickObserver`
classes that prevent Turbo Drive from interfering with `<a>` clicks and
`<form>` submissions that target `<iframe>` elements do not account for
the presence of an `<iframe name="">`. This commit extends those guard
clauses to first check for the presence of `a[target]`, `form[target]`,
and `input[formtarget]` or `button[formtarget]` attributes before
searching the document for an `iframe[name]` that matches.

Additionally, it adds tests to cover a special-case scenario where there
**is** an `iframe[name=""]` **and** an element that targets it (for
example, `a[target=""]`).

For example, consider the following example (along with a Turbo-less
[JSFiddle][] that reproduces the behavior):

```html
<iframe name=""></iframe>

<a href="https://example.com" target="">Targets [name=""]</a>
```

When clicked, the `<a>` element drives the entire page. In our test
suite, there are test cases that cover this behavior, and ensure that
Turbo doesn't interfere in these scenarios.

[Google Adsense]: https://www.google.com/adsense/start/
[hotwired#834]: hotwired#834
[JSFiddle]: https://jsfiddle.net/hk6587oz/
seanpdoyle added a commit to seanpdoyle/turbo that referenced this issue Dec 30, 2022
Closes [hotwired#834][]

When applications embed [Google Adsense][]-powered `<iframe>` elements,
the snippets they provide render them **with** a `[name]` attribute
that's set to the empty string `""`:

```html
<iframe
  frameborder="0"
  src="REDACTED"
  id="google_ads_iframe_/REDACTED"
  title="3rd party ad content"
  name=""
  scrolling="no"
  marginwidth="0"
  marginheight="0"
  width="728" height="90"
  data-is-safeframe="true"
  sandbox="allow-forms allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts allow-top-navigation-by-user-activation"
  role="region"
  aria-label="Advertisement"
  tabindex="0"
  data-google-container-id="1"
  style="border: 0px; vertical-align: bottom;"
  data-load-complete="true"></iframe>
```

Note the `[name=""]` in the snippet.

The guard clauses in the `FormSubmitObserver` and `LinkClickObserver`
classes that prevent Turbo Drive from interfering with `<a>` clicks and
`<form>` submissions that target `<iframe>` elements do not account for
the presence of an `<iframe name="">`. This commit extends those guard
clauses to first check for the presence of `a[target]`, `form[target]`,
and `input[formtarget]` or `button[formtarget]` attributes before
searching the document for an `iframe[name]` that matches.

Additionally, it adds tests to cover a special-case scenario where there
**is** an `iframe[name=""]` **and** an element that targets it (for
example, `a[target=""]`).

For example, consider the following example (along with a Turbo-less
[JSFiddle][] that reproduces the behavior):

```html
<iframe name=""></iframe>

<a href="https://example.com" target="">Targets [name=""]</a>
```

When clicked, the `<a>` element drives the entire page. In our test
suite, there are test cases that cover this behavior, and ensure that
Turbo doesn't interfere in these scenarios.

[Google Adsense]: https://www.google.com/adsense/start/
[hotwired#834]: hotwired#834
[JSFiddle]: https://jsfiddle.net/hk6587oz/
@eladmarg
Copy link
Author

fixed.

dhh pushed a commit that referenced this issue Dec 31, 2022
Closes [#834][]

When applications embed [Google Adsense][]-powered `<iframe>` elements,
the snippets they provide render them **with** a `[name]` attribute
that's set to the empty string `""`:

```html
<iframe
  frameborder="0"
  src="REDACTED"
  id="google_ads_iframe_/REDACTED"
  title="3rd party ad content"
  name=""
  scrolling="no"
  marginwidth="0"
  marginheight="0"
  width="728" height="90"
  data-is-safeframe="true"
  sandbox="allow-forms allow-popups allow-popups-to-escape-sandbox allow-same-origin allow-scripts allow-top-navigation-by-user-activation"
  role="region"
  aria-label="Advertisement"
  tabindex="0"
  data-google-container-id="1"
  style="border: 0px; vertical-align: bottom;"
  data-load-complete="true"></iframe>
```

Note the `[name=""]` in the snippet.

The guard clauses in the `FormSubmitObserver` and `LinkClickObserver`
classes that prevent Turbo Drive from interfering with `<a>` clicks and
`<form>` submissions that target `<iframe>` elements do not account for
the presence of an `<iframe name="">`. This commit extends those guard
clauses to first check for the presence of `a[target]`, `form[target]`,
and `input[formtarget]` or `button[formtarget]` attributes before
searching the document for an `iframe[name]` that matches.

Additionally, it adds tests to cover a special-case scenario where there
**is** an `iframe[name=""]` **and** an element that targets it (for
example, `a[target=""]`).

For example, consider the following example (along with a Turbo-less
[JSFiddle][] that reproduces the behavior):

```html
<iframe name=""></iframe>

<a href="https://example.com" target="">Targets [name=""]</a>
```

When clicked, the `<a>` element drives the entire page. In our test
suite, there are test cases that cover this behavior, and ensure that
Turbo doesn't interfere in these scenarios.

[Google Adsense]: https://www.google.com/adsense/start/
[#834]: #834
[JSFiddle]: https://jsfiddle.net/hk6587oz/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants