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

Investigate possible performance improvements of show/hide methods #2057

Closed
markelog opened this Issue Feb 3, 2015 · 10 comments

Comments

Projects
None yet
9 participants
@markelog
Member

markelog commented Feb 3, 2015

@markelog markelog added the Effects label Feb 3, 2015

@markelog markelog changed the title from Invistigate possible performance improvements of show/hide methods to Investigate possible performance improvements of show/hide methods Feb 3, 2015

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 4, 2015

Member

We could improve the case from jquery/jquery.com#88 by simplifying – or avoiding – the check for visibility. It seems that webkit was taking a while getting that value. I'm not sure how that would work yet, though.

Member

timmywil commented Feb 4, 2015

We could improve the case from jquery/jquery.com#88 by simplifying – or avoiding – the check for visibility. It seems that webkit was taking a while getting that value. I'm not sure how that would work yet, though.

@ivantodorovich

This comment has been minimized.

Show comment
Hide comment
@ivantodorovich

ivantodorovich Feb 8, 2015

The check for visibility could be optional by passing an argument and disabled by default.
It wouldn't sacrifice performance until it is really needed.

ivantodorovich commented Feb 8, 2015

The check for visibility could be optional by passing an argument and disabled by default.
It wouldn't sacrifice performance until it is really needed.

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen Feb 9, 2015

According to this, if jQuery always adds [hidden] { display: none; } to the page just before page-load, then it can use much simpler code:

hide: function() {
  this.attr('hidden', true);
},
show: function() {
  this.attr('hidden', false);
}

That will support IE 7+ (browsers that obey the [hidden] selector).

I don't know the performance characteristics of .attr or [hidden], though. Class selectors are probably faster than attribute selectors.

Another benefit of either class-based or attribute-based toggling is that it can be animated in CSS. That means a jQuery plugin can do .show() and I can decide show that show happens without changing the plugin's code.

jamesarosen commented Feb 9, 2015

According to this, if jQuery always adds [hidden] { display: none; } to the page just before page-load, then it can use much simpler code:

hide: function() {
  this.attr('hidden', true);
},
show: function() {
  this.attr('hidden', false);
}

That will support IE 7+ (browsers that obey the [hidden] selector).

I don't know the performance characteristics of .attr or [hidden], though. Class selectors are probably faster than attribute selectors.

Another benefit of either class-based or attribute-based toggling is that it can be animated in CSS. That means a jQuery plugin can do .show() and I can decide show that show happens without changing the plugin's code.

@zzzzBov

This comment has been minimized.

Show comment
Hide comment
@zzzzBov

zzzzBov Feb 13, 2015

@jamesarosen when showing/hidding elements, I used to think that the [hidden] attribute was a good choice, however the HTML5 spec includes a description that makes it inappropriate in a number of cases:

The hidden attribute must not be used to hide content that could legitimately be shown in another presentation. For example, it is incorrect to use hidden to hide panels in a tabbed dialog, because the tabbed interface is merely a kind of overflow presentation — one could equally well just show all the form controls in one big page with a scrollbar. It is similarly incorrect to use this attribute to hide content just from one presentation — if something is marked hidden, it is hidden from all presentations, including, for instance, printers.

Because of this, I encourage the use of [aria-hidden], as it ties directly into the actual state of the element and also means that the element will be hidden in an accessible manner.

zzzzBov commented Feb 13, 2015

@jamesarosen when showing/hidding elements, I used to think that the [hidden] attribute was a good choice, however the HTML5 spec includes a description that makes it inappropriate in a number of cases:

The hidden attribute must not be used to hide content that could legitimately be shown in another presentation. For example, it is incorrect to use hidden to hide panels in a tabbed dialog, because the tabbed interface is merely a kind of overflow presentation — one could equally well just show all the form controls in one big page with a scrollbar. It is similarly incorrect to use this attribute to hide content just from one presentation — if something is marked hidden, it is hidden from all presentations, including, for instance, printers.

