Restore previous show/hide behavior #2654

Closed
timmywil opened this Issue Oct 16, 2015 · 26 comments

Projects

None yet

7 participants

@timmywil
Member

No description provided.

@timmywil timmywil added the CSS label Oct 16, 2015
@timmywil timmywil added this to the 3.0.0 milestone Oct 16, 2015
@mgol
Member
mgol commented Oct 16, 2015

We need to remember to re-close all solved tickets as won't fix (which means removing the milestones from them).

@gibson042 gibson042 was assigned by timmywil Oct 18, 2015
@timmywil timmywil assigned markelog and unassigned gibson042 Oct 26, 2015
@timmywil
Member

It sounds like @markelog wants to take this, if that's alright with @gibson042. Note that after restoring the old behavior, we'll still want to make a couple changes...

  1. only cache inline display values
  2. only cache the display value if it was set by the user
  3. Ensure we are initially trying to show elements by removing the display value

This means we are providing a partial fix for the responsive issue.

@gibson042
Member

I'm fine with yielding this, but make no mistake about what a monster it is. I'd like to see the following tables filled in, agreed upon, and then fixed and asserted in a PR:

Expected inline display

Non-animated

Initially block

Initial state .show() .hide() or .toggle() .hide().show() or .toggle().toggle() .show().hide()
default block, no inline
cascade block, no inline
default list-item, inline block
cascade list-item, inline block
cascade none, inline block

Initially inline

Initial state .show() .hide() or .toggle() .hide().show() or .toggle().toggle() .show().hide()
default inline, no inline
cascade inline, no inline
default block, inline inline
cascade block, inline inline
cascade none, inline inline

Initially list-item (or other non-block non-inline)

Initial state .show() .hide() or .toggle() .hide().show() or .toggle().toggle() .show().hide()
default list-item, no inline
cascade list-item, no inline
default inline, inline list-item
cascade inline, inline list-item
cascade none, inline list-item

Initially none

Initial state .show() or .toggle() .hide() .hide().show() .show().hide() or .toggle().toggle()
default none, no inline
cascade none, no inline
default block, inline none
default inline, inline none
default list-item, inline none
cascade block, inline none
cascade inline, inline none
cascade list-item, inline none
cascade none, inline none

Animated

Given the following helpers:

function toggle( T, callback ) {
    return jQuery( this ).toggle( T, callback );
}

function partial( T, callback ) {
    return toggle.call( this, T )
        .delay( T/2, "simultaneous" )
        .queue( "simultaneous", function( next ) {
            if ( callback ) {
                callback.call( this, T/2 );
            }
            next();
        } )
        .dequeue( "simultaneous" );
}

Initially block

Initial state partial.call(el, T) partial.call(el, T, toggle)
default block, no inline
cascade block, no inline
default list-item, inline block
cascade list-item, inline block
cascade none, inline block

Initially inline

Initial state partial.call(el, T) partial.call(el, T, toggle)
default inline, no inline
cascade inline, no inline
default block, inline inline
cascade block, inline inline
cascade none, inline inline

Initially list-item (or other non-block non-inline)

Initial state partial.call(el, T) partial.call(el, T, toggle)
default list-item, no inline
cascade list-item, no inline
default inline, inline list-item
cascade inline, inline list-item
cascade none, inline list-item

Initially none

Initial state partial.call(el, T) partial.call(el, T, toggle)
default none, no inline
cascade none, no inline
default block, inline none
default inline, inline none
default list-item, inline none
cascade block, inline none
cascade inline, inline none
cascade list-item, inline none
cascade none, inline none

I think you'll find that "restoring the old behavior" doesn't really help. Still want it, @markelog?

@markelog
Member

I'm sure i will have a lot of surprises filling in these tables :-), i think @gibson042 comment might be a good start for the blog post or some sort of documentation page or both.

I think we can do this on step by step basis, first of all - revert, then fill the tables, apply new changes mentioned by @timmywil, fill the tables again. Compare and find possible improvements. Apply the improvements. Then create a proposal or participate on existing one to DOM API that improve current situation.

/cc @AurelioDeRosa as it could became a documentation ticket and @mzgol for DOM API proposal.

@markelog
Member

I also think that restoring previous behaviour is essential for 3.0 final regardless of a possible mess we are in now...

@dmethvin
Member

i think @gibson042 comment might be a good start for the blog post or some sort of documentation page or both.

Yeah we need to put that somewhere that people can see how hairy this is! 😄

@gibson042
Member

