Fix/time tooltip overflow

[amtins]

Description

Specific Changes proposed

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

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (d2dc5b9) 82.65% compared to head (830d8dc) 82.64%.

❗ Current head 830d8dc differs from pull request most recent head 72b0c27. Consider uploading reports for the commit 72b0c27 to get more accurate results

Files	Patch %	Lines
...rc/js/control-bar/progress-control/time-tooltip.js	72.22%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8530      +/-   ##
==========================================
- Coverage   82.65%   82.64%   -0.02%     
==========================================
  Files         113      113              
  Lines        7605     7605              
  Branches     1828     1830       +2     
==========================================
- Hits         6286     6285       -1     
- Misses       1319     1320       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[phloxic]

@amtins, just a quick feedback re out of bounds: at the moment the tooltips do not go out of bounds of the container, but they also do not go out of bounds of the seekbar. I think the boundary should still be the container.
When I seek with the slider the current time tooltip intermittently moves in the opposite direction or jumps.

[amtins]

TL;DR: It's a complex case with a multitude of edge cases

@phloxic thanks for your feedback, that's exactly what I'm trying to determine/understand.

If I summarize, I see 3 cases:

1. The experience currently proposed by video.js is that we have a timeTooltip that is centered with respect to

1. mouseTimeDisplay
2. playProgressBar

If we want to keep this experience regardless of the various problems reported by the different issues. I think we can easily simplify the code. This is exactly what commits 37f6acb and b28bf97 do, where css is preferred to javascript to center the timeTooltip. Technically it's debatable as this type of positioning prevents the application of a transform scale. However, I don't see this as a regression, as video.js offers a powerful mechanism for changing the size of a component based on its font-size.

However, I'd like to reiterate that there may be use cases that I've missed.

2. The timeTooltip is positioned within the limits of the progressControl's size and therefore does not overflow outside the component.

This is the experience offered by this PR. Personally, I don't really have an opinion/preference as to whether it's a good or bad experience. What I do notice is that it would greatly simplify the code base. However, as you rightly point out, there is a edge case due to the fact that the playProgressBar doesn't update in real time. This means that if, for example, we're in the middle of a media and we move to one end or the other, the playProgressBar's timeTooltip will tend to move to the left or right. Which is not acceptable/desirable. This case wouldn't be a problem if #8287 were the default behavior of video.js.

3. The timeTooltip is positioned within the limit of the controlBar

I have the impression that this is the experience you're proposing, don't hesitate to correct me if I'm wrong. With this approach, the timeTooltip could freely overflow the progressControl while remaining within the limits of the controlBar and therefore of the player.

If I explore this path, I see a potential problem:

If the calculation reference point becomes the controlBar, then we're introducing a strong dependency of the progressControl on its parent. This may not be desirable in the case where a developer wants to create its own control bar from scratch.

A potential solution would therefore be to take only the player size as a reference and not the controlBar. This would potentially solve the problem, as each component has a functional dependency on the player anyway. However, I'm pretty sure I'm missing some use cases. Not to mention that I think I'll be running into the same problem you mentioned.

When I seek with the slider the current time tooltip intermittently moves in the opposite direction or jumps.

In conclusion, now I'm beginning to understand the complexity of the subject. I was a little too naive in my approach. 😅

[amtins]

I tried to explore option 3 without success.

However, I created an example page with option 2 without the bug described below:

When I seek with the slider the current time tooltip intermittently moves in the opposite direction or jumps.

Test page: https://amtins.github.io/bug-free-noodles/

Unfortunately I'm afraid that's the best solution I can come up with.

[phloxic]

1. The experience currently proposed by video.js is that we have a timeTooltip that is centered with respect to

1. `mouseTimeDisplay`

