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

1277 - Bug fix: Kaltura video player has various issues #1282

Open
wants to merge 511 commits into
base: master
Choose a base branch
from

Conversation

wynnset
Copy link
Collaborator

@wynnset wynnset commented Nov 14, 2022

Changes

  • Ensure Kaltura player fits well in multi-content
  • Fix issue where the player would not show the user progress and always show it's at 0
  • Fix issue where initial user seeking was not working (would always start at 0)
  • Fix issue where player would play in the background with pop-up on top if seeked past a pop-up time

Issue Linkage

Closes #1277

Automated Testing

No changes to tests

zifgu and others added 30 commits June 21, 2022 16:00
- This is necessary because the Kaltura upload changes the node data in the backend, but the changes are not propagated to the frontend.
- Should implement a better solution in the future
- Also renamed the Kaltura upload function, since the old name was somewhat confusing
- Also fixes some double quotes to be single quotes
- Also add a notice to inform the user when an upload is in progress and recommend them not to clean the videos
- Change DEV mode to be activated from wp-config
- Automatically set node version to v16 when using `npm start`
@wynnset wynnset added needs first review Code has not been reviewed yet needs final review Has been tested and reviewed once and needs another code review to become ready for merge and removed needs first review Code has not been reviewed yet labels Nov 14, 2022
@cypress
Copy link

cypress bot commented Nov 14, 2022

2 flaky tests on run #3974 ↗︎

0 100 0 0 Flakiness 2

Details:

Merge branch '1213-video-captions' into 1277-kaltura-fixes
Project: tapestry-wp Commit: 650295a72d
Status: Passed Duration: 05:33 💡
Started: May 5, 2023 12:05 AM Ended: May 5, 2023 12:10 AM
Flakiness  node-authoring.spec.js • 1 flaky test

View Output Video

Test Artifacts
Node Authoring > should not be able to delete a non-leaf node Output Screenshots Video
Flakiness  lightbox/multicontent.spec.js • 1 flaky test

View Output Video

Test Artifacts
Multi-content > In-place > should be able to add/edit/delete multi-content child node in-place Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@wynnset wynnset changed the base branch from 1213-video-captions to master November 29, 2022 01:12
@wynnset wynnset changed the base branch from master to 1213-video-captions November 29, 2022 01:13
Copy link
Contributor

@legendword legendword left a comment

Choose a reason for hiding this comment

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

The Kaltura fixes are working and the refactoring looks good. Just waiting to test if loading multiple Kaltura scripts from different Kaltura servers breaks anything (when calling kWidget.embed, the kWidget global variable can only be from the latest loaded kWidget script).

Base automatically changed from 1213-video-captions to master June 8, 2023 18:34
@legendword
Copy link
Contributor

The Kaltura fixes are working and the refactoring looks good. Just waiting to test if loading multiple Kaltura scripts from different Kaltura servers breaks anything (when calling kWidget.embed, the kWidget global variable can only be from the latest loaded kWidget script).

Indeed, after testing, two videos from different Kaltura servers will not be played properly, especially of they are in the same multi-content node. The Kaltura script that is loaded last will overwrite any existing kWidget global variable. Currently we are not planning to fix this, as most Tapestry sites using Kaltura are only using videos from one server. If a situation arises that require more than one Kaltura server to be used, this issue will need to be reconsidered.

@legendword
Copy link
Contributor

After reviewing the PR description, there is one item that might have been fixed but is now still a problem:

  • Fix issue where player would play in the background with pop-up on top if seeked past a pop-up time

Some notes after I did a bit of testing (for context, this is a Kaltura video with Kaltura player and a popup node at 10 seconds). Tried seeking from before 10s to after 10s multiple times. The popup does not trigger every time (see below).

Scenario 1: lastTime update happens after seeking, meaning the lastTime is already updated and it is not considered to have sought over the 10s threshold. The popup does not trigger.
image

Scenario 2: lastTime update happens before seeking, so the popup at 10s triggers. The pause command is sent to the Kaltura player, yet the video is not paused.
image

If playing video normally without seeking, the popup always shows up normally and the video is paused normally:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs final review Has been tested and reviewed once and needs another code review to become ready for merge needs testing Has been tested by author and needs a second pair of eyes to test it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Kaltura video player has various issues
4 participants