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

Commit to how disconnected elements toggle #2863

Closed
gibson042 opened this Issue Jan 25, 2016 · 18 comments

Comments

Projects
None yet
4 participants
@gibson042
Member

gibson042 commented Jan 25, 2016

Ref #2404 (comment)

.toggle() uses the newly-renamed isHiddenWithinTree to determine whether a .show() or .hide() is called for, and effects methods use it to determine if they should invoke a preliminary .show(). But since isHiddenWithinTree does not yet perform exactly as advertised, all disconnected elements are treated as hidden. In code, jQuery( "<div><a/></div>" ).find( "*" ).addBack().toggle() locks in inline defaultDisplay values for both elements, as does $visible.detach().toggle().appendTo( inPageParent ).

This is counterintuitive to me, especially when it comes to element descendants like the above <a/>. I'd propose updating isHiddenWithinTree to use only inline display values when evaluating disconnected elements. Other possibilities include a) treating elements differently by parent (document or element vs. document fragment or null)—please no, and b) keeping everything as-is. But no matter what, we should either add unit tests asserting the consensus decision, or document that disconnected elements are invalid context for the methods.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 25, 2016

Member

but I don't think that should be the case for elements with element parents unless they have inline display: none (e.g., the <p> in jQuery("<div><p></div>"))

Why?! Again, this was working for years, no complaints, fast, small, clear and now we suddenly need to reverse the logic?

Member

markelog commented Jan 25, 2016

but I don't think that should be the case for elements with element parents unless they have inline display: none (e.g., the <p> in jQuery("<div><p></div>"))

Why?! Again, this was working for years, no complaints, fast, small, clear and now we suddenly need to reverse the logic?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 25, 2016

Member

But no matter what, we should either add unit tests asserting the consensus decision, or document that disconnected elements are invalid context for the methods.

That was a concision decision, until you removed those tests -

