fix(volume-panel): a long press on the volume button displays the volume slider on touch devices

[amtins]

Description

Prevents the volume slider from displaying when a long press is made on the volume button, forcing the user to make a long press or tap elsewhere on the player or page to make the volume slider disappear.

Screencast.from.16.07.23.13.24.51.webm

Refers to #8320 (comment)

Specific Changes proposed

• adds class .vjs-touch-enabled to pseudo-class :not at the root of the rule displaying the volume slider

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 #8361 (b41cfc2) into main (c66bf40) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #8361      +/-   ##
==========================================
+ Coverage   82.69%   82.71%   +0.01%     
==========================================
  Files         113      113              
  Lines        7578     7578              
  Branches     1821     1821              
==========================================
+ Hits         6267     6268       +1     
+ Misses       1311     1310       -1     

see 1 file with indirect coverage changes

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

[amtins]

Observation

Although the fix seems trivial, it does have an impact on touch-screen laptops such as Chromebooks, Microsoft Surface and ThinkPad Yoga (yet to be confirmed). On these devices, hovering over the volume button won't display the volume slider.

The main reason for this is how a touch device is detected, see:

video.js/src/js/utils/browser.js

Lines 150 to 153 in c66bf40

	export const TOUCH_ENABLED = Boolean(Dom.isReal() && (
	'ontouchstart' in window ||
	window.navigator.maxTouchPoints ||
	window.DocumentTouch && window.document instanceof window.DocumentTouch));

One approach would be to use the matchMedia API ^{1. 2.}, which could potentially target only mobile devices such as smartphones and tablets:

export const TOUCH_ENABLED = Boolean(Dom.isReal()) && 
  window.matchMedia('(pointer: coarse)').matches &&
  window.matchMedia('(any-pointer: coarse)').matches;

If the above code more accurately reflects the original intention, a separate PR could be made. This would allow the fix proposed in this PR to work in all the cases tested below.

Test

If a test does not explicitly indicate Browserstack, this means that it was run on a real device. If the test indicates more than one browser per device, this means that different results were found.

Galaxy note (Browserstack)

• pointer: coarse
• any-pointer: coarse
• any-pointer: fine
• hover: hover
• any-hover: hover

Lenovo Tab P11 Plus

• Chrome

  • pointer: coarse
  • any-pointer: coarse
  • any-pointer: fine
  • hover: hover
  • any-hover: hover

• Firefox

  • pointer: coarse
  • any-pointer: coarse
  • any-pointer: fine
  • hover: none
  • any-hover: hover

iPad (Browserstack), iPhone, "regular" Android Smartphones

• pointer: coarse
• any-pointer: coarse
• hover: none
• any-hover: none

Chromebook with touch screen

• Firefox

  • pointer: fine
  • any-pointer: fine
  • hover: hover
  • any-hover: hover

• Chrome

  • pointer: fine
  • any-pointer: coarse
  • any-pointer: fine
  • hover: hover
  • any-hover: hover

[mister-ben]

That version of TOUCH_ENABLED would be false on Chrome on Pixel 7. It reports only having a fine pointer.

[misteroneill]

I agree that the TOUCH_ENABLED field leaves a lot to be desired.

90% of the time, people treat it as a proxy for phone/tablet touch device, but, as you point out, it includes hybrid devices like the Surface. Further, I think a lot of users are probably relying on the current behavior of TOUCH_ENABLED, so we should probably not change it out from under them.

What might be better is to have a different value/class name for mobile/tablet devices where they are detected (either via matchMedia or user agent or whatever). More granular class names allows us to mix and match feature detection to target exactly what we need. And adding new class(es) is a non-breaking change.

It seems to me what we really care about is how the user is interacting with a control. This could be through touch input, mouse input, or keyboard input. The complexity comes in because touch and mouse input intersect in how they get handled in the UI.

A backward compatible way might be to do device type detection (using matchMedia or user agent hints or whatever) to add classes like vjs-device-mobile, vjs-device-tablet, vjs-device-desktop, vjs-device-smart-tv and others.

Kind of thinking out loud here.

[amtins]

Thank you for taking the time to read and respond to my comment.

@mister-ben, I had indeed missed this case, which shows that my approach was not the right one.

@misteroneill, I really like your proposal for the various reasons you mentioned and for the improvement it would bring to developers.

My only concern is that one demonstrated by mister-ben, where we see that some devices don't necessarily return consistent information. If we add on top of that the differences that can exist on the same device with different browsers, exotic UAs that don't make it easy to clearly distinguish the type of device, and future limitations on UAs.

This seems to me to be a substantial piece of work, potentially difficult to maintain and perhaps requiring the creation of a new repo so as not to clutter up the original code base too much.

In any case, thank you once again for your comments, which have given me the answer to the question I was asking myself: should I close this PR?

The answer is: yes

So I'm closing this PR, which can be reopened if needed.