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

Don't advertise .show() on the front page #88

Closed
mgol opened this Issue Jan 30, 2015 · 15 comments

Comments

Projects
None yet
9 participants
@mgol
Member

mgol commented Jan 30, 2015

The .show() & .hide() methods are costly and they don't play well with responsive design. We shouldn't advertise them right on the front page.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol
Member

mgol commented Jan 30, 2015

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 1, 2015

Member

And switch to addClass? I wouldn't remove them, since they so cozy and all that :-).

We should update ajax example as well.

Member

markelog commented Feb 1, 2015

And switch to addClass? I wouldn't remove them, since they so cozy and all that :-).

We should update ajax example as well.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 1, 2015

Member

And yeah, since jquery/contribute.jquery.org#104 is a done deal, we would need to update all code examples so they would adhere to code style changes?

/cc @arthurvr

Member

markelog commented Feb 1, 2015

And yeah, since jquery/contribute.jquery.org#104 is a done deal, we would need to update all code examples so they would adhere to code style changes?

/cc @arthurvr

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Feb 1, 2015

Member

And switch to addClass?

Yes, for example. It's obviously harder to present in a small example but currently we're contradicting ourselves by saing .show()/.hide() should be avoided and yet presenting them on the front page.

And yeah, since jquery/contribute.jquery.org#104 is a done deal, we would need to update all code examples so they would adhere to code style changes?

That's a separate issue, let's not conflate them.

Member

mgol commented Feb 1, 2015

And switch to addClass?

Yes, for example. It's obviously harder to present in a small example but currently we're contradicting ourselves by saing .show()/.hide() should be avoided and yet presenting them on the front page.

And yeah, since jquery/contribute.jquery.org#104 is a done deal, we would need to update all code examples so they would adhere to code style changes?

That's a separate issue, let's not conflate them.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Feb 1, 2015

Member

I disagree that .show() and .hide() are inherently bad. I also disagree that the example on the homepage is bad or would cause problems in real world scenarios.

Member

scottgonzalez commented Feb 1, 2015

I disagree that .show() and .hide() are inherently bad. I also disagree that the example on the homepage is bad or would cause problems in real world scenarios.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 1, 2015

Member

@scottgonzalez Agreed.

There seems to be a misconception growing in the community that all JavaScript animations are bad. That isn't true. It's that applications should use tools to analyze performance and discover real bottlenecks. CSS animations can be very slow too.

Yet, I think what we're mostly referring to here is the cost of retrieving actual display values when the element is hidden – in a couple cases it is very costly – and then the problems caused when we cache those values, such as breaking responsive layouts. We are still discussing the implications of that and what we might do about it. I'd recommend we wait until we reach a resolution on that issue before we make changes to the homepage.

Member

timmywil commented Feb 1, 2015

@scottgonzalez Agreed.

There seems to be a misconception growing in the community that all JavaScript animations are bad. That isn't true. It's that applications should use tools to analyze performance and discover real bottlenecks. CSS animations can be very slow too.

Yet, I think what we're mostly referring to here is the cost of retrieving actual display values when the element is hidden – in a couple cases it is very costly – and then the problems caused when we cache those values, such as breaking responsive layouts. We are still discussing the implications of that and what we might do about it. I'd recommend we wait until we reach a resolution on that issue before we make changes to the homepage.

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 2, 2015

Member

It's that applications should use tools to analyze performance and discover real bottlenecks.

Let's do that.

I've recently profiled the load of wikipedia's new visual editor https://en.wikipedia.org/wiki/Barack_Obama?veaction=edit

Here's a quick look:
image

The heaviest self-time stack of JS during the editor load is from curCSS. It's the biggest bottleneck in loading the editor. And most of its work is coming from hide/show.

Was wikimedia surprised when they learned that their largest bottleneck is from calling jQuery(elem).hide()? Very.

Here's a single instance from this app:

image

And let's look at how it ended up getting used:

image

The logic inside showHandles is a little funny but the use of show/hide is straightforward. It's extremely simple imperative "i want to hide these" "i want to show these" statements.


With hide(), jQuery is doing much more magic behind the scenes than developers expect. And that magic comes at a significant price.

Aside: Now that I'm looking, this magic behavior doesn't make much sense. There seem to be two edge cases that have caused jQuery to punish every app's performance:

  1. div { display:none; } is defined and user wants to override that with show()
  2. User has a custom display value defined on the inline style.

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

Scott, Tim.. I ask you to load up https://en.wikipedia.org/wiki/Barack_Obama?veaction=edit and profile it with the browser and tools of your choice. The engineers behind this app used jQuery in a perfectly rational way, but it's fighting against them.

@atdt

Member

paulirish commented Feb 2, 2015

