slideDown, show and more stopped working for `display: none` elements

phistuck commented 9 years ago

Test case -

<!doctype html>
<script src="http://code.jquery.com/jquery-git2.js"></script>
<style>
div, p { display: none }
</style>
<div>stuff<br/>that<br/>takes<br/>a<br/>height</div>
<p>stuff<br/>that<br/>takes<br/>a<br/>height</p>
<script>
$("p").show();
$("div").slideDown();
</script>

Changing jquery-git2 to jquery-2.1.1 fixes the issue. This is a regression.

This is caused by https://github.com/jquery/jquery/commit/86419b10bfa5e3b71a7d416288ab806d47a31d1f

matthaywardwebdesign commented 9 years ago

@phistuck Can confirm, did break a client's site which happened to be using jquery-git.js. Seeing identical behaviour to above.

mgol commented 9 years ago

Wow, don't use jquery-git.js in production, ever.

matthaywardwebdesign commented 9 years ago

@mzgol Haha yep, never have since. This site happened to be my first paid job many a year ago. Definitely a bad idea, I'm surprised it survived as long as it did.

timmywil commented 9 years ago

This is an intended behavior change for jQuery 3.0. It fixes a few long-standing issues.

scottgonzalez commented 9 years ago

So for the extremely common case that was presented, what is the recommended solution?

jzaefferer commented 9 years ago

Somewhat related, if its supposed to go into 3.0, why is it changing jquery-gi2t?

mgol commented 9 years ago

@jzaefferer jquery-git2 currently mirrors jquery-git, jquery-git1 mirrors jquery-git-compat, i.e. both development lines. I've set it up this way some time ago so that some tools (like jsFiddle/jsBin) that pull the newest jQuery for testing still works. jsFiddle & jsBin now use the new URLs so we should just remove jquery-git2 & jquery-git1.

jzaefferer commented 9 years ago

I see. Well, you should start with permanent redirects before deleting them. For jQuery UI we use jquery-git and jquery-git2 for testing. Other projects might be doing the same, so some kind of official information about these changes would be quite useful.

mgol commented 9 years ago

@jzaefferer jquery-git & jquery-git2 point to the same file now so you're effectively testing master twice & compat not at all.

scottgonzalez commented 9 years ago

He meant that we test against jquery-git and jquery-git1.

scottgonzalez commented 9 years ago

Getting back on track though, what is the recommended solution to the extremely common use case that was presented?

timmywil commented 9 years ago

To hide with a class and remove that class (e.g. .removeClass('hidden')) or to put style="display: none" on the element itself, which can be done beforehand in HTML or later in JS (.css('display', 'block').hide()).

http://jsbin.com/tiqico/1/edit?html,css,js,output

That said, this behavior change is a trial run. With more tickets like this, we'll have to think of other solutions to the performance and cascade issues we fixed.

timmywil commented 9 years ago

Or possibly revert and live with certain issues.

scottgonzalez commented 9 years ago

To hide with a class and remove that class (e.g. .removeClass('hidden'))

So, to eliminate or unnecessarily complicate the use of basic animations. Also, this is impossible to do for any plugin author.

or to put style="display: none" on the element itself, which can be done beforehand in HTML or later in JS (.css('display', 'block').hide()).

So, inline styles or using JavaScript to set the preferred initial rendering state.

That said, this behavior change is a trial run. With more tickets like this, we'll have to think of other solutions to the performance and cascade issues we fixed.

I'd say this must be reverted or we'll end up with a massive uproar.

timmywil commented 9 years ago

So, to eliminate or unnecessarily complicate the use of basic animations

It's not that different than setting inline styles. http://jsbin.com/tiqico/2/edit?html,css,js,output

timmywil commented 9 years ago

I'd say this must be reverted or we'll end up with a massive uproar.

I don't think it's that simple. Basic usage of show/hide was causing noticeable performance issues.

timmywil commented 9 years ago

But we're still discussing this, so I'll reopen.

dmethvin commented 9 years ago

The reason we did this was exactly to see how much of an effect it would have, this is WIP after all and we haven't yet done a beta release or blog post about the changes.

There are certainly valid use cases for this gun, but unfortunately a common use seems to be for shooting feet. The compatibility arguments are definitely for keeping all the quirky edge cases so existing code will not have to change.

If we have to revert this I'll go back to recommending that people just avoid show/hide and use classes, since the nuances are too hard for most developers to understand. I don't say that to be disrespectful of them, it's just that these methods have evolved to be a lot more complex and expensive than a developer's intuition says they should be.

