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.

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: time tooltip truncated #8527

Merged
merged 4 commits into from
Apr 10, 2024
Merged

fix: time tooltip truncated #8527

merged 4 commits into from
Apr 10, 2024

Conversation

harisha-swaminathan
Copy link
Contributor

@harisha-swaminathan harisha-swaminathan commented Dec 14, 2023

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.

truncated

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.

fixed

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

Copy link

codecov bot commented Dec 14, 2023

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
Copy link
Contributor

phloxic commented Dec 15, 2023

@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
Copy link
Contributor

amtins commented Dec 15, 2023

@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
Copy link
Contributor

phloxic commented Dec 15, 2023

@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
Copy link
Contributor

amtins commented Dec 15, 2023

@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
Copy link
Contributor

phloxic commented Dec 15, 2023

@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
Copy link
Contributor Author

harisha-swaminathan commented Jan 10, 2024

@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

Copy link
Contributor

@roman-bc-dev roman-bc-dev left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

phloxic commented Jan 18, 2024

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
Copy link
Contributor Author

harisha-swaminathan commented Feb 15, 2024

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
Copy link
Contributor

phloxic commented Feb 16, 2024

@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.

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants