hide/show methods should toggle visibility:hidden instead of display:none

[albell]

Width/height getters, as written, will work on visibility:hidden, but not on display:none elements. Because display: none elements are taken out of the render tree, their computed style dimensions always return zero.

This change would solve this "known issue":

https://github.com/videojs/video.js/blob/055d81dc3abc74c7a4dc88801c95cee67d2ead04/src/js/component.js#L795

Some history on this: jQuery introduced a hack many versions back for getting dims on display: none elements. It temporarily sets position to be absolute, quickly measures the dimension, then sets display back to what it was before. This seemed really clever at the time, and allowed a whole bunch of people to never bother to do it right. It also introduced a bunch of unexpected behaviors, and doesn't work right with nested elements. Methvin has since recanted, saying the hack was a mistake in retrospect, but can't be taken out, because it's so heavily used. The upshot of all this is that if you know you might need to measure something later, best practice is to hide it with visibility:hidden instead, which returns normal dimension values, as expected, and doesn't intercept clicks. I will write up the PR if there's agreement on this.

Talking about:

vjs.Component.prototype.show = function(){
  this.el_.style.display = 'block';
  return this;
};

/**
 * Hide the component element if currently showing
 *
 * @return {vjs.Component}
 */
vjs.Component.prototype.hide = function(){
  this.el_.style.display = 'none';
  return this;
};

The only wrinkle here is that visibility doesn't inherit like display. A visible element inside a hidden element will still be visible. So we have to toggle between hidden and inherit using hide() and show(), respectively, and avoid visible because we never want to override the parent's visibility.

[dmlap]

I think a lot of the code that is currently using show()/hide() actually wants the component to be removed from the render tree. Removing the element's visibility and removing it from the layout are two distinct operations and they're both useful in many situations. Given jQuery uses show()/hide() to mean "removal from the render tree", I think it would confuse many developers if video.js had different semantics.

[albell]

I think a lot of the code that is currently using show()/hide() actually wants the component to be removed from the render tree.

Really? Authors don't just want to visually hide things? Agreed that these are distinct operations. Can you post a gist/fiddle of specific code that this fix would break?

Given jQuery uses show()/hide() to mean "removal from the render tree", I think it would confuse many developers if video.js had different semantics.

No, the outward behavior is substantially the same. What's arguably more confusing to authors is to measure a width and get zero when the author knows it has a width. Again, jQuery is deeply broken in this area. For more background on this, check out:

http://bugs.jquery.com/ticket/14685 http://bugs.jquery.com/ticket/14849

So jQuery should not be the model. vjs is not a jQuery plugin. jQuery is hemmed in by backwards compatibility in ways that vjs is not. vjs reimplements lots of Resig-style utility code at considerable code weight in order to not be dependent on jQuery, and we should be free to take full advantage of that independence. Lastly, jQuery cannot afford to make any assumptions about available stylesheets, and vjs absolutely depends on its stylesheet to function. So it's a totally different ecosystem of assumptions. I only brought up jQuery as an example of how not to do this. jQuery's implementation of show()/hide() is 2007-era cruft. We should learn from their acknowledged mistakes.

In retrospect, on a very high level, having the CSS inheritance model for visibility be different for that of display is questionable. The inheritance 'gotcha' is probably one of the reasons visibility was for a long time generally underused. That's changing now. I personally often prefer to reset visibility globally to behave more like display with:

* {visibility: inherit}

An absolute positioned visibility:hidden element will not affect layout flow, so I fail to so how this would break anything. There would be a very minor rendering performance hit (though not in the realm of paint) but that's a hit we would want to incur, in order to be able to query dimensions of the hidden elements later. That performance micro-hit would probably be well compensated for by moving to utility classes for show()/hide(), which cache better in the renderer than inline CSS. At the very least we should be toggling utility classes here, not touching inline CSS (which should be a last resort) e.g. add/remove:

.vjs-visually-hidden {visibility:hidden}

[heff]

This is an interesting idea, and there clearly would be some benefits. I'm not sure if I think they outweigh the negatives yet.

On example of where the difference comes into play is UI elements that are meant to flow into space. For example the buttons on the control bar (rate, captions, subtitles) that are floated right. If we used visibility instead of display there would be gaps between buttons where other buttons exist but are hidden. We could specifically remove them from the dom when they're not in use, but if we wanted to maintain their specific order while adding/removing them it would be a much more complicated solution.