scottgonzalez commented 9 years ago

Well, no matter how much you recommend that, it will never be a viable solution for plugin authors.

gibson042 commented 9 years ago

As I alluded to in https://github.com/jquery/jquery/pull/2180#issuecomment-101360936, animating cascade-hidden elements can still be accomplished with "bring your own" defaultDisplay logic—which will be trivial in many cases (.removeClass) and damn near impossible in others. But our built-in all-singing all-dancing iframe implementation is completely untenable in my opinion—there's just no reliable way for us to figure out the proper display value of arbitrary hidden elements.

Mottie commented 9 years ago

@scottgonzalez mentions, this new method does make things very difficult for plugin authors.

This new method doesn't override HTML5's hidden attribute (demo). So now I would need to use:

$el.removeClass("hidden").removeAttr('hidden').show();

I don't understand why an iframe was needed previously, but given that the display option has a finite number of settings (and some that wouldn't even need to be supported like table-column & table-column-group), couldn't a cross-reference of display settings based on the element nodeName work just as well?

dmethvin commented 9 years ago

If the content is a hide/show candidate, it is unlikely you'd add or remove the hidden attribute dynamically. The spec says:

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, screen readers.

https://html.spec.whatwg.org/multipage/interaction.html#the-hidden-attribute

gibson042 commented 9 years ago

couldn't a cross-reference of display settings based on the element nodeName work just as well?

Aside from being enormous (and necessarily incomplete, because custom elements are now not only possible but headed towards standardization), such a list could also be rendered inaccurate by CSS overriding default display values. It's edge cases like those that first drove us to look up default values with an iframe when clearing inline display failed to show an element, and now drive us to stop trying. Like I keep saying, you know your application better than we do, and therefore know the right way to reveal an element for animation. We could leverage a cross-platform way to inspect the full cascade for getting it right, but absent that it doesn't seem worth the necessary contortions.

All that said, however, hidden is a special case that we can clear before every "show" animation, which I would be willing to do.

timmywil commented 9 years ago

All that said, however, hidden is a special case that we can clear before every "show" animation, which I would be willing to do.

But, like Dave pointed out, that doesn't even seem like correct usage of hidden.

timmywil commented 9 years ago

Like I keep saying, you know your application better than we do, and therefore know the right way to reveal an element for animation.