QUnit.test( "show() resolves correct default display for detached nodes", function( assert ) {

Member

markelog commented Jan 25, 2016

But no matter what, we should either add unit tests asserting the consensus decision, or document that disconnected elements are invalid context for the methods.

That was a concision decision, until you removed those tests -

QUnit.test( "show() resolves correct default display for detached nodes", function( assert ) {

@timmywil timmywil added this to the 3.0.0 milestone Jan 27, 2016

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 1, 2016

Member

That was a concision decision, until you removed those tests -

QUnit.test( "show() resolves correct default display for detached nodes", function( assert ) {

That is completely unrelated... note the complete absence of .toggle().

Member

gibson042 commented Feb 1, 2016

That was a concision decision, until you removed those tests -

QUnit.test( "show() resolves correct default display for detached nodes", function( assert ) {

That is completely unrelated... note the complete absence of .toggle().

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 1, 2016

Member

That is completely unrelated... note the complete absence of .toggle().

Those tests were done to support show()/hide() logic, and toggle() logic of showing or hiding something should be equivalent to show()/hide(), because toggle() is basically choosing what to call - show() or hide(), which also incapsulated logic of the choice.

p.s. sorry for verbosity

Member

markelog commented Feb 1, 2016

That is completely unrelated... note the complete absence of .toggle().

Those tests were done to support show()/hide() logic, and toggle() logic of showing or hiding something should be equivalent to show()/hide(), because toggle() is basically choosing what to call - show() or hide(), which also incapsulated logic of the choice.

p.s. sorry for verbosity

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 1, 2016

Member

Not at all, that's precisely the point of this ticket—deciding whether to call .show() or .hide(). It's different from the effects of .show() or .hide(), but ideally would coincide such that .show().toggle() is always equivalent to .hide() and .hide().toggle() is always equivalent to .show().

Member

gibson042 commented Feb 1, 2016

Not at all, that's precisely the point of this ticket—deciding whether to call .show() or .hide(). It's different from the effects of .show() or .hide(), but ideally would coincide such that .show().toggle() is always equivalent to .hide() and .hide().toggle() is always equivalent to .show().

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 1, 2016

Member

deciding whether to call

Yes, exactly, before it was the same logic of deciding, because it has to be the same logic, one should reflect the other, which is obvious relation i think, but now we have a discrepancy.

but ideally

Don't think "ideally" is correct word here, it seems "logically" is more on point.

Member

markelog commented Feb 1, 2016

deciding whether to call

Yes, exactly, before it was the same logic of deciding, because it has to be the same logic, one should reflect the other, which is obvious relation i think, but now we have a discrepancy.

but ideally

Don't think "ideally" is correct word here, it seems "logically" is more on point.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 1, 2016

Member

Don't think "ideally" is correct word here, it seems "logically" is more on point.

I agree, but that is not the case now, nor was it in the past.

Member

gibson042 commented Feb 1, 2016

Don't think "ideally" is correct word here, it seems "logically" is more on point.

I agree, but that is not the case now, nor was it in the past.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

I agree, but that is not the case now, nor was it in the past.

I would count it as a bug :/.

Member

markelog commented Feb 2, 2016

I agree, but that is not the case now, nor was it in the past.

I would count it as a bug :/.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 2, 2016

Member

Then why does it seem like you're arguing against a fix?

Member

gibson042 commented Feb 2, 2016

Then why does it seem like you're arguing against a fix?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

I'm arguing for keeping the old logic, whereas fix for that bug could be an additional to the old behaviour, not argument for its replacement.

My position comes down to two statements:

  1. show()/hide() and toggle() should use same exact condition for determining if element is hidden or not. I think it should be given truth
  2. Previous behaviour was not only in line with first condition but was battle tested and there is no arguments for its change
Member

markelog commented Feb 2, 2016

I'm arguing for keeping the old logic, whereas fix for that bug could be an additional to the old behaviour, not argument for its replacement.

My position comes down to two statements:

  1. show()/hide() and toggle() should use same exact condition for determining if element is hidden or not. I think it should be given truth
  2. Previous behaviour was not only in line with first condition but was battle tested and there is no arguments for its change
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 2, 2016

Member
  1. show()/hide() and toggle() should use same exact condition for determining if element is hidden or not. I think it should be given truth

.show() and .hide() never need to determine if an element is hidden. .show() needs to guess when setting a non-empty inline display is both necessary and sufficient to give an element layout, but the distinction is significant. Determining if an element is hidden matters only to methods that might need to .show() it, not to those that already know a priori.

  1. Previous behaviour was not only in line with first condition but was battle tested and there is no arguments for its change

Repeating that claim doesn't make it true. There were arguments for it to change (principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades), and I've already demonstrated its inconsistency.

Member

gibson042 commented Feb 2, 2016

  1. show()/hide() and toggle() should use same exact condition for determining if element is hidden or not. I think it should be given truth

.show() and .hide() never need to determine if an element is hidden. .show() needs to guess when setting a non-empty inline display is both necessary and sufficient to give an element layout, but the distinction is significant. Determining if an element is hidden matters only to methods that might need to .show() it, not to those that already know a priori.

  1. Previous behaviour was not only in line with first condition but was battle tested and there is no arguments for its change

Repeating that claim doesn't make it true. There were arguments for it to change (principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades), and I've already demonstrated its inconsistency.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

.show() and .hide() never need to determine if an element is hidden.

That is simple not true -
https://github.com/jquery/jquery/blob/2.1.0/src/css.js#L176
https://github.com/jquery/jquery/blob/2.1.0/src/css.js#L182

Or i don't understand what you are trying to say. Above way of determining if element is hidden should be the same as in toggle().

Repeating that claim doesn't make it true.

I repeated it because it seems you were confused by my position, so i just summarized it for you.

There were arguments for it to change (principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades), and I've already demonstrated its inconsistency.

It still sets inline display, but only in some cases, so that is still there, and won't go away. Our preferred end result was to improve the performance, that was what "asked" from us, which we did, no one is "asked" us to change the logic, no one found it unexpected or confusing, yes, there is that case of responsive stylesheets, but as i have said, that is still there.

In other words -

principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades

that is not sufficient argument for me, that is why i stand by my position.

We can leave the perf part improvement but hook out the new logic. Those do not contradict. So if do change the logic of the show()/hide() back we wouldn't need to fix that ticket, but would need to consider fixing the inconsistency you showed.

Member

markelog commented Feb 2, 2016

.show() and .hide() never need to determine if an element is hidden.

That is simple not true -
https://github.com/jquery/jquery/blob/2.1.0/src/css.js#L176
https://github.com/jquery/jquery/blob/2.1.0/src/css.js#L182

Or i don't understand what you are trying to say. Above way of determining if element is hidden should be the same as in toggle().

Repeating that claim doesn't make it true.

I repeated it because it seems you were confused by my position, so i just summarized it for you.

There were arguments for it to change (principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades), and I've already demonstrated its inconsistency.

It still sets inline display, but only in some cases, so that is still there, and won't go away. Our preferred end result was to improve the performance, that was what "asked" from us, which we did, no one is "asked" us to change the logic, no one found it unexpected or confusing, yes, there is that case of responsive stylesheets, but as i have said, that is still there.

In other words -

principally its eagerness to permanently and unnecessarily set inline display values derived from circumstantial style cascades

that is not sufficient argument for me, that is why i stand by my position.

We can leave the perf part improvement but hook out the new logic. Those do not contradict. So if do change the logic of the show()/hide() back we wouldn't need to fix that ticket, but would need to consider fixing the inconsistency you showed.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

.show() needs to guess when setting a non-empty inline display is both necessary and sufficient to give an element layout, but the distinction is significant. Determining if an element is hidden matters only to methods that might need to .show() it, not to those that already know a priori.

We can call it significant, we can say that toggle() needs to guess, we can say that it does or doesn't matter for show()/hide() or toggle() whatever, but the way of determining should be the same.

That is my stands anyhow.

Member

markelog commented Feb 2, 2016

.show() needs to guess when setting a non-empty inline display is both necessary and sufficient to give an element layout, but the distinction is significant. Determining if an element is hidden matters only to methods that might need to .show() it, not to those that already know a priori.

We can call it significant, we can say that toggle() needs to guess, we can say that it does or doesn't matter for show()/hide() or toggle() whatever, but the way of determining should be the same.

That is my stands anyhow.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

Same for fadeTo btw

Member

markelog commented Feb 2, 2016

Same for fadeTo btw

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Feb 2, 2016

Member

To summarize my position:

  • .hide() should completely ignore the hiddenness of elements
  • .show() should too, except when deciding to try forcing layout, a very specific goal
    • i.e., that is it desirable (empty inline display AND computed display: none) and possible (non-disconnected)
  • .toggle(), .fadeTo(), .animate(), etc. need to know when elements are hidden (because it affects their behavior), and should all use the same logic to determine that
    • …which I believe should not assume disconnected elements to be hidden, but that's for this ticket to determine
Member

gibson042 commented Feb 2, 2016

To summarize my position:

  • .hide() should completely ignore the hiddenness of elements
  • .show() should too, except when deciding to try forcing layout, a very specific goal
    • i.e., that is it desirable (empty inline display AND computed display: none) and possible (non-disconnected)
  • .toggle(), .fadeTo(), .animate(), etc. need to know when elements are hidden (because it affects their behavior), and should all use the same logic to determine that
    • …which I believe should not assume disconnected elements to be hidden, but that's for this ticket to determine
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 2, 2016

Member

.hide() should completely ignore the hiddenness of elements

+1, if there is no logic for determination, then there is nothing to explore

.show() should too...

-1

.toggle(), .fadeTo(), .animate(), etc. need to...

+1

…which I believe should not assume disconnected elements to be hidden

-1

Member

markelog commented Feb 2, 2016

.hide() should completely ignore the hiddenness of elements

+1, if there is no logic for determination, then there is nothing to explore

.show() should too...

-1

.toggle(), .fadeTo(), .animate(), etc. need to...

+1

…which I believe should not assume disconnected elements to be hidden

-1

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Feb 10, 2016

Member

So have we come to agreement on this? The reason I ask is that this is a blocker for 3.0.0 and also I can't even grasp how to warn or shim in the old behavior for Migrate without a list of behavioral changes. We'll also need that for the docs.

Member

dmethvin commented Feb 10, 2016

So have we come to agreement on this? The reason I ask is that this is a blocker for 3.0.0 and also I can't even grasp how to warn or shim in the old behavior for Migrate without a list of behavioral changes. We'll also need that for the docs.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Feb 10, 2016

Member

Yeah, i need to submit a pull request with the details in it, was in a bit of swamp lately, should do one this week, sorry Dave.

Member

markelog commented Feb 10, 2016

Yeah, i need to submit a pull request with the details in it, was in a bit of swamp lately, should do one this week, sorry Dave.

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