first of all - revert, then fill the tables, apply new changes mentioned by @timmywil, fill the tables again. Compare and find possible improvements. Apply the improvements.

That's exactly what I'm recommending against... we need to fill in the tables before changing anything. There's no point in implementing behavior we can't define. It's also why I'm against tackling this by reverting commits–the old code and tests are no closer to the mark than master, and a lot messier.

@markelog
Member

That's exactly what I'm recommending against... we need to fill in the tables before changing anything.

Well, as the first step, we don't implement yet anything, we reverting. Behaviour is already defined in our docs, filling those tables means defining side-effects in some cases. Which isn't strictly necessary for most APIs.

From http://api.jquery.com/show/ (as an one example) -

This is roughly equivalent to calling .css( "display", "block"), except that the display property is restored to whatever it was initially. If an element has a display value of inline, then is hidden and shown, it will once again be displayed inline.

Users are fine with "current" behaviour, with two exceptions, since we received only two complains about show/hide logic - slow performance in some cases and inability to change previously caches display in environment with adaptive styles.

So besides those exceptions, we can recognize inconsistencies by filling in those tables and try to fix it, if we can...

Otherwise, instead of reverting, we might end up in implementing something else. Which is a bigger discussion and whole different kind of work.

That is why i think we shouldn't try to tackle the whole thing in one effort, but do it gradually, evaluating every step as we go along and try different tactics.

@dmethvin
Member

It seems like filling in that table would be a good thing though. Let's define what we want the code to do and then see if we can implement that. There is no problem with saying "don't care" or "not supported" for some of the cells either, but let's make that a conscious choice and not an accidental one.

@markelog
Member

Just to clarify - i think it is important to fill them up and document it

@scottgonzalez
Member

I've filled in the table for non-animated elements. I'd rather start with just half the scenarios before tackling the whole thing. See https://gist.github.com/scottgonzalez/5aadcd7e37cccced1239

@markelog
Member

Cool, i'd would propose though to put it somewhere in our test directive as interactive page/tests

@markelog
Member

What conclusion can we take from it btw? Is there something unexpected/incorrect/inconsistent? I don't see anything ambiguous

@scottgonzalez
Member

I'm not sure what you're asking. My table is the values that I expect, it does not line up with any implementation that has existed in jQuery.

@timmywil
Member

It doesn't, but I think it's closer to what we had before than what's in master now, not that I'm recommending reverting commits. I commented on the gist with one suggested revision.

@timmywil
Member

Also, I agree with @scottgonzalez. I think we can do an implementation that addresses non-animated first and see where we are.

@scottgonzalez
Member

Bah, stupid lack of notification on gists... I was going to put the table in a comment here, but figured this page would get crazy long if people started doing that. Anyway, I replied to your comments.

@markelog
Member

@scottgonzalez oh, i thought you were posting results as if we reverted to the previous behaviour.

@gibson042
Member

Do you want it as a Google spreadsheet? That or editing my comment above seems best for collaborative population.

@timmywil
Member

Editing the comment also doesn't notify, tho. How about a spreadsheet?

@scottgonzalez
Member

I didn't edit your table because that makes it hard to actually discuss changes and see what's going on. A spreadsheet where we can actually comment on individual fields would be great.

@scottgonzalez
Member

I've filled in the doc with my table.

@scottgonzalez
Member

I've filled in the two new columns in the Google doc.

@timmywil timmywil assigned gibson042 and unassigned markelog Nov 18, 2015
@timmywil timmywil added the Blocker label Nov 18, 2015
@gibson042 gibson042 added a commit to gibson042/jquery that referenced this issue Jan 11, 2016
@gibson042 gibson042 CSS: Restore cascade-override behavior in .show
Fixes gh-2654
Fixes gh-2308
Ref 86419b1
aabf960
@timmywil timmywil pushed a commit that closed this issue Jan 13, 2016
@gibson042 gibson042 + Timmy Willison CSS: Restore cascade-override behavior in .show
Fixes gh-2654
Fixes gh-2308
Close gh-2810
Ref 86419b1
dba93f7
@timmywil timmywil closed this in dba93f7 Jan 13, 2016
@AurelioDeRosa
Member

Hi everyone.

I've followed the discussion and I was wondering if you think it's a good time to improve the show()/hide() documentation with the table you have created here and to advocate what the best practices are for showing/hiding elements. So far, the only note that I've added was in commit 4cd3452c which might be not enough.

@dmethvin
Member

Some form of that table in the API docs seems like a good idea.

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