Playing devil's advocate for a second. While this is totally true, sometimes making a best guess is better than nothing. I was nervous about the behavior change with stylesheet-hidden elements and I still get the feeling this is going to cause an uproar. However, I don't want to revert everything we've done. What about falling back to display: block for stylesheet-hidden elements? We wouldn't need to read the display value (which was the performance issue on Obama's website), and we'd only do it when we don't have a display in data. If the user cares about different display values in responsive layouts, there would still be a way to make it work (i.e. not hiding elements in the stylesheet).

gibson042 commented 9 years ago

I mentioned the possibility in https://github.com/jquery/jquery/pull/2180#issuecomment-101794581 , but I'm curious how you intend to detect stylesheet-hidden elements without reading the display value (cf. https://github.com/jquery/jquery/commit/86419b10bfa5e3b71a7d416288ab806d47a31d1f#diff-414c8f59bff0d1b63680b64763d8c529L168 , replaced by https://github.com/jquery/jquery/commit/86419b10bfa5e3b71a7d416288ab806d47a31d1f#diff-f584c95a54f22b45ff73937ddd094f4dR19 ).

timmywil commented 9 years ago

We're going to do a blog post asking for community feedback on this issue.

markelog commented 9 years ago

Some code for the one of those ideas i mentioned at the meeting - https://github.com/jquery/jquery/issues/2374

lazd commented 9 years ago

@dmethvin, that paragraph under the example is a bit confusing, and I think you may have misinterpreted it when you said this:

If the content is a hide/show candidate, it is unlikely you'd add or remove the hidden attribute dynamically.

The example in the spec shows a situation where you set the hidden property dynamically -- the #login form is hidden and the #game element is shown once the user logs in.

I think we can agree that this is exactly how folks use jQuery's show() and hide() today.

To quote the guidelines in the spec:

The hidden attribute must not be used to hide content that could legitimately be shown in another presentation. ... 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, screen readers.

I interpret this as meaning that, if you intend for a screenreader to still be able to access the content, you shouldn't use [hidden]. But the same holds true for display: none -- when you use it, the element you use it on is totally hidden from everything (including screen readers).

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.

This makes a lot of assumptions about how a tabpanel is being used, and I think it's what tripped you up.

When read one way, it implies that you shouldn't use [hidden] (or even display: none!) on a tabpanel, which is not true. Every tabpanel implementation I've seen uses display: none or [hidden] on hidden on all panels not associated with the selected tab (and sometimes also aria-hidden to really hammer it in that it's hidden from screen readers too).

Read another way (and I think this was the intended meaning), it means that, if your intention is to perform "overflow management" -- that is, presenting the user with a subset of a large set of content -- then [hidden] is the wrong tool to use as it hides the rest of the content completely, when it should really only be "out of sight," yet not actively hidden from assistive technology and keyboard navigation.

Without debating the intention of a tabpanel, I think we can agree that, when a user calls jQuery.hide(), they intend for the content to be hidden visually and from screenreaders. That's exactly how it works now when hide() sets display: none, and I believe that toggling of the [hidden] attribute in jQuery's show() and hide() would be in line with the spec.

Eccenux commented 9 years ago

To my understanding the problem of using the iframe is with default display of some elements right? If so then why not just use something like in PR #2374 and simply allowing adding edge-cases for advanced developers. I would see this by documenting how to add new {elementName:defaultDisplay} mapping (to support defining new elements if someone must).

I also like the approach in jQueryMobile where they have a standard click event that just works and vclick for those that need more performance. Forcing someone to use something more complicated for something simple might end up just loosing him on the way. So, for high performance maybe introduce something like showBlock, hideBlock... and simply assume you always show/hide something that is an element with display:block. Or introduce fastShow, fastHide... that takes default display as the first parameter.

blyndcide commented 9 years ago

+1 for keeping the old behavior of hide/fade/slide around in some form. I attempted to use the velocity.js version of fade/slide. Yeah, it is faster, but not taking into account edge cases made it less useful.

e-oz commented 9 years ago

Please don't do it. CSS shouldn't depend on what JS lib you use. And plugin authors can't dictate to users if they can use display: none or not - it's ridiculous to expect all CSS styles will be revised just because some new jQuery plugin want it. They just will use jQuery 2.x and forget about upgrade.

Main point: CSS shouldn't care about JS libs, please follow single responsibility principle.

mgol commented 9 years ago

And plugin authors can't dictate to users if they can use display: none or not

One solution to this problem would be what @timmywil suggested in this comment, i.e.:

elem.css('display', 'block').hide();

This requires you to know the expected display value beforehand but otherwise will work without modifying the input stylesheets.

Eccenux commented 9 years ago

Unless jQuery will do some magic delay for css('display', 'block') running elem.css('display', 'block') will make the browser render elem and all it's descendants... Seems like a worse solution then using an inframe (at least performance wise).

If you really want to go that way why not just remove all show/hide alike functions. Seriously. If jQuery 3.0 won't be doing that to move away complication from devs, than maybe some other library will. Or maybe move it to UI or some optional jQuery build.

gibson042 commented 9 years ago

This is starting to look like general griping, so I'm going to ask some specific questions:

Have you tried the jQuery 3.0 alpha release? Excepting @phistuck and @matthaywardwebdesign, who obviously have.
Given that .show()ing a display: none element should generally leave the element without any inline display property (for e.g. responsive stylesheets), what is your expected end inline CSS of a cascade-hidden element? Keep in mind that we could only guess at a proper display, and that it may actually differ between elements of the same name.

Mottie commented 9 years ago

If the code is basically being boiled down to be a shortcut for elem.css('display', '') & elem.css('display', 'block'), I would have to agree with @Eccenux and say completely remove show, hide & toggle from jQuery.

The current code can be moved into an addon, if you even want to continue to support it. Either way, this change will make devs think more carefully about how to hide or show elements on their pages.

Eccenux commented 9 years ago

@gibson042 Do you mean a responsive website or a squishy website? The difference is that responsive website can render fine on a certain device. Once something is show it stays shown. Squishy website is something you showoff to your boss or friends by resizing a browser. Some things might disappear and then show again while you squish the browser back and forth. But most of the time it's not an actual use case.

Also I did try 3.0 and there is one solution I can see to make show/hide work as expected and that is running something like $('.hidden').hide().removeClass('hidden');. But that would break squishiness too (if I understand the use case correctly).

mr21 commented 9 years ago

@Eccenux, hmmm something not responsive is a web application who are detecting the device on the server-side and send back something only for this specific device. Having something responsive is having only one version of the design's code. So if the website doesn't respond when you resize your tab, it's not Responsive Web Design.

phistuck commented 9 years ago

@Mr21 - I actually agree with @Eccenux - resizing the tab is not necessarily the use case for a responsive design. It is a nice side effect. After all, resizing the tab to a mobile breakpoint may bring, for example, Geolocation features that are irrelevant to desktop users. Just because the user resized the tab, it does not mean they are suddenly mobile.

mr21 commented 9 years ago

@phistuck, okay, maybe if you are doing something like:

$(function() {
  var w = $(window).width();
  switch( true ) {
    case w < 1000:
      // ...
    case w < 500:
      // ...
  }
})

But, the responsive is only one version of the code for all, and 90% of the responsive design is about media queries.

Eccenux commented 9 years ago

Sorry, I didn't want to spark a new discussion about responsiveness and squishiness. The concept of being responsive just to show off squishiness was discussed by Brad Frost in his presentation. I kind of thought it got estabilished by now that responsiveness is not about squishiness but about adapting to screen size (or more exactly to device).

Yes, responsive websites should use media queries, but once the page is loaded, and initial view is established, most o the time you don't need to re-adapt (because -- other then for testing -- people don't squish websites around).

Besides. If you must you can always use !important in RWD.

But again I don't really see all this to be important for show/hide. Usually things that are shown are in some containers that don't change display that much (most of the time just change width and maybe loose float attribute).

Eccenux commented 9 years ago

BTW. I've made a fiddle with two simple examples that doesn't work in jQ3-alpha https://jsfiddle.net/c50p3Lmv/

Switch to jQ3 from 2.1 on the left. The workaround is not as bad as I originally thought (shouldn't cause performance issues), but I still find it confusing. Note that this is a very simple example as it assumes you use one single class to hide elements that are to be shown and that you can add display:none for all of them onload.

phistuck commented 9 years ago

@Mr21 - an example is very simple - showing or hiding an off canvas menu on mobile, but the menu is horizontally open on desktop (the media query hides or shows the button and the menu, but showing the menu on mobile needs some show and hide mechanism that the button triggers).

phistuck commented 9 years ago

@Eccenux - by the way, one specific use case for squishiness in responsive design is changing orientation.

Eccenux commented 9 years ago

@phistuck

by the way, one specific use case for squishiness in responsive design is changing orientation."

Yes, but I don't see any real-world use case where you would want to change display upon orientation change. I dont' think you should hide slide-out menu upon oriention change. If you can show me a use case I can show you an easy workaround.

Can someone show me a workarond for this one (without changing HTML or CSS): https://jsfiddle.net/40zuavo2/2/ (note that it works fine if you change to jQuery 2.x)

mgol commented 9 years ago

Can someone show me a workarond for this one (without changing HTML or CSS): https://jsfiddle.net/40zuavo2/2/

https://jsfiddle.net/40zuavo2/3/

Eccenux commented 9 years ago

Can someone show me a workarond for this one (without changing HTML or CSS): https://jsfiddle.net/40zuavo2/2/

https://jsfiddle.net/40zuavo2/3/

That won't work if I have more dialogs e.g. on other pages.

Eccenux commented 9 years ago

Also this is the one which makes the browser render the dialog (and all other hidden elements) for a brief moment. This will probably not be a problem on desktop, but might be a problem for mobile -- especially when other elements need to be recalculated when elements are shown.

mgol commented 9 years ago

Also this is the one which makes the browser render the dialog (and all other hidden elements) for a brief moment.

Do you have an example of a browser in which that happens? Browsers batch style recalculations so multiple changes don't trigger multiple reflows unless someone asks for the resolved style of the element in the meantime (because then the browser has to know the real value to return it). So this:

div.style.display = 'block';
div.style.display = 'none';

shouldn't show the element in any browser. The new .hide() doesn't check the resolved display so a reflow shouldn't happen.

It's true, though, that the reflow would happen if such code was executed with jQuery 2.1.4 or older where .hide() does check the resolved display.

Eccenux commented 9 years ago

It's true, though, that the reflow would happen if such code was executed with jQuery 2.1.4 or older where .hide() does check the resolved display .

Ahhhh. So that's what happened :-).

OK. So indeed .css('display', 'block').hide() should be a valid workaround for most cases.

jquery / jquery

slideDown, show and more stopped working for `display: none` elements #2308