Wrong embedded content could be displayed when site contains multiple embedded posts from the same service and visitor accepted its cookies after the site loaded

+1 I can confirm that this also happens to me with multiple Vimeo and Youtube videos on the same page. It duplicates the first iframe across all iframes.

Drupal Version
9.4.8

Web Server
nginx/1.22.0
 
PHP
Version
8.0.21 (more information)

Hi all,

I wasn't able to reproduce this yet. It sounds as your live sites are affected? If yes, would you mind to share an example URL to test this?
Hopefully we can fix this ASAP then.

I guess there's something wrong in the JS implementation. Which exact Cookies module version are you using?

Best would be to have a JavaScript tests reproducing what's visible, for example using three YouTube Videos, but I guess that test will be quite complicated to set up @Grevil?

Furthermore in a perfect world, we'd need the test for every submodule to ensure they all work.

For your manual tests, youtube might be the easiest integration with Drupal Core Media Remote Video.

Can confirm, that this issue is reproducible, we are working on a fix ASAP!

@Grevil my guess is, that the var in the for loop is only set at the first run and never overwritten then. Seem to be a special case in JavaScript loops that we should be aware of?

Still not sure, as it looks crazy, but I think this might be the faulty line:
var $element = $(element);

For our own interest, please also check if a let instead of var would fix it.
Then we should use that in the Drupal 10 Cookies release with Vue...

But first, please try, if my assumption is right.
And if yes, this needs to be fixed in every submodule using a similar implementation.

Confirming we can reproduce the issue and we're quite confident we found the reason. Working on it with @Grevil!

@Grevil I think I finally found it:
Seems this was the key issue. We were looking for the wrapper globally, instead of relative:

Wrong:
var $wrapper = $(document).find('.' + serviceBaseWrapperStr);

Correct:
var $wrapper = $(this).parent('.' + serviceBaseWrapperStr);

Still the other fixes were also relevant, I think.

I guess it's worth to have JS tests to ensure
a) only the selected service type is unlocked
b) the result is exactly the same after unlocking as it's after a page reload

Please first test manually now, that my fix solves the issue and doesn't introduce new issues, hopefully. That's what the tests will also be good for.

@kaszarobert and @lexsoft please check, if the MR fixes the issue for you or if you find other issues with this fix now. Quite complicated part here... -.-

FYI: I added just this fix as simple experiment in !56 to see if that would also work.

MR!56 also fixes the issue and is less disruptive.

For that reason I'm going to commit this now as quick-fix. @Grevil afterwards I think we should add tests here and if we find further issues, let's look into MR!55 again, otherwise let's close that far more complicated one. Seems my assumptions regarding the variables here were not matching (in this specific case - the general logic hint is still important for (event) callbacks!)

Merged MR!56 as simple fix to at least remove this clear logical mistake and marked the complex MR!55 as Draft.
Let's proceed with tests and feedback here now!

Let's create another issue for creating the tests, so this issue won't get cluttered.