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

Discuss new show/hide logic (mostly for detached element) #2854

Closed
markelog opened this Issue Jan 20, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@markelog
Member

markelog commented Jan 20, 2016

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 20, 2016

Member

Context:

// jQuery 2.x
elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)
// jQuery 3.0
elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
 // Support: Firefox <=42 - 43
 jQuery.contains( elem.ownerDocument, elem)

Note that that 3.0 is logically only the first line—the containment check was added because Gecko reports disconnected elements as having computed display: none, whereas WebKit and Blink report empty.

in 3.0 we have -

/*case #1*/$("<div/>").show().appendTo("body")[0].style.display; // ""
/*case #2*/$("<div/>").hide().appendTo("body")[0].style.display; // "none"

Whereas before, in 2.x, we had -

/*case #2*/$("<div/>").show().appendTo("body")[0].style.display; // "block"
/*case #2*/$("<div/>").hide().appendTo("body")[0].style.display; // "none"

Which means for the case #2 in 3.0 we expect for the user to know where their want to append newly created element, but for the case #1 in 3.0 we don't. So in new concept, detached elements are now considered to be shown, which is counterintuitive and inconsistent in my opinion.

I disagree with that summary of 3.0 behavior, because it always respects inline display above all else. What changed with respect to disconnected elements is that we no longer make any assumptions about cascaded display, which is why those without inline display are treated as visible.

I believe previous behaviour was correct and expected, since we never seen complaints about it.

#1767 and #2071 are the complaints about locking in default display.

So for the case, when user shouldn't call show() - cascade-visible less work (no contains) we improved performance and for the case when user should call show() - cascade-hidden more work (contains) performance is worse.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast. I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

I'd like these cases -

show()
cascade-visible less work (no contains) 
cascade-hidden  more work (contains)

to be reversed. I believe if we do it, behaviour or performance wouldn't suffer, but would be improved.

You seem to be focusing on a microcosm of show/hide behavior. But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere" by introducing a support test. If there's consensus on desiring that, I'll put up a PR.

Member

gibson042 commented Jan 20, 2016

Context:

// jQuery 2.x
elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)
// jQuery 3.0
elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
 // Support: Firefox <=42 - 43
 jQuery.contains( elem.ownerDocument, elem)

Note that that 3.0 is logically only the first line—the containment check was added because Gecko reports disconnected elements as having computed display: none, whereas WebKit and Blink report empty.

in 3.0 we have -

/*case #1*/$("<div/>").show().appendTo("body")[0].style.display; // ""
/*case #2*/$("<div/>").hide().appendTo("body")[0].style.display; // "none"

Whereas before, in 2.x, we had -

/*case #2*/$("<div/>").show().appendTo("body")[0].style.display; // "block"
/*case #2*/$("<div/>").hide().appendTo("body")[0].style.display; // "none"

Which means for the case #2 in 3.0 we expect for the user to know where their want to append newly created element, but for the case #1 in 3.0 we don't. So in new concept, detached elements are now considered to be shown, which is counterintuitive and inconsistent in my opinion.

I disagree with that summary of 3.0 behavior, because it always respects inline display above all else. What changed with respect to disconnected elements is that we no longer make any assumptions about cascaded display, which is why those without inline display are treated as visible.

I believe previous behaviour was correct and expected, since we never seen complaints about it.

#1767 and #2071 are the complaints about locking in default display.

So for the case, when user shouldn't call show() - cascade-visible less work (no contains) we improved performance and for the case when user should call show() - cascade-hidden more work (contains) performance is worse.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast. I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

I'd like these cases -

show()
cascade-visible less work (no contains) 
cascade-hidden  more work (contains)

to be reversed. I believe if we do it, behaviour or performance wouldn't suffer, but would be improved.

You seem to be focusing on a microcosm of show/hide behavior. But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere" by introducing a support test. If there's consensus on desiring that, I'll put up a PR.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jan 20, 2016

Member

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere" by introducing a support test. If there's consensus on desiring that, I'll put up a PR.

Related: would checking elem.getClientRects().length be faster? We use it in other places in place of jQuery.contains if we're just checking an element for being disconnected or not.

Member

mgol commented Jan 20, 2016

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere" by introducing a support test. If there's consensus on desiring that, I'll put up a PR.

Related: would checking elem.getClientRects().length be faster? We use it in other places in place of jQuery.contains if we're just checking an element for being disconnected or not.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 20, 2016

Member

getClientRects answers a different question than contains, and essentially conflates detachment with computed display: none. It might be able to replace jQuery.css in the 3.0 code if there's a performance boost, but it couldn't replace contains (and would in fact require it for all browsers instead of just Firefox).

Member

gibson042 commented Jan 20, 2016

