fix: time tooltip truncated

[harisha-swaminathan]

Description

spaceRightOfPoint is always NaN for mouse time display (unlike playProgressBar's time display) because the seekbarRect (dom.findPosition(seekbar)) does not have a right property.

And since spaceRightOfPosition is always NaN, when the mouse tool tip is moved close to edge of the right side player, it's position is not adjusted as it is on the left and so it get's cut off.

Specific Changes proposed

The proposed change is to ignore the gap between the right edge of the SeekBar and the player (playerRect.right - seekBarRect.right) in such cases.

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
• Reviewed by Two Core Contributors

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6a5e1ee) 83.45% compared to head (ce8d0ae) 83.88%.
Report is 20 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8527      +/-   ##
==========================================
+ Coverage   83.45%   83.88%   +0.42%     
==========================================
  Files         113      113              
  Lines        7596     8834    +1238     
  Branches     1827     2211     +384     
==========================================
+ Hits         6339     7410    +1071     
- Misses       1257     1424     +167     

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

[phloxic]

@harisha-swaminathan - I believe the solution at #7308 is better, as I tried to explain here: #8273. The reasons why that change was reverted imo do not apply any longer.

[amtins]

@harisha-swaminathan, @phloxic I'm not quite sure I understand the problem as a whole.

What I do understand (but maybe I'm wrong) is that we want to prevent the timeTooltip from overflowing. If that's the case, perhaps we need to change the approach to calculating the positioning of the timeTooltip component.

To do this, the only parameters we need are:

• seek bar width
• timeTooltip width
• The position of the timeTooltip in the seek bar

With these 3 parameters, we should be able to keep the timeTooltip inside the seek bar.

In this case, we could do something like #8530

[phloxic]

@amtins - I just rebased what I am using onto main here.
Basically, findPosition does the correct thing for the left side, but not for the right side. getBoundingClientRect provides data for left and right and works for me[tm].

Demo v8.3.0 without the change here
Demo v8.3.0 with the change here
afaics the demo with the fix (2nd example) also shows that issues leading to using findPosition do not exist anymore.

[amtins]

@phloxic thank you for these clarifications. I'm currently going through all the issues (and there are a lot) to try and understand the initial problem.

[phloxic]

@phloxic thank you for these clarifications. I'm currently going through all the issues (and there are a lot) to try and understand the initial problem.

Certainly a good idea, see #7308 (comment). I was too focused on the out of bounds on the right problem (which does still exist though), sorry.

[harisha-swaminathan]

@amtins @phloxic
I agree that a solution like #7308 is a more ideal permanent solution but since it reverts the change from #5773 (which fixes #6726 and #1102) I think this PR could work as a good solution for now as it'a a smaller change and causes no regression.
The mouse tooltip position still aligns as it should (even when transform: scale is applied to the video container).

I set up demo players to show

• that there is no regression in the player behavior when transform property is applied. (Specifically that the fix for 6726 and 1102 is not reverted) - https://codepen.io/hswaminathan/pen/xxBOVLZ
• how the fix works when the progress bar is extended to the full width of the player - with fix vs without fix

[roman-bc-dev]

I think this is a simple fix that addresses the issue, helps define expected UI behavior, and doesn't prevent a deeper refactor in the future (if that's something we'd like to invest effort into).

[phloxic]

Both in https://codepen.io/hswaminathan/pen/xxBOVLZ and https://codepen.io/hswaminathan/pen/VwRjmBW you can see the tooltip staying within the left boundary of the control bar or the player(which imo is the correct behavior), but stuck on the right by the boundary of the progress bar.

It might still worth looking into #8530

Also, when the cursor is dragged the current time tooltip does weird jumps on both ends. In my tests this could be remedied by setting enableSmoothSeeking.

[harisha-swaminathan]

Both in https://codepen.io/hswaminathan/pen/xxBOVLZ and https://codepen.io/hswaminathan/pen/VwRjmBW you can see the tooltip staying within the left boundary of the control bar or the player(which imo is the correct behavior), but stuck on the right by the boundary of the progress bar.

It might still worth looking into #8530

Also, when the cursor is dragged the current time tooltip does weird jumps on both ends. In my tests this could be remedied by setting enableSmoothSeeking.

Both in https://codepen.io/hswaminathan/pen/xxBOVLZ and https://codepen.io/hswaminathan/pen/VwRjmBW you can see the tooltip staying within the left boundary of the control bar or the player(which imo is the correct behavior), but stuck on the right by the boundary of the progress bar.

It might still worth looking into #8530

Also, when the cursor is dragged the current time tooltip does weird jumps on both ends. In my tests this could be remedied by setting enableSmoothSeeking.

@phloxic I've updated the PR to set the tooltip boundary within the progress bar on both sides for the sake of consistency. Looks like #8530 too limits the tooltip within the progress bar instead of the control bar.

Also, I observed that in https://phloxic.productions/test/videojs/ttt/v8-dev.html, the tooltip overflows in the left side.

[phloxic]

@phloxic I've updated the PR to set the tooltip boundary within the progress bar on both sides for the sake of consistency.

Yes, that's consistent. But isn't it kind of ironic (or inconsistent) from a viewer's perspective to reduce the room for tooltip display specifically for the use cases of tooltips which are enlarged, e.g., thumbnail previews etc. or, indeed, scaling the player?

It's technically correct, but in my personal opinion a step backwards from the audience's point of view. I don't think there are a lot of complaints about the tooltip sticking out of bounds of the progress bar. I'm reasonably certain that I would notice the general behaviour change (and complain). But then I might be the only one ;-)

Having said that I'm obviously fine with conceding that @roman-bc-dev's argument may be the realistic way forward for the project.

Looks like #8530 too limits the tooltip within the progress bar instead of the control bar.

Yes, in the second example.

Also, I observed that in https://phloxic.productions/test/videojs/ttt/v8-dev.html, the tooltip overflows in the left side.

Yes, scaling the player is still an issue.

[misteroneill]

I agree with @roman-bc-dev that this is an overall improvement even with existing limitations. Great discussion and thanks for the feedback @phloxic!