B-Interactive / cloudflare-stream-wordpress

A fork of the official Cloudflare Stream plugin for WordPress.
GNU General Public License v2.0
16 stars 3 forks source link

Not all URL parameters work in iframe embed player #14

Open davidmpurdy opened 2 years ago

davidmpurdy commented 2 years ago

It appears that the iframe player treats boolean parameters that are false by default as "true" if they are present at all in the querystring (even if they are set to false).

Affected parameters I've identified are muted, loop, and autoplay. (The behavior is documented for autoplay.)

For example, the following iframe src results in a video that does autoplay, start muted, and loop. (But does not show controls.)

https://iframe.cloudflarestream.com/uid123456?muted=false&preload=false&loop=false&autoplay=false&controls=false

I found this updating the block, but it affects shortcodes as well. For example, the following shortcode generates a URL like the one above which treats autoplay, loop, and muted as effectively true.

[cloudflare_stream uid="[uid]" controls="false" autoplay="false" loop="false" preload="false" muted="false"]

I've raised the issue of at least documenting the behavior, if not allowing false values.

I think the best solution is to add some logic to where querystring parameters are added in Cloudflare_Stream_API::get_video_embed to simply not add parameters that are falsy or "false".

I'm working on a PR now.

B-Interactive commented 2 years ago

Playing with the embed options in the Cloudflare Stream console, it would seem to confirm what you've found, and also for preload. The assumption that specifying the inverse would set it as such, was an error on my part.

Default when no options specified:

davidmpurdy commented 2 years ago

I didn't include preload because in my testing it looks to work a little differently - it appears to pass through to the <video> element if the value is valid, but if I recall seems to work OK with mapping false to none and true to auto.

I meant to raise a separate issue of how to handle it. The Stream documentation seems to suggest that it wants the HTML enumerated options, so maybe we should add a line to map true and false values on our end (seems worth keeping them working for backwards compatibility).

B-Interactive commented 2 years ago

Fix implemented in 1cb20385ab9c7a9754a29d0b7315b36afadb45f5.

davidmpurdy commented 2 years ago

These seem to be broken again for me. At first glance from inspecting the page source and the iframe URL, I'm wondering if it has something to do with encoded ampersands...?

B-Interactive commented 2 years ago

Hopefully fixed now in 8b0b131890e9af0edca9ac5b25a8a0697cfae6a7.

I've reverted to the use of & instead of %26 and simultaneous multiple parameters test as working in LibreWolf under Linux and Edge under Windows.

The available advice was to percentage encode ampersands to %26 when used in URI's. I must be missing something because that clearly failed.