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

$.show() can break responsive stylesheets #1767

Closed
mgol opened this Issue Oct 21, 2014 · 4 comments

Comments

Projects
None yet
5 participants
@mgol
Member

mgol commented Oct 21, 2014

Originally reported by sten_paa@… at: http://bugs.jquery.com/ticket/15037

When $.show() is called the computed display value is cemented as an inline style. This can cause inconsistent behavior.

Fiddle:  http://jsfiddle.net/jinglesthula/ZttLJ/

Note that in the fiddle I use JavaScript to change the 'layout'. This simulates what would happen if the display: inline style were applied conditionally based on a responsive media query. That is, a given block of code could produce different results based on something like the orientation of a device and when the device orientation (really, the viewport width) changed with respect to when the show/hide calls are made.

I understand that based on the fact that an inline style is applied at the point when .show() is called that the browser is rendering the styles properly. The 'bug' I suppose comes in the fact that a dynamic computed value is assumed to be ok to translate to a more fixed inline style.

Resolution may involve finding a way to 'clean up' the inline style at the end of .show() and allow all styles not provided by jQuery to continue doing exactly what they were before show/hide were invoked.

Issue reported for jQuery 1.11.0

@mgol mgol added this to the 3.0.0 milestone Oct 21, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: gibson042

Cleanup is not possible in the current API because (for better or worse) .show() works on elements suppressed by stylesheet rules. This may be an issue best solved by plugin, but it does seem like we've taken the wrong path (which has also affected us through defaultDisplay). I'll leave this for 1.12/2.2 consideration.

Member

mgol commented Oct 21, 2014

Comment author: gibson042

Cleanup is not possible in the current API because (for better or worse) .show() works on elements suppressed by stylesheet rules. This may be an issue best solved by plugin, but it does seem like we've taken the wrong path (which has also affected us through defaultDisplay). I'll leave this for 1.12/2.2 consideration.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: dmethvin

Let's address this via docs, because I don't think there is any way we can read all possible intents with .show() and .hide(). In general I'd like to deprecate them and encourage people to use classes instead. There may be some code changes to also be made here (*simplifications* I would hope) so I'll leave the ticket open.

Member

mgol commented Oct 21, 2014

Comment author: dmethvin

Let's address this via docs, because I don't think there is any way we can read all possible intents with .show() and .hide(). In general I'd like to deprecate them and encourage people to use classes instead. There may be some code changes to also be made here (*simplifications* I would hope) so I'll leave the ticket open.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

Comment author: gibson042

It was noted in the meeting today that ignoring stylesheets and just looking at inline styles would resolve this, #15192, #13962, #9065, and a host of other show/hide/toggle bugs... and probably eliminate defaultDisplay to boot. Of course, there's no way to know about bugs that would have been filed with different behavior, but it's worth looking into the consequences of such a breaking change.

Member

mgol commented Oct 21, 2014

Comment author: gibson042

It was noted in the meeting today that ignoring stylesheets and just looking at inline styles would resolve this, #15192, #13962, #9065, and a host of other show/hide/toggle bugs... and probably eliminate defaultDisplay to boot. Of course, there's no way to know about bugs that would have been filed with different behavior, but it's worth looking into the consequences of such a breaking change.

@gibson042 gibson042 self-assigned this Dec 7, 2014

@timmywil timmywil added the CSS label Feb 1, 2015

@timmywil timmywil added the Blocker label Feb 10, 2015

gibson042 added a commit to gibson042/jquery that referenced this issue Mar 30, 2015

gibson042 added a commit to gibson042/jquery that referenced this issue Apr 5, 2015

gibson042 added a commit to gibson042/jquery that referenced this issue Apr 6, 2015

gibson042 added a commit that referenced this issue May 11, 2015

CSS: Ignore the CSS cascade in show()/hide()/etc.
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

@gibson042 gibson042 closed this in 86419b1 May 11, 2015

@jinglesthula

This comment has been minimized.

Show comment
Hide comment
@jinglesthula

jinglesthula Jul 27, 2016

On this page http://api.jquery.com/show/ a yellow Note callout was added indicating show can cause issues with responsive layout. Now that 3.0 has been released and fixes the issue* I'm wondering if the note should be updated to indicate that <3.0 has the responsive issue, unless there are still other responsive layout issues with show even in 3.0.

* at least in the fiddle above it fixed it when I switched it to 3.0 - I didn't do other testing regarding responsive layout + show/hide.

jinglesthula commented Jul 27, 2016

On this page http://api.jquery.com/show/ a yellow Note callout was added indicating show can cause issues with responsive layout. Now that 3.0 has been released and fixes the issue* I'm wondering if the note should be updated to indicate that <3.0 has the responsive issue, unless there are still other responsive layout issues with show even in 3.0.

* at least in the fiddle above it fixed it when I switched it to 3.0 - I didn't do other testing regarding responsive layout + show/hide.

@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.