Because of this, I encourage the use of [aria-hidden], as it ties directly into the actual state of the element and also means that the element will be hidden in an accessible manner.

@markelog markelog added this to the 3.0.0 milestone Feb 15, 2015

@markelog markelog self-assigned this Feb 15, 2015

@timmywil timmywil added the Blocker label Feb 16, 2015

@niutech

This comment has been minimized.

Show comment
Hide comment
@niutech

niutech Apr 10, 2015

It may be a silly question, but why not just keep it simple?

hide: function() {
  this.style.display = 'none';
},
show: function() {
  this.style.display = ''; //or block or initial
}

niutech commented Apr 10, 2015

It may be a silly question, but why not just keep it simple?

hide: function() {
  this.style.display = 'none';
},
show: function() {
  this.style.display = ''; //or block or initial
}
@zzzzBov

This comment has been minimized.

Show comment
Hide comment
@zzzzBov

zzzzBov Apr 10, 2015

@niutech overwriting the display style will kill the cascade

<style>
.foo {
    display: inline-block;
}
</style>
<div class="foo">lorem ipsum</div>

and worse yet, if there are pre-existing inline styles, you're blowing those away as well:

<div style="display: inline">lorem ipsum</div>

If you use a different attribute for managing the visibility of an element, the state stays independent of the styles.

zzzzBov commented Apr 10, 2015

@niutech overwriting the display style will kill the cascade

<style>
.foo {
    display: inline-block;
}
</style>
<div class="foo">lorem ipsum</div>

and worse yet, if there are pre-existing inline styles, you're blowing those away as well:

<div style="display: inline">lorem ipsum</div>

If you use a different attribute for managing the visibility of an element, the state stays independent of the styles.

@Nettsentrisk

This comment has been minimized.

Show comment
Hide comment
@Nettsentrisk

Nettsentrisk Jan 4, 2016

Because of this, I encourage the use of [aria-hidden], as it ties directly into the actual state of the element and also means that the element will be hidden in an accessible manner.

[hidden] => [aria-hidden=true]

The hidden attribute should be used if the intention of show/hide is to in fact hide it completely, from view and thus from the accessibility API. If the intention is to keep this in line with how show/hide have always been used, then this is the correct choice, as hide() => display:none which is essentially the same as setting a hidden attribute, which then means the accessibility API takes that as aria-hidden=true.

In regards to the problem of responsive, and perhaps you don't want to hide it in certain contexts when you use show/hide - well then I'd say you've designed your application poorly. If what you're attempting to hide should only be hidden in certain scenarios, then you should be toggling a class name or something similar to control whether it's visible or not through the CSS.

So, it makes sense for show/hide to remove/set the hidden attribute, respectively, because:

  1. It's the modern HTML5 equivalent of what show/hide/toggle have been doing all along
  2. It will force correct usage in scenarios where it's being used (wrong) in combination with responsive CSS

jQuery should not be creating any inline CSS for this; all modern browsers already support the hidden attribute natively. Any front end web developer with respect for themselves are already using a reset/normalize CSS setup that includes the hidden style rule, and if not - it's literally one line of CSS that should be there anyway due to HTML5 compatibility.

@zzzzBov, the example from the spec you're citing is a scenario based on allowing the other "hidden" tabbed content to be accessible directly by screen readers, instead of setting up an interactive manner in which the hidden content is to be relayed to the screen reader.

If you're hiding tabbed content, and that content should be accessible to screen readers, either you use CSS that hides it without removing it from the accessibility API, or you hide it and also provide interactive ARIA-based controls to expose that content.

This is an authoring choice and bears no direct relevance to whether or not the hidden attribute should be used in general.

Nettsentrisk commented Jan 4, 2016

Because of this, I encourage the use of [aria-hidden], as it ties directly into the actual state of the element and also means that the element will be hidden in an accessible manner.

[hidden] => [aria-hidden=true]