It's that applications should use tools to analyze performance and discover real bottlenecks.

Let's do that.

I've recently profiled the load of wikipedia's new visual editor https://en.wikipedia.org/wiki/Barack_Obama?veaction=edit

Here's a quick look:
image

The heaviest self-time stack of JS during the editor load is from curCSS. It's the biggest bottleneck in loading the editor. And most of its work is coming from hide/show.

Was wikimedia surprised when they learned that their largest bottleneck is from calling jQuery(elem).hide()? Very.

Here's a single instance from this app:

image

And let's look at how it ended up getting used:

image

The logic inside showHandles is a little funny but the use of show/hide is straightforward. It's extremely simple imperative "i want to hide these" "i want to show these" statements.


With hide(), jQuery is doing much more magic behind the scenes than developers expect. And that magic comes at a significant price.

Aside: Now that I'm looking, this magic behavior doesn't make much sense. There seem to be two edge cases that have caused jQuery to punish every app's performance:

  1. div { display:none; } is defined and user wants to override that with show()
  2. User has a custom display value defined on the inline style.

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

Scott, Tim.. I ask you to load up https://en.wikipedia.org/wiki/Barack_Obama?veaction=edit and profile it with the browser and tools of your choice. The engineers behind this app used jQuery in a perfectly rational way, but it's fighting against them.

@atdt

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Feb 2, 2015

Member

Let's assume .show() and .hide() are bad and therefore should not be used. How can a plugin developer show and hide elements if they can only do so through classes and every other plugin developer and page author must do the same, with no standard classes to be used?

I'd say @paulirish summed it up well with the question:

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

It's not even just the bottleneck, I don't think we can reasonably expect any solution other than .show() and .hide() to work for many developers.

Member

scottgonzalez commented Feb 2, 2015

Let's assume .show() and .hide() are bad and therefore should not be used. How can a plugin developer show and hide elements if they can only do so through classes and every other plugin developer and page author must do the same, with no standard classes to be used?

I'd say @paulirish summed it up well with the question:

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

It's not even just the bottleneck, I don't think we can reasonably expect any solution other than .show() and .hide() to work for many developers.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2015

Member

That's a separate issue, let's not conflate them.

I meant if this code gonna change it should follow new code style, but that would mean we would need to update everything, otherwise code examples would be inconsistent. Or we could follow old code style and update everything later.

@paulirish, @scottgonzalez, @timmywil at the meeting, couple months back, we discussed the possibility of significant logic simplification of those methods - throwing away any lookups whatsoever and just doing display:block/display:none. Not sure if it could be that simple, but its definitely worth looking into.

I think we need to be more careful with "not recommend" decisions. If there is one bad example out there it shouldn't outweigh all other use-cases.

I remember @Krinkle was working on new editor, maybe he has a bit more info about it.

Member

markelog commented Feb 2, 2015

That's a separate issue, let's not conflate them.

I meant if this code gonna change it should follow new code style, but that would mean we would need to update everything, otherwise code examples would be inconsistent. Or we could follow old code style and update everything later.

@paulirish, @scottgonzalez, @timmywil at the meeting, couple months back, we discussed the possibility of significant logic simplification of those methods - throwing away any lookups whatsoever and just doing display:block/display:none. Not sure if it could be that simple, but its definitely worth looking into.

I think we need to be more careful with "not recommend" decisions. If there is one bad example out there it shouldn't outweigh all other use-cases.

I remember @Krinkle was working on new editor, maybe he has a bit more info about it.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Feb 2, 2015

Member

Piggybacking off of what @markelog said, my concern is that .show()/.hide() is, for better or worse, already prevalent in lots of user code. I'd like to see us move in a direction that makes these methods faster (even if that means breaking some code), rather than giving up on them completely. It doesn't seem realistic to me to deprecate them.

@paulirish You're the man. Thank you for that write-up!

What's interesting to me is that it doesn't seem to be taking up time in defaultDisplay – which is our internal method for handling the first edge case you've described – but rather the native getPropertyValue called in curCSS to retrieve the current display value. However, that is even more disconcerting because removing defaultDisplay won't solve this issue. It seems that retrieving computed CSS for all of those elements is the bottleneck, which is very unfortunate because that isn't unique to .show()/.hide(). Maybe someone else could confirm what I'm seeing?

screenshot 2015-02-02 10 39 11

Member

timmywil commented Feb 2, 2015

Piggybacking off of what @markelog said, my concern is that .show()/.hide() is, for better or worse, already prevalent in lots of user code. I'd like to see us move in a direction that makes these methods faster (even if that means breaking some code), rather than giving up on them completely. It doesn't seem realistic to me to deprecate them.

@paulirish You're the man. Thank you for that write-up!

