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.

Sign up for GitHub

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

Fix TypeError when calling unbindVideoElement #3043

Merged
merged 5 commits into from
Feb 25, 2025
Merged

Fix TypeError when calling unbindVideoElement #3043

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
806baa2
Fix TypeError when calling unbindVideoElement
Feb 24, 2025
d1c4fe4
Fix typo
Feb 24, 2025
099ef48
Add expect
Feb 24, 2025
5bc9828
Add expect 2
Feb 24, 2025
52141ee
Improve unobserve
Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

### Fixed
Fix TypeError when calling unbindVideoElement

## [3.27.0] - 2024-12-11

Expand Down
13 changes: 8 additions & 5 deletions src/videotile/DefaultVideoElementResolutionMonitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import VideoElementResolutionMonitor, {
export default class DefaultVideoElementResolutionMonitor implements VideoElementResolutionMonitor {
private observerQueue = new Set<VideoElementResolutionObserver>();
private resizeObserver: ResizeObserver;
private element?: HTMLVideoElement;
private element: HTMLVideoElement | null = null;

constructor() {
this.resizeObserver = new ResizeObserver(entries => {
Expand All @@ -32,12 +32,15 @@ export default class DefaultVideoElementResolutionMonitor implements VideoElemen
this.observerQueue.delete(observer);
}

bindVideoElement(newElement: HTMLVideoElement): void {
this.element = newElement;
if (!this.element) {
this.resizeObserver.unobserve(this.element);
bindVideoElement(newElement: HTMLVideoElement | null): void {
if (!newElement) {
if (this.element) {
this.resizeObserver.unobserve(this.element);
}
this.element = newElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

explicit this.element = null should be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this change, makes it a bit easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed ResizeObserver can observe multiple elements. So I updated the function to unobserve the element being observed if not null, and then observe newElement if not null. Also added a function to skip when newElement equals to the one being observed

return;
}
this.element = newElement;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: if newElement is null, is it supposed to be observed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If newElement it will be handled inside the if (!newElement) section above call unobserve if it was observing an element

this.resizeObserver.observe(this.element);
}
}
12 changes: 12 additions & 0 deletions test/videotile/DefaultVideoElementResolutionMonitor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import * as chai from 'chai';

import NoOpVideoElementFactory from '../../src/videoelementfactory/NoOpVideoElementFactory';
import DefaultVideoElementResolutionMonitor from '../../src/videotile/DefaultVideoElementResolutionMonitor';
import { VideoElementResolutionObserver } from '../../src/videotile/VideoElementResolutionMonitor';
import DOMMockBehavior from '../dommock/DOMMockBehavior';
Expand Down Expand Up @@ -70,5 +71,16 @@ describe('DefaultVideoElementResolutionMonitor', () => {

monitor.removeObserver(mockObserver);
});

it('should bind and unbind video elements', () => {
const videoElementFactory = new NoOpVideoElementFactory();
const videoElement = videoElementFactory.create();
monitor.bindVideoElement(videoElement);
monitor.bindVideoElement(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need some expect statements for this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function is expected to handle these inputs. Added expect no exception to throw

});

it('should skip unobserve if no element is being beserved', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo bserved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

monitor.bindVideoElement(null);
});
});
});
Loading