fix: Loading spinner animation misalignment

[roman-bc-dev]

Description

Add top/left positioning rules to the .vjs-loading-spinner:before and .vjs-loading-spinner:after to fix the loading spinner animated segment misalignment.

This seems to occur because the border: 0.6em rule on the .vjs-loading-spinner parent effectively pushes content inward with the current box-model interpretation, against a set height/width, like padding. Unlike padding, however, the edge of the element box is interpreted as inside the border, not outside, so the spinner segment pseudo-elements inheriting its positioning rules are visually nudged downward and to the right.

Adding negative top/left positioning rules to account for the border fixes the misalignment. It's most likely why there was a negative margin in place for the segments in the past.

Specific Changes proposed

Add top/left positioning rules targeting .vjs-loading-spinner::before, .vjs-loading-spinner::after

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

All modified and coverable lines are covered by tests ✅

Comparison is base (395d608) 82.71% compared to head (4a671d7) 82.77%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8595      +/-   ##
==========================================
+ Coverage   82.71%   82.77%   +0.06%     
==========================================
  Files         113      113              
  Lines        7636     7636              
  Branches     1835     1835              
==========================================
+ Hits         6316     6321       +5     
+ Misses       1320     1315       -5     

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

[amtins]

@roman-bc-dev this should no longer be the case since this #8369. Is it possible to have an example to reproduce the issue?

I did a quick test of this PR and it seems to bring the issue back.

Screencast.from.16.02.24.19.16.13.webm

[amtins]

I was able to reproduce the issue by doing:

player.loadingSpinner.lockShowing()

If it's the case I would do something like #8435 instead.

[roman-bc-dev]

Hi @amtins. I'm able to reproduce this issue consistently in local videojs samples by updating visibility on the .vjs-loading-spinner element with:

  display: block;
  visibility: visible;

I might be misinterpreting how .vjs-lock-showing is applied generally but changing that display rule up top to flex instead of block did not alleviate the issue. I wouldn't expect it to because neither block nor flex rules modify the box model of an element and they wouldn't change how the border affects descendant elements.

Furthermore, because the pseudo-elements are affected by position: absolute rules, the correct fix here is to add top and left positioning. Not just to account for the border in this case but because not stipulating those rule can cause unexpected behavior.

[usmanonazim]

Looks good to me! Was able to reproduce the issue in several of the sandbox player samples, and the left: -0.6em; top: -0.6em; fixed it 😁

[amtins]

@roman-bc-dev basically, .vjs-lock-showing is used in menus. If it were to be used for .vjs-lock-showing for .vjs-loading-spinner, the rule would have to be adapted as follows:

.vjs-loading-spinner.vjs-lock-showing {
  display: flex !important;
  justify-content: center;
  align-items: center;
}

The problem with the proposed fix is that if you change the thickness of the .vjs-loading-spinner border, the alignment problem reappears:

player.loadingSpinner.el().style.borderWidth = '.2em'; 

Do you encounter this problem only with player.loadingSpinner.lockShowing() or when the .vjs-seeking class is added like player.addClass('vjs-seeking') ?

[amtins]

@usmanonazim could you share in which sandbox example you have the problem, I can't reproduce the problem unless I use player.loadingSpinner.lockShowing()? 😢

[roman-bc-dev]

@amtins This is a weird issue that seems to affect the spinner and its visibility outside of the .vjs-seeking state, specifically in conjunction to ad playback. It's visible when manually updating the style for that element (Which I guess contradicts my second point below! Ugh.).

1. We're not changing .vjs-lock-showing behavior with this update and are not interested in doing so.
2. Manually modifying an individual style rule out of context for a single element will negatively affect any part of the spinner animation solution. That is especially true for the border width because it must match the .vjs-loading-spinner element and its pseudo-element descendants. I don't believe it's reasonable to expect any single solution to cover all possible permutations and combinations of sizing in this instance. However, it absolutely should at least accommodate the existing 0.6em border width (which adding left/top rules does).

Editor's note here: The proposed change also doesn't interact with how .vjs-seeking affects the spinner because the following rules essentially cause the element to ignore descendent positioning (top/left):

display: flex;
justify-content: center;
align-items: center;

[amtins]

Thanks for the clarification 👍🏽. Since change #8369 the loading-spinner uses centering with flex positioning. This change makes it easier to customize the thickness of the component border without having to worry about adapting margins to stay aligned with the parent, which improves the developer experience.

There are several solutions, depending on the experience you wish to offer developers:

• Removing entirely the change fix(loading-spinner): border size customization #8369, which repositions the element in block and adds margins back to pseudo-elements. This avoids having a top and a left, which makes customization even more complex than before change fix(loading-spinner): border size customization #8369.
• Moving justify-content, align-items directly into the .vjs-loading-spinner class, so that if for any reason a developer wants to display the loading spinner, all the developer has to do is a display: flex.

  video.js/src/css/components/_loading.scss

  Lines 26 to 27 in 395d608

  	justify-content: center;
  	align-items: center;

  can be moved here

  video.js/src/css/components/_loading.scss

  Lines 1 to 21 in 395d608

  	.vjs-loading-spinner {
  	display: none;
  	position: absolute;
  	top: 50%;
  	left: 50%;
  	transform: translate(-50%, -50%);
  	opacity: 0.85;
  	// Need to fix centered page layouts
  	text-align: left;
  	border: .6em solid rgba($primary-background-color, $primary-background-transparency);
  	// border: 6px solid rgba(43, 51, 63, 0.5);
  	box-sizing: border-box;
  	background-clip: padding-box;
  	width: 5em;
  	height: 5em;
  	border-radius: 50%;
  	visibility: hidden;
  	}

To illustrate my point, here's an example:

• With the fix 8369

  .vjs-loading-spinner {
    border: 0.9em solid green;
    display: flex;
  }

• Before the fix 8369

  .vjs-loading-spinner {
    border: 0.9em solid green;
    display: block;
  }
  .vjs-loading-spinner::before,
  .vjs-loading-spinner::after {
    margin: -0.9em;
  }

• With this PR

  .vjs-loading-spinner {
    border: 0.9em solid green;
    display: block;
  }
  .vjs-loading-spinner::before,
  .vjs-loading-spinner::after {
    left: -0.9em;
    top: -0.9em;
  }

• Moving justify-content, align-items directly into the .vjs-loading-spinner

  .vjs-loading-spinner {
    border: 0.9em solid green;
    display: flex;
    visibility: visible;
  }

[roman-bc-dev]

Thank you @Essk for finding this. It looks like videojs-contrib-ads has its own class/rules to make the spinner appear, which explains why we're seeing this behavior in connection with an ad implementation.

I'll submit a change for that project. However, I think we may still want to move forward with the positioning rules here as a default state when display:flex is not used. My reasoning being:

1. It does not interfere with the flex/justify/align solution.
2. We've shown a precedent for using display: block for the spinner. Other projects which use it must catch up to implement flex as well when managing that component's visibility and we can't guarantee that those users are even aware of this change (as with contrib-ads, for instance). I think the positioning rules here are a nice fallback to make sure those folks are covered.
3. @amtins I absolutely prefer the flex solution way more than the one with negative margins we had in place before. I don't feel that stipulating top/left rules on elements that are already position:absolute is complicated or difficult to reason through. Maybe a comment would be useful here.

[roman-bc-dev]

Closing this since the associated fix seems to have been enough to address the issue on our end. The points I made above back in February don't seem relevant since we'd have seen feedback about the flex update since then.