fix(picture-in-picture-control): hide the component in non-compatible browsers

[amtins]

Description

This PR fixes the display of the PiP button in disabled state in browsers that do not support this feature e.g. Firefox. This behavior occurs mainly when the order of the components is changed:

var player = videojs('player', {
  controlBar: {
      children: [
        'pictureInPictureToggle', // In Firefox it displays the button 
        'playToggle',
        'volumePanel',
        'currentTimeDisplay',
        'timeDivider',
        'durationDisplay',
        'progressControl',
        'liveDisplay',
        'seekToLive',
        'remainingTimeDisplay',
        'customControlSpacer',
        'playbackRateMenuButton',
        'chaptersButton',
        'descriptionsButton',
        'subsCapsButton',
        'audioTrackButton',
        'fullscreenToggle'
    ]
  }
});

Fixes #7898 and also adresses #5824 (comment)

Specific Changes proposed

• control-bar.js removes the logic that does not allow to change properly the order of the pictureInPictureToggle component.
• picture-in-picture-toggle.js
  • the component is hidden by default and will be displayed when the media is loaded if the browser supports the PiP API
  • extracts the listener in a dedicated method (handlePictureInPictureAudioModeChange) and reduces the indentation level
  • the show method does not display the component if the browser does not support the PiP api
• add a test case

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Change has been verified in an actual browser (Chrome, Firefox, IE)
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[codecov]

Codecov Report

Merging #7899 (e135cc7) into main (f1558c6) will decrease coverage by 0.01%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main    #7899      +/-   ##
==========================================
- Coverage   82.36%   82.35%   -0.01%     
==========================================
  Files         112      112              
  Lines        7483     7488       +5     
  Branches     1804     1805       +1     
==========================================
+ Hits         6163     6167       +4     
- Misses       1320     1321       +1     

Impacted Files	Coverage Δ
src/js/control-bar/control-bar.js	100.00% <ø> (+16.66%)	⬆️
src/js/control-bar/picture-in-picture-toggle.js	84.21% <86.66%> (+0.33%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mister-ben]

We should probably update isAudio_ when this matches as the source is set, then use isAudio() here.

[amtins]

@mister-ben it make sens. Your suggestion is to set the audio flag somewhere in

video.js/src/js/player.js

Lines 1582 to 1629 in f1558c6

	handleTechSourceset_(event) {
	// only update the source cache when the source
	// was not updated using the player api
	if (!this.changingSrc_) {
	let updateSourceCaches = (src) => this.updateSourceCaches_(src);
	const playerSrc = this.currentSource().src;
	const eventSrc = event.src;
	// if we have a playerSrc that is not a blob, and a tech src that is a blob
	if (playerSrc && !(/^blob:/).test(playerSrc) && (/^blob:/).test(eventSrc)) {
	// if both the tech source and the player source were updated we assume
	// something like @videojs/http-streaming did the sourceset and skip updating the source cache.
	if (!this.lastSource_ || (this.lastSource_.tech !== eventSrc && this.lastSource_.player !== playerSrc)) {
	updateSourceCaches = () => {};
	}
	}
	// update the source to the initial source right away
	// in some cases this will be empty string
	updateSourceCaches(eventSrc);
	// if the `sourceset` `src` was an empty string
	// wait for a `loadstart` to update the cache to `currentSrc`.
	// If a sourceset happens before a `loadstart`, we reset the state
	if (!event.src) {
	this.tech_.any(['sourceset', 'loadstart'], (e) => {
	// if a sourceset happens before a `loadstart` there
	// is nothing to do as this `handleTechSourceset_`
	// will be called again and this will be handled there.
	if (e.type === 'sourceset') {
	return;
	}
	const techSrc = this.techGet('currentSrc');
	this.lastSource_.tech = techSrc;
	this.updateSourceCaches_(techSrc);
	});
	}
	}
	this.lastSource_ = {player: this.currentSource().src, tech: event.src};
	this.trigger({
	src: event.src,
	type: 'sourceset'
	});
	}

Do you recommend doing it in another PR? If yes with what kind of scope? feat, refactor or fix?

I would tend to create another PR to keep modification more atomic. This would allow to try to better handle the HLS and Dash audio.

[mister-ben]

Yes, should be a separate PR. Digging a bit more I think isAudio() and its purpose need a bit of a rethink and discussion, so this PR shouldn't wait for that.

[mister-ben]

Codecov is being weird again, it's flagging a coverage change in an unrelated file that doesn't make any sense.

[misteroneill]

Nice, thank you!