getClientRects answers a different question than contains, and essentially conflates detachment with computed display: none. It might be able to replace jQuery.css in the 3.0 code if there's a performance boost, but it couldn't replace contains (and would in fact require it for all browsers instead of just Firefox).

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 21, 2016

Member

Also, I would characterize inline display: none toggling as the preferred mode of operation, and cascade-overriding as legacy behavior. Where performance truly matters, users should be utilizing the former (which won't ever hit that contains).

Member

gibson042 commented Jan 21, 2016

Also, I would characterize inline display: none toggling as the preferred mode of operation, and cascade-overriding as legacy behavior. Where performance truly matters, users should be utilizing the former (which won't ever hit that contains).

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 21, 2016

Member

Note that that 3.0 is logically only the first line—the containment check was added because Gecko reports disconnected elements as having computed display: none, whereas WebKit and Blink report empty.

It doesn't really matter why it was added, now it matters that it is there.

I disagree with that summary of 3.0 behavior, because it always respects inline display above all else. What changed with respect to disconnected elements is that we no longer make any assumptions about cascaded display, which is why those without inline display are treated as visible.

I understand the rational for it, but i guess you can see who it would be interpreted from my point of view, so yes, elements without display are treated as visible, but in the same truth - detached elements are now visible.

#1767 and #2071 are the complaints about locking in default display.

Those tickets has nothing to do with detached elements and those tickets are still very actual.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast.

Users wanted for it to be faster, not sure about "fast"

I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

Not sure what you are trying to say...

You seem to be focusing on a microcosm of show/hide behavior.

I care more about the logic, once again, we worsen the expected behaviour by improving incorrect one.

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere"

By increasing size :/, we already increased the size by not using isHidden.

but it couldn't replace contains (and would in fact require it for all browsers instead of just Firefox).

We already checking if element is attached with getClientRects and i did replace contains() in isHidden method with getClientRects(), all tests are good. So i'm not sure why you have this opinion.

Member

markelog commented Jan 21, 2016

Note that that 3.0 is logically only the first line—the containment check was added because Gecko reports disconnected elements as having computed display: none, whereas WebKit and Blink report empty.

It doesn't really matter why it was added, now it matters that it is there.

I disagree with that summary of 3.0 behavior, because it always respects inline display above all else. What changed with respect to disconnected elements is that we no longer make any assumptions about cascaded display, which is why those without inline display are treated as visible.

I understand the rational for it, but i guess you can see who it would be interpreted from my point of view, so yes, elements without display are treated as visible, but in the same truth - detached elements are now visible.

#1767 and #2071 are the complaints about locking in default display.

Those tickets has nothing to do with detached elements and those tickets are still very actual.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast.

Users wanted for it to be faster, not sure about "fast"

I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

Not sure what you are trying to say...

You seem to be focusing on a microcosm of show/hide behavior.

I care more about the logic, once again, we worsen the expected behaviour by improving incorrect one.

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere"

By increasing size :/, we already increased the size by not using isHidden.

but it couldn't replace contains (and would in fact require it for all browsers instead of just Firefox).

We already checking if element is attached with getClientRects and i did replace contains() in isHidden method with getClientRects(), all tests are good. So i'm not sure why you have this opinion.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 21, 2016

Member

So my two main concerns are perf and compat. I think we established that the code now in master/3.0.0 will be faster for at least a few important cases, which was one of the goals. Maybe we can play that up in the blog post with some graphs, this is going to be the spoonful of sugar that helps the medicine go down. Plus it makes Paul Irish happy!

The compat concern is best answered by looking at some cases that break and proposing reasonable solutions that people can use to get around them. When we tried that with the earlier purist approach we kept reaching points where we just didn't have anything to offer. So, what breaks and how will people fix it? If "hide detached elements" fails for some cases, I'm totally okay with asking people to change $(elem).hide().appendTo("body") into $(elem).appendTo("body").hide() for example. We just need to explain what they should do.

Member

dmethvin commented Jan 21, 2016

So my two main concerns are perf and compat. I think we established that the code now in master/3.0.0 will be faster for at least a few important cases, which was one of the goals. Maybe we can play that up in the blog post with some graphs, this is going to be the spoonful of sugar that helps the medicine go down. Plus it makes Paul Irish happy!

The compat concern is best answered by looking at some cases that break and proposing reasonable solutions that people can use to get around them. When we tried that with the earlier purist approach we kept reaching points where we just didn't have anything to offer. So, what breaks and how will people fix it? If "hide detached elements" fails for some cases, I'm totally okay with asking people to change $(elem).hide().appendTo("body") into $(elem).appendTo("body").hide() for example. We just need to explain what they should do.

@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 21, 2016

Member

I understand the rational for it, but i guess you can see who it would be interpreted from my point of view, so yes, elements without display are treated as visible, but in the same truth - detached elements are now visible.

Except those with inline display: none, e.g. from a previous .hide().

#1767 and #2071 are the complaints about locking in default display.

Those tickets has nothing to do with detached elements and those tickets are still very actual.

They were caused by the very lines of code in question here.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast.

Users wanted for it to be faster, not sure about "fast"

Just .show()? How about .hide()? Or .toggle()? And just on the first run, or on repeats? And again, if they really want speed then they shouldn't be relying on us to detect cascaded display: none—they should be using classes or inline display. But even so, we still made things faster in many cases.

I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

Not sure what you are trying to say...

DUDE.

I care more about the logic, once again, we worsen the expected behaviour by improving incorrect one.

I'd say overriding the CSS cascade with a guess is already at least on the border of "incorrect".

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere"

By increasing size :/, we already increased the size by not using isHidden.

isHidden is the wrong tool for the job. It breaks $detached.show().

We already checking if element is attached with getClientRects and i did replace contains() in isHidden method with getClientRects(), all tests are good. So i'm not sure why you have this opinion.

getClientRects() doesn't map to "attached", it maps to "has layout" (which implies "attached", but not vice versa). So it's perfect for isHidden :hidden, but wrong here.

Member

gibson042 commented Jan 21, 2016

I understand the rational for it, but i guess you can see who it would be interpreted from my point of view, so yes, elements without display are treated as visible, but in the same truth - detached elements are now visible.

Except those with inline display: none, e.g. from a previous .hide().

#1767 and #2071 are the complaints about locking in default display.

Those tickets has nothing to do with detached elements and those tickets are still very actual.

They were caused by the very lines of code in question here.

I don't think it's fair to assume that .show()ing cascade-hidden elements will be fast.

Users wanted for it to be faster, not sure about "fast"

Just .show()? How about .hide()? Or .toggle()? And just on the first run, or on repeats? And again, if they really want speed then they shouldn't be relying on us to detect cascaded display: none—they should be using classes or inline display. But even so, we still made things faster in many cases.

I do think it's fair to assume that .show()ing elements won't break a site's (responsive) layout.

Not sure what you are trying to say...

DUDE.

I care more about the logic, once again, we worsen the expected behaviour by improving incorrect one.

I'd say overriding the CSS cascade with a guess is already at least on the border of "incorrect".

But even so, we could easily convert that "more work (contains)" into "more work in Firefox (contains), unchanged elsewhere"

By increasing size :/, we already increased the size by not using isHidden.

isHidden is the wrong tool for the job. It breaks $detached.show().

We already checking if element is attached with getClientRects and i did replace contains() in isHidden method with getClientRects(), all tests are good. So i'm not sure why you have this opinion.

getClientRects() doesn't map to "attached", it maps to "has layout" (which implies "attached", but not vice versa). So it's perfect for isHidden :hidden, but wrong here.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 25, 2016

Member

Except those with inline display: none, e.g. from a previous .hide().

Yeah, but then we have explicit desire from the user to make them hidden, so my point still holds.

They were caused by the very lines of code in question here.

Those tickets wasn't resolved with the new logic, so there is actually no code that tries to correct it.

#1767 and #2071 are still relevant, like (from #1767) -

display value is cemented as an inline style

Is still true.

But even so, we still made things faster in many cases.

Yes, and goodie! But we changed the logic (worsen by improving... and all that stuff i have said repeatedly), whereas we can yet still improve the perf and make the logic as it was before.

DUDE.

I meant those tickets are still relevant

I'd say overriding the CSS cascade with a guess is already at least on the border of "incorrect".

No-no, you confusing our "incorrect" with user "incorrect" behaviour. One might argue we shouldn't "overriding the CSS cascade with a guess" but it is best that we can do, user, on the other hand, shouldn't call hide() on already hidden element.

isHidden is the wrong tool for the job. It breaks $detached.show().

I'm saying it would improve it, not break it.

What disturbing me the most, that i'm, yet, still didn't saw any justification for reversing the logic for disconnected nodes that we had for three years and didn't receive any complaints about.

getClientRects() doesn't map to "attached", it maps to "has layout" (which implies "attached", but not vice versa). So it's perfect for isHidden :hidden, but wrong here.

Whatever gets the job done, personally i don't care what the method called, we use DOM methods in most unconventional ways, because it is broken in most unconventional ways. But that is not what is discussion is about.

Member

markelog commented Jan 25, 2016

Except those with inline display: none, e.g. from a previous .hide().

Yeah, but then we have explicit desire from the user to make them hidden, so my point still holds.

They were caused by the very lines of code in question here.

Those tickets wasn't resolved with the new logic, so there is actually no code that tries to correct it.

#1767 and #2071 are still relevant, like (from #1767) -

display value is cemented as an inline style

Is still true.

But even so, we still made things faster in many cases.

Yes, and goodie! But we changed the logic (worsen by improving... and all that stuff i have said repeatedly), whereas we can yet still improve the perf and make the logic as it was before.

DUDE.

I meant those tickets are still relevant

I'd say overriding the CSS cascade with a guess is already at least on the border of "incorrect".

No-no, you confusing our "incorrect" with user "incorrect" behaviour. One might argue we shouldn't "overriding the CSS cascade with a guess" but it is best that we can do, user, on the other hand, shouldn't call hide() on already hidden element.

isHidden is the wrong tool for the job. It breaks $detached.show().

I'm saying it would improve it, not break it.

What disturbing me the most, that i'm, yet, still didn't saw any justification for reversing the logic for disconnected nodes that we had for three years and didn't receive any complaints about.

getClientRects() doesn't map to "attached", it maps to "has layout" (which implies "attached", but not vice versa). So it's perfect for isHidden :hidden, but wrong here.

Whatever gets the job done, personally i don't care what the method called, we use DOM methods in most unconventional ways, because it is broken in most unconventional ways. But that is not what is discussion is about.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 25, 2016

Member

@dmethvin

If "hide detached elements" fails for some cases, I'm totally okay with asking people to change $(elem).hide().appendTo("body") into $(elem).appendTo("body").hide() for example. We just need to explain what they should do.

It is really hard to make arguments when situation is not clear to most participants, i.e. hide() works perfectly on detached elements, you don't need to do $(elem).appendTo("body").hide(), that is not what we discussing here :/. I tried to explain this couple times, i guess i can't be more clear then that.

Member

markelog commented Jan 25, 2016

@dmethvin

If "hide detached elements" fails for some cases, I'm totally okay with asking people to change $(elem).hide().appendTo("body") into $(elem).appendTo("body").hide() for example. We just need to explain what they should do.

It is really hard to make arguments when situation is not clear to most participants, i.e. hide() works perfectly on detached elements, you don't need to do $(elem).appendTo("body").hide(), that is not what we discussing here :/. I tried to explain this couple times, i guess i can't be more clear then that.

@timmywil timmywil modified the milestone: 3.0.0 Jan 27, 2016

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Mar 14, 2016

Member

We're going to stick with what we have now for 3.0. I see merit on both sides of this argument, but we will address any complaints as they come in. @markelog Feel free to say "nanny nanny boo boo" if we do. Behavior is locked in pending user-submitted tickets.

Member

timmywil commented Mar 14, 2016

We're going to stick with what we have now for 3.0. I see merit on both sides of this argument, but we will address any complaints as they come in. @markelog Feel free to say "nanny nanny boo boo" if we do. Behavior is locked in pending user-submitted tickets.

@timmywil timmywil closed this Mar 14, 2016

@zh99998

This comment has been minimized.

Show comment
Hide comment
@zh99998

zh99998 Jan 14, 2017

this logic broken in shadow dom.

$.contains(document, elem) will return false when elem is shown in a shadow dom.
change to elem.getRootNode({composed: true}) === elem.ownerDocument works.

but notice getRootNode is a new feature and may require polyfill.
https://github.com/foobarhq/get-root-node-polyfill
It's awful, maybe we should just access parentNode recursively.
https://github.com/foobarhq/get-root-node-polyfill/blob/master/index.js#L37-L43

var isHiddenWithinTree = function( elem, el ) {

		// isHiddenWithinTree might be called from jQuery#filter function;
		// in that case, element will be second argument
		elem = el || elem;

		// Inline style trumps all
		return elem.style.display === "none" ||
			elem.style.display === "" &&

			// Otherwise, check computed style
			// Support: Firefox <=43 - 45
			// Disconnected elements can have computed display: none, so first confirm that elem is
			// in the document.
			elem.getRootNode({composed: true}) === elem.ownerDocument &&

			jQuery.css( elem, "display" ) === "none";
	};

zh99998 commented Jan 14, 2017

this logic broken in shadow dom.

$.contains(document, elem) will return false when elem is shown in a shadow dom.
change to elem.getRootNode({composed: true}) === elem.ownerDocument works.

but notice getRootNode is a new feature and may require polyfill.
https://github.com/foobarhq/get-root-node-polyfill
It's awful, maybe we should just access parentNode recursively.
https://github.com/foobarhq/get-root-node-polyfill/blob/master/index.js#L37-L43

var isHiddenWithinTree = function( elem, el ) {

		// isHiddenWithinTree might be called from jQuery#filter function;
		// in that case, element will be second argument
		elem = el || elem;

		// Inline style trumps all
		return elem.style.display === "none" ||
			elem.style.display === "" &&

			// Otherwise, check computed style
			// Support: Firefox <=43 - 45
			// Disconnected elements can have computed display: none, so first confirm that elem is
			// in the document.
			elem.getRootNode({composed: true}) === elem.ownerDocument &&

			jQuery.css( elem, "display" ) === "none";
	};

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