2. `playProgressBar

I'm not 100% sure I understand. If it comes to centering, in my view the tooltips are centered over the relevant position (current time or mouse respectively) in the timeline (be that playProgressBar or seekBar). I believe this makes sense from the user perspective, and it should be the case as much as possible.

The problem is: where are the limits for what one considers as "possible".

If we want to keep this experience regardless of the various problems reported by the different issues. I think we can easily simplify the code. This is exactly what commits 37f6acb and b28bf97 do, where css is preferred to javascript to center the timeTooltip. Technically it's debatable as this type of positioning prevents the application of a transform scale. However, I don't see this as a regression, as video.js offers a powerful mechanism for changing the size of a component based on its font-size.

At least in the stage I tested, the limits of what is possible have shrunk compared to the current release. On both sides the tooltips are 'de-centered' when the play-head or the cursor/mouse-time are nearer to the end of the playProgress than half their width.

However, I'd like to reiterate that there may be use cases that I've missed.

2. The timeTooltip is positioned within the limits of the progressControl's size and therefore does not overflow outside the component.

This is the experience offered by this PR. Personally, I don't really have an opinion/preference as to whether it's a good or bad experience.

I do have an opinion ;-) As above my opinion is that the cases where the toolips are 'de-centered' should be reduced as much as possible. Their connection to their reference point is 'stretched' and the viewer has a moment of: Oh, what's happening, ah, ok, there is some kind of limit, tooltip banged at door frame (hehe) ...

What I do notice is that it would greatly simplify the code base. However, as you rightly point out, there is a edge case due to the fact that the playProgressBar doesn't update in real time. This means that if, for example, we're in the middle of a media and we move to one end or the other, the playProgressBar's timeTooltip will tend to move to the left or right. Which is not acceptable/desirable. This case wouldn't be a problem if #8287 were the default behavior of video.js.

Yes, I thought it was related to the lack of smooth seeking (I 'backported' smooth seeking immediately to v7 in my deployments).

3. The timeTooltip is positioned within the limit of the controlBar

I have the impression that this is the experience you're proposing, don't hesitate to correct me if I'm wrong.

Indeed.

It is also the current behaviour, or was, until it was changed (or rather disabled) for the right side because of eac77b4 – which is why I wanted to revert it.

With this approach, the timeTooltip could freely overflow the progressControl while remaining within the limits of the controlBar and therefore of the player.

Can one make the controlBar shorter than the player? If yes, then I am for player. – I have admittedly no use case for an extremely short controlbar (unless there is no timeline anyway, and therefore no tooltips).

If I explore this path, I see a potential problem:

If the calculation reference point becomes the controlBar, then we're introducing a strong dependency of the progressControl on its parent. This may not be desirable in the case where a developer wants to create its own control bar from scratch.

I don't understand exactly, sorry.

A potential solution would therefore be to take only the player size as a reference and not the controlBar.

Regardless of coding problems, this would make the most sense to me.

This would potentially solve the problem, as each component has a functional dependency on the player anyway. However, I'm pretty sure I'm missing some use cases.

Massive tooltip speech bubbles?! ;-) But yes, all these elements are dependent of the player, especially the tooltips. In principle they should stay in there (my door frame analogy). – That being said, when I experimented with applying scale to the player the boundary was gone on both sides .

Not to mention that I think I'll be running into the same problem you mentioned.

When I seek with the slider the current time tooltip intermittently moves in the opposite direction or jumps.

Nevermind, only teething problems.

In conclusion, now I'm beginning to understand the complexity of the subject. I was a little too naive in my approach. 😅

Welcome to the club. 😅

[phloxic]

Test page: https://amtins.github.io/bug-free-noodles/

Thanks for that. I like the CSS approach.

Unfortunately I'm afraid that's the best solution I can come up with.

It's certainly more consistent than the current state. And I obviously have no problem with examples 3 and 4 because the progress bar spans the entire width of the player.

[amtins]

@phloxic I think I have a solution that might meet your preference and also solve the problem when a transform scale is applied to the player. Please see https://amtins.github.io/bug-free-noodles/scale.html

[phloxic]

@amtins - almost there. Great.

From the viewer's perspective it makes sense structurally and aesthetically to use the player boundaries as tooltip boundaries. I don't think the intended behaviour should be changed.

My test page.

The non-scaled version seems to be there.
In the scaled version the tooltips are still stuck within seekbar limit.

And of course it requires smooth-seeking. Which is fine by me, but may be an issue for others.

Test of current behaviour for comparison.