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

Add missing controls property to media containers #761

Merged
merged 2 commits into from
Sep 13, 2021

Conversation

Jaifroid
Copy link
Member

Fixes #760. Although that should be fixed in the YouTube scraper, it will take a long time for that fix (if it is accepted) to come through to new ZIMs. This patch is a one-liner in the specific jQuery code that inserts media blobs, so I think it is worth doing, as it will redstore the ability to play videos to existing ZIM archives as well.

I tested this on khan-academy-videos_en_special-relativity-physics-khan-academy_2021-03.zim.

Copy link
Contributor

@mossroy mossroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's a jQuery-specific improvement that we decided to avoid.
But this one is very straightforward, has a significant impact, and looks very safe.

I did not test, but as long as you did, it's fine for me

@Jaifroid
Copy link
Member Author

Well, it's a jQuery-specific improvement that we decided to avoid.
But this one is very straightforward, has a significant impact, and looks very safe.

I agree it's jQuery-specific, but I wouldn't call it an improvement. I would say it comes under the category of a bugfix to keep existing functionality, which I believe we said we'd do if it's not too complicated.

I tested on Edge Chromium, Edge Legacy and Firefox 92, with khan-academy-videos_en_special-relativity-physics-khan-academy_2021-03.zim. IE11 shows the controls (with this patch), but says "Invalid source...". This is because IE11 doesn't support WebM. On KJSW, I offer to a download of the video if the player doesn't support the format (IE11, Edge Mobile), but we don't do that on Kiwix JS.

I still need to do some more testing. I need to download some recent TED talks, to see if the issue is in those ZIMs (or not) and if this patch works with them. I'll post the results here.

@Jaifroid
Copy link
Member Author

Slightly safer code, conforming to the spec. See https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute:

Boolean attributes are considered to be true if they're present on the element at all. You should set value to the empty string ("") or the attribute's name, with no leading or trailing whitespace. See the example below for a practical demonstration.

In other words, we shouldn't strictly set the attribute to "true", even though it works. We just have to ensure the attribute is present. See also https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video#attr-controls .

@Jaifroid
Copy link
Member Author

I downloaded ted_en_global_issues_2021-08.zim, which turns out not to have this issue. However, the TED scraper is different from the YouTube scraper, and it appears from openzim/youtube#150 that the missing attribute is caused by the latter. In any case, this patch makes at least Khan Academy ZIMs useable in jQuery mode, and they're an important educational resource.

@Jaifroid Jaifroid merged commit 28223d0 into master Sep 13, 2021
@Jaifroid Jaifroid deleted the Add-missing-controls-property-to-media-containers branch September 13, 2021 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Khan Academy videos do not play in jQuery mode
2 participants