What's interesting to me is that it doesn't seem to be taking up time in defaultDisplay – which is our internal method for handling the first edge case you've described – but rather the native getPropertyValue called in curCSS to retrieve the current display value. However, that is even more disconcerting because removing defaultDisplay won't solve this issue. It seems that retrieving computed CSS for all of those elements is the bottleneck, which is very unfortunate because that isn't unique to .show()/.hide(). Maybe someone else could confirm what I'm seeing?

screenshot 2015-02-02 10 39 11

@MatmaRex

This comment has been minimized.

Show comment
Hide comment
@MatmaRex

MatmaRex Feb 2, 2015

Aside: Now that I'm looking, this magic behavior doesn't make much sense. There seem to be two edge cases that have caused jQuery to punish every app's performance:

  • div { display:none; } is defined and user wants to override that with show()
  • User has a custom display value defined on the inline style.

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

Further aside: This, or some related behavior, has caused me headache in at least one other situation when working on VisualEditor:
https://github.com/wikimedia/oojs-ui/blob/57b2cc74b44c9efa7a262d30ef1cfdb935fa6138/src/toolgroups/ListToolGroup.js#L76-L81. I would be quite happy to see it go away. :)

MatmaRex commented Feb 2, 2015

Aside: Now that I'm looking, this magic behavior doesn't make much sense. There seem to be two edge cases that have caused jQuery to punish every app's performance:

  • div { display:none; } is defined and user wants to override that with show()
  • User has a custom display value defined on the inline style.

Is baking in support for these two edge cases into one of jQuery's most simple looking APIs worth being the largest bottleneck in this editor?

Further aside: This, or some related behavior, has caused me headache in at least one other situation when working on VisualEditor:
https://github.com/wikimedia/oojs-ui/blob/57b2cc74b44c9efa7a262d30ef1cfdb935fa6138/src/toolgroups/ListToolGroup.js#L76-L81. I would be quite happy to see it go away. :)

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 2, 2015

Member

@MatmaRex: Your second headache appears to be jquery/jquery#1767, which we hope to resolve in 3.0 by ignoring cascaded display values for .show and .hide.

Member

gibson042 commented Feb 2, 2015

@MatmaRex: Your second headache appears to be jquery/jquery#1767, which we hope to resolve in 3.0 by ignoring cascaded display values for .show and .hide.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 3, 2015

Member

We discussed this on the last meeting, it was decided that we would like to improve those methods instead - jquery/jquery#2057

Member

markelog commented Feb 3, 2015

We discussed this on the last meeting, it was decided that we would like to improve those methods instead - jquery/jquery#2057

@jamesarosen

This comment has been minimized.

Show comment
Hide comment
@jamesarosen

jamesarosen commented Feb 9, 2015

(Edit: moved to jquery/jquery#2057 (comment))

@panayotov

This comment has been minimized.

Show comment
Hide comment
@panayotov

panayotov Feb 9, 2015

You can use safely $().hide(), if you've load jquery.gsap.js :)

panayotov commented Feb 9, 2015

You can use safely $().hide(), if you've load jquery.gsap.js :)

@jquery jquery locked and limited conversation to collaborators Feb 9, 2015

@rickydazla rickydazla referenced this issue Feb 10, 2015

Closed

Modalizing #2

atdt added a commit to wikimedia/mediawiki-extensions-UniversalLanguageSelector that referenced this issue Mar 9, 2015

Don't force #p-lang to show using jQuery.fn.show
It's a performance anti-pattern; see
<jquery/jquery.com#88 (comment)>

Change-Id: If6e521c27975f7d64932268fde5778aa63fd08ee

atdt added a commit to wikimedia/mediawiki-extensions that referenced this issue Mar 9, 2015

Updated mediawiki/extensions
Project: mediawiki/extensions/UniversalLanguageSelector  57b617f775b3e709e4d7cde69b77230aa73c88ae

Don't force #p-lang to show using jQuery.fn.show

It's a performance anti-pattern; see
<jquery/jquery.com#88 (comment)>

Change-Id: If6e521c27975f7d64932268fde5778aa63fd08ee

@jquery jquery unlocked this conversation Mar 25, 2015

samccone referenced this issue in twosigma/beakerx Mar 27, 2015

wjt added a commit to wjt/annotator that referenced this issue May 1, 2015

highlighter: remove redundant (?) call to $.show()
I don't understand why this would be necessary -- the element's just
been created, no style is (presumably) changing its display to anything
other than the default... -- and when highlighting all text in a 5000×3
table, $.show() accounts for 10s out of the 14s it takes to draw the
highlights.

$.show() being absurdly slow is a known problem:
jquery/jquery.com#88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment