-
Notifications
You must be signed in to change notification settings - Fork 20.6k
Commit to how disconnected elements toggle #2863
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
Comments
Why?! Again, this was working for years, no complaints, fast, small, clear and now we suddenly need to reverse the logic? |
That was a concision decision, until you removed those tests - Line 559 in c4d9eac
|
That is completely unrelated... note the complete absence of |
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 |
Not at all, that's precisely the point of this ticket—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.
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. |
I would count it as a bug :/. |
Then why does it seem like you're arguing against a fix? |
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:
|
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. |
That is simple not true - 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
I repeated it because it seems you were confused by my position, so i just summarized it for you.
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 -
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 |
We can call it significant, we can say that That is my stands anyhow. |
Same for |
To summarize my position:
|
+1, if there is no logic for determination, then there is nothing to explore
-1
+1
-1 |
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. |
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. |
Ref #2404 (comment)
.toggle()
uses the newly-renamedisHiddenWithinTree
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 sinceisHiddenWithinTree
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 inlinedefaultDisplay
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 updatingisHiddenWithinTree
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.The text was updated successfully, but these errors were encountered: