CSS: Ignore the CSS cascade in show()/hide()/etc. #2180

gibson042 · 2015-03-30T16:24:50Z

Fixes gh-1767
Fixes gh-2071

No more showing or animating stylesheet-hidden elements. And please note the new use of jQuery.swap (which interacts with #2182).

   raw     gz Compared to master @ 062b5267d0a3538f1f6dee3df16da536b73061ea    
 -1335   -425 dist/jquery.js                                                   
  -433   -204 dist/jquery.min.js

WIP (failing effects tests) Fixes jquerygh-1767

Fixes broken tests

gibson042 · 2015-04-06T16:11:55Z

Ready for review. 🎆

Fixes jquerygh-2071

dmethvin · 2015-04-08T15:19:54Z

src/css.js

 	"./css/addGetHookIf",
 	"./css/support",
-	"./data/var/dataPriv",
+	"./css/showHide",


Can we pull .show() and .hide() into this file and remove the dependency inside css.js completely? That way it becomes a separate module and can be removed in a custom build if you use classes for showing and hiding.

So, introduce a new showHide module (.show, .hide, and .toggle) depending on data and depended upon by effects? That sounds reasonable, but out of scope for this ticket.

Yeah, it can go in a different ticket and pull request, I'll create an issue. Now that showHide is broken out it seems pretty clear we could do that, and in fact the new module only helps with size reductions if it can be excluded. As it is right now, it can't ever be excluded because of the dependency here.

dmethvin · 2015-04-10T20:31:11Z

I love this PR, it simplifies things so much. The current code was really over-thinking so many edge cases and making life miserable.

It is a huge breaking change though, so it will need a lot of advance warning before 3.0 ships. All those edge cases are no doubt being used by people in various places, and we'll need to update the docs for these methods to clarify the cases we no longer support.

timmywil · 2015-04-13T15:59:17Z

test/unit/css.js

 	hiddendiv.show();
-	equal( hiddendiv.css("display"), "block", "Pre-hidden div shown" );
+	equal( hiddendiv.css("display"), "none", "Show does not trump CSS cascade" );


This is the case that concerns me. I have used the stylesheet to hide elements initially and later called .show() on them. The new workflow – namely removing the "hidden" class – seems fine, but could require lots of changes by the user. We'll want to make sure this is covered in the blog post, docs, and migrate.

I've added a note to the draft blog post on it. Adding this to Migrate will be fun since it has to add back all the crap above.

Yes, that is the crux of it, and the reason why #1767 is a blocker. It seems worth it to me, especially after seeing the code changes, but does require some substantial changes from everyone trying to show stylesheet-hidden elements.

dmethvin · 2015-04-30T21:03:41Z

This should reference #2057 and would resolve it as far as I'm concerned.

timmywil · 2015-05-01T13:01:33Z

@dmethvin Thanks Dave. I agree, and now it does. :)

Fixes gh-1767 Fixes gh-2071 Closes gh-2180 (cherry picked from commit 86419b1) Conflicts: src/css.js src/css/defaultDisplay.js src/effects.js test/data/testsuite.css test/unit/css.js test/unit/effects.js

Mottie · 2015-05-12T05:03:12Z

I would like to suggest adding a boolean (e.g. .show(true)) to mimic the current behavior and show cascade-hidden elements. A named option would also be required if the developer is using an object. Sadly, this idea would not work with .toggle().

gibson042 · 2015-05-12T17:37:06Z

I don't see that happening, for a few reasons:

We're trying to avoid new boolean traps: https://github.com/jquery/jquery/wiki/Adding-new-features
It's easy to handle with client code (e.g., $el.removeClass("hidden").show() or—at worst—$el.css( "display", getVisibleDisplay ).show())
We relied on ugly kludges to determine substitute display values, causing new problems without even guaranteeing correctness

gibson042 · 2015-05-13T19:49:22Z

However, I just looked over the code again, and defaultDisplay may be separable from post-animation display restoration (which were the primary impetus for this PR). If it is, and if we're willing to pay the size and maintenance costs, it should be possible to bring back showing cascade-hidden elements without re-breaking gh-1767 and gh-2071 (except for initially-hidden elements, which are SOL regardless).

I'm still against it, but thought the idea should be on the record.

jquerybot added the CLA: Valid label Mar 30, 2015

gibson042 force-pushed the 1767 branch from 4ae9095 to c5e6c5f Compare April 5, 2015 20:15

gibson042 added 9 commits April 6, 2015 11:56

Effects: Improve comments

dde0aa8

CSS: Ignore the CSS cascade in show()/hide()/etc.

38f8819

WIP (failing effects tests) Fixes jquerygh-1767

Tests: Replace CSS & Effects #show-tests with inline display: none

f64a6b6

Tests: Generalize effects assertions from elem.style to jQuery.css

be3cda1

Tests: Avoid using getComputedStyle on disconnected elements

e8c6f33

Tests: Convert an effects test to fake asynchronicitiy

6e6eeb6

Effects: Reset display to inline before finishing a hide animation

dacb47e

Fixes broken tests

Tests: Remove "Animation ends immediately when document.hidden"

7b8b177

Effects: Abandon defaultDisplay

867b24b

gibson042 force-pushed the 1767 branch from c5e6c5f to 351092f Compare April 6, 2015 16:04

Effects: Reduce size

f5fdbd0

gibson042 force-pushed the 1767 branch from 351092f to f5fdbd0 Compare April 6, 2015 16:07

gibson042 changed the title ~~[WIP] CSS: Ignore the CSS cascade in show()/hide()/etc.~~ CSS: Ignore the CSS cascade in show()/hide()/etc. Apr 6, 2015

Effects: Reference other tickets

bda5c2b

Fixes jquerygh-2071

dmethvin reviewed Apr 8, 2015
View reviewed changes

dmethvin mentioned this pull request Apr 8, 2015

Break out .show(), .hide(), and .toggle() to a new module #2193

Closed

CSS: Rename private data key "olddisplay" for smaller size

65d3d4a

timmywil reviewed Apr 13, 2015
View reviewed changes

dmethvin mentioned this pull request Apr 14, 2015

Detect and add back exotic .show() and .hide() logic jquery/jquery-migrate#102

Closed

jdalton mentioned this pull request Apr 30, 2015

console.warn for .hide() and other perf gotchas. #2250

Closed

timmywil mentioned this pull request May 6, 2015

CSS: .show() will no longer override stylesheet css jquery/api.jquery.com#733

Closed

gibson042 closed this in 86419b1 May 11, 2015

gibson042 mentioned this pull request May 13, 2015

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

Closed

markelog mentioned this pull request Nov 16, 2015

2.2 Release progress #2723

Closed

markelog mentioned this pull request Dec 22, 2015

1.12 Release progress #2787

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSS: Ignore the CSS cascade in show()/hide()/etc. #2180

CSS: Ignore the CSS cascade in show()/hide()/etc. #2180

gibson042 commented Mar 30, 2015

gibson042 commented Apr 6, 2015

dmethvin Apr 8, 2015

gibson042 Apr 8, 2015

dmethvin Apr 8, 2015

dmethvin commented Apr 10, 2015

timmywil Apr 13, 2015

dmethvin Apr 14, 2015

gibson042 Apr 14, 2015

dmethvin commented Apr 30, 2015

timmywil commented May 1, 2015

Mottie commented May 12, 2015

gibson042 commented May 12, 2015

gibson042 commented May 13, 2015

CSS: Ignore the CSS cascade in show()/hide()/etc. #2180

CSS: Ignore the CSS cascade in show()/hide()/etc. #2180

Conversation

gibson042 commented Mar 30, 2015

gibson042 commented Apr 6, 2015

dmethvin Apr 8, 2015

Choose a reason for hiding this comment

gibson042 Apr 8, 2015

Choose a reason for hiding this comment

dmethvin Apr 8, 2015

Choose a reason for hiding this comment

dmethvin commented Apr 10, 2015

timmywil Apr 13, 2015

Choose a reason for hiding this comment

dmethvin Apr 14, 2015

Choose a reason for hiding this comment

gibson042 Apr 14, 2015

Choose a reason for hiding this comment

dmethvin commented Apr 30, 2015

timmywil commented May 1, 2015

Mottie commented May 12, 2015

gibson042 commented May 12, 2015

gibson042 commented May 13, 2015