I wonder if also adding position: absolute would hurt anything. Do we know if this affects mouse interactions in any way, or any other potential issues that could come of this?

we should be free to take full advantage of that independence

To have the option at all is definitely a benefit of video.js. At the same time it's not about trying to match jQuery specifically, it's about designing APIs to match user expectations. Since jQuery, Prototype, MooTools, and ExtJS all hide using display: none, there's a strong expectation attached to that. Creating a function that's named the same but works differently makes me cringe a little. However if we can use visibility + position:absolute without any side effects, that sounds like the best of both worlds.

[albell]

Proof of concept (and then some) here:

https://github.com/baloneysandwiches/video.js/compare/hide-using-visibility

Not yet PR ready, but FWIW it seems to work.

One example of where the difference comes into play is UI elements that are meant to flow into space. For example the buttons on the control bar (rate, captions, subtitles) that are floated right. If we used visibility instead of display there would be gaps between buttons where other buttons exist but are hidden.

(visibility: hidden and position:absolute, taken together) and display:none both take the element and all of its descendants out of document flow. And they both cause the element to be unpainted. So in those two respects they are identical. I see the family of concerns you're wondering about, but I suspect we can handle it. Screenshot?

We could specifically remove them from the dom when they're not in use, but if we wanted to maintain their specific order while adding/removing them it would be a much more complicated solution.

visibility: hidden and display:none both leave all elements in the DOM. No difference there. Only script can actually take something out of the DOM, right?

I wonder if also adding position: absolute would hurt anything. Do we know if this affects mouse interactions in any way, or any other potential issues that could come of this?

Good question. visibility: hidden elements don't receive click events. Or right-click events. None of that. I glanced quickly for the spec on this and can't find it but it's settled business. It's actually kind of nice to be able to mouse on hidden elements in the dev tools Elements Inspector and see where they would be painted if they were visible, because the boxes are getting drawn. That might be an additional tiny win in terms of developer aesthetics.

Since jQuery, Prototype, MooTools, and ExtJS all hide using display: none, there's a strong expectation attached to that. Creating a function that's named the same but works differently makes me cringe a little.

I hear you. At the same time, the big new frameworks all have a "hidden" class utility. vjs itself has one already, too, I'm just generalizing it. display:none inheritance is definitely more intuitive. You can definitely see why all the 2007-era utilities used it. It just makes measurement impossible!

One idea for a possible transition to this approach would be to add new methods to 5.0, with new names "paint()/unpaint()"? and then preserve hide()/show() function declarations as-is, but mark hide()/show() as deprecated in the docs, and stop using them internally.

[heff]

Related issue here: #1681. After that change we could try overriding the player's hide/show methods to use visibility + position, and see how that works.

[mmcc]

When you say

try overriding the player's hide/show methods

do you mean changing the CSS to use visibility + position?

[heff]

Not on the default vjs-hidden class that's used by all components, but I didn't really think through how we'd accomplish that specifically for the player.

[albell]

Not on the default vjs-hidden class that's used by all components...

Why not? Why can't it be generalized?

I totally get that this isn't highest priority, but I did 90% of the exploratory/explanatory work on this in August. The proof of concept (and then some) is still sitting there in my branch. I've tried to address each of your concerns specifically in the thread. The inherit trick basically solves them AFAICT. I'm honestly happy to pick this up again but I'd like to know that it's at least getting glanced at. :sweat_smile:

[heff]

Thanks Alex, we haven't forgotten the rest of the conversation and the work you've done there. We're just taking this in small steps to understand the impact. First by moving hide handling to classes over inline, then by changing what css properties are used. I want to start by testing on the player because that's where we'll have the most impact and potential benefit. If that works well we can move it to all components.

[kud]

I'm currently interested about performance on show/hidden toggle in general (not only videojs), and I arrived on this thread.

To be fair, I have better performance with height:0; overflow: hidden; than display: none when I've got to "hide" lots of text/nodes. (I only speak about toggling elements like a tab component).

If you want to understand what it costs, go there: https://csstriggers.com

Understand that it's ordered by high performance breaker:

Layout > Paint > Composite.