The hidden attribute should be used if the intention of show/hide is to in fact hide it completely, from view and thus from the accessibility API. If the intention is to keep this in line with how show/hide have always been used, then this is the correct choice, as hide() => display:none which is essentially the same as setting a hidden attribute, which then means the accessibility API takes that as aria-hidden=true.

In regards to the problem of responsive, and perhaps you don't want to hide it in certain contexts when you use show/hide - well then I'd say you've designed your application poorly. If what you're attempting to hide should only be hidden in certain scenarios, then you should be toggling a class name or something similar to control whether it's visible or not through the CSS.

So, it makes sense for show/hide to remove/set the hidden attribute, respectively, because:

  1. It's the modern HTML5 equivalent of what show/hide/toggle have been doing all along
  2. It will force correct usage in scenarios where it's being used (wrong) in combination with responsive CSS

jQuery should not be creating any inline CSS for this; all modern browsers already support the hidden attribute natively. Any front end web developer with respect for themselves are already using a reset/normalize CSS setup that includes the hidden style rule, and if not - it's literally one line of CSS that should be there anyway due to HTML5 compatibility.

@zzzzBov, the example from the spec you're citing is a scenario based on allowing the other "hidden" tabbed content to be accessible directly by screen readers, instead of setting up an interactive manner in which the hidden content is to be relayed to the screen reader.

If you're hiding tabbed content, and that content should be accessible to screen readers, either you use CSS that hides it without removing it from the accessibility API, or you hide it and also provide interactive ARIA-based controls to expose that content.

This is an authoring choice and bears no direct relevance to whether or not the hidden attribute should be used in general.

@zzzzBov

This comment has been minimized.

Show comment
Hide comment
@zzzzBov

zzzzBov Jan 4, 2016

@Nettsentrisk

Any front end web developer with respect for themselves are already using a reset/normalize CSS setup that includes the hidden style rule, and if not - it's literally one line of CSS that should be there anyway due to HTML5 compatibility.

Except the CSS that's being used in normalizecss is

[hidden] {
  display: none;
}

Which is overridden by any rule that happens to have a higher specificity, such as:

#foo {
  display: block;
}

I brought up the issue a while back, and it was closed as a won't fix.

zzzzBov commented Jan 4, 2016

@Nettsentrisk

Any front end web developer with respect for themselves are already using a reset/normalize CSS setup that includes the hidden style rule, and if not - it's literally one line of CSS that should be there anyway due to HTML5 compatibility.

Except the CSS that's being used in normalizecss is

[hidden] {
  display: none;
}

Which is overridden by any rule that happens to have a higher specificity, such as:

#foo {
  display: block;
}

I brought up the issue a while back, and it was closed as a won't fix.

@Nettsentrisk

This comment has been minimized.

Show comment
Hide comment
@Nettsentrisk

Nettsentrisk Jan 5, 2016

Yeah, I think that just emulates what the native browser stylesheets are doing, which is also according to spec, which includes this note:

Because this attribute is typically implemented using CSS, it's also possible to override it using CSS. For instance, a rule that applies 'display: block' to all elements will cancel the effects of the hidden attribute. Authors therefore have to take care when writing their style sheets to make sure that the attribute is still styled as expected.

It also suggests user agent style to use for the attribute according to that.

Which inevitably leads to using "display: none !important" :)

Nettsentrisk commented Jan 5, 2016

Yeah, I think that just emulates what the native browser stylesheets are doing, which is also according to spec, which includes this note:

Because this attribute is typically implemented using CSS, it's also possible to override it using CSS. For instance, a rule that applies 'display: block' to all elements will cancel the effects of the hidden attribute. Authors therefore have to take care when writing their style sheets to make sure that the attribute is still styled as expected.

It also suggests user agent style to use for the attribute according to that.

Which inevitably leads to using "display: none !important" :)

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 13, 2016

Member

Fixed by dba93f7

Member

timmywil commented Jan 13, 2016

Fixed by dba93f7

@timmywil timmywil closed this Jan 13, 2016

@mgol mgol removed the Has Pull Request label Mar 6, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.