Permalink
Browse files

CSS: Stop Firefox from treating disconnected elements as cascade-hidden

Fixes gh-2833
Ref dba93f7
Close gh-2835
  • Loading branch information...
gibson042 authored and timmywil committed Jan 14, 2016
1 parent dbc4608 commit fe05cf37ffd4795988f9b2343df2182e108728ca
Showing with 6 additions and 1 deletion.
  1. +6 −1 src/css/showHide.js
View
@@ -54,7 +54,12 @@ function showHide( elements, show ) {
elem.style.display = "";
}
}
if ( elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ) {
if ( elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
// Support: Firefox <=42 - 43
// Don't set inline display on disconnected elements with computed display: none

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 15, 2016

Member

This is done only for FF? Do we support disconnected elements or not? Since this fixes other cases as well.

If we do, than these comments are incorrect, see discussion in #2810 (comment)

@markelog

markelog Jan 15, 2016

Member

This is done only for FF? Do we support disconnected elements or not? Since this fixes other cases as well.

If we do, than these comments are incorrect, see discussion in #2810 (comment)

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 15, 2016

Member

Well, we support disconnected elements. The question is what does that support look like? I might argue that supporting disconnected elements means not putting a display on it (it has no effect until attached, and then it might be the wrong display based on the cascade). And by that argument, this makes FF behavior consistent with all other browsers, so the comments are correct.

@timmywil

timmywil Jan 15, 2016

Member

Well, we support disconnected elements. The question is what does that support look like? I might argue that supporting disconnected elements means not putting a display on it (it has no effect until attached, and then it might be the wrong display based on the cascade). And by that argument, this makes FF behavior consistent with all other browsers, so the comments are correct.

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 15, 2016

Member

Okay, let me rephrase this, in previous versions, if node was disconnected, we were setting default display, we accomplish this behaviour by use of contains(), now we call that method only to fix one edge bug in FF... to do the opposite.

Now though, it hits contains() when element is hidden, whereas before, it would reach this point when element is shown.

Which means, we worsen the perf for the case when element supposed to be shown, by improving the case when element is already shown a.k.a. incorrect use-case.

So why do we want to change the logic on which we never receive any complaints in three years by worsen the perf for the case for which the show method was written?

@markelog

markelog Jan 15, 2016

Member

Okay, let me rephrase this, in previous versions, if node was disconnected, we were setting default display, we accomplish this behaviour by use of contains(), now we call that method only to fix one edge bug in FF... to do the opposite.

Now though, it hits contains() when element is hidden, whereas before, it would reach this point when element is shown.

Which means, we worsen the perf for the case when element supposed to be shown, by improving the case when element is already shown a.k.a. incorrect use-case.

So why do we want to change the logic on which we never receive any complaints in three years by worsen the perf for the case for which the show method was written?

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Jan 15, 2016

Member

This will probably break some stuff related to show/hide on detached elements but it has been sorta broken multiple times in the past so if the behavior changes for that case well ¯_(ツ)_/¯ it's a 3.0 release. People might need to change $elem.hide().appendTo() to $elem.appendTo().hide() to get it to work?

I think the main point @markelog makes is that we're defining our behavior this way primarily to get around a Firefox bug, right? So, this line isn't trying to work around a FF bug, we changed our behavior on detached elements this way so that a FF bug wouldn't come in to play. At least that's the way I understood it.

@dmethvin

dmethvin Jan 15, 2016

Member

This will probably break some stuff related to show/hide on detached elements but it has been sorta broken multiple times in the past so if the behavior changes for that case well ¯_(ツ)_/¯ it's a 3.0 release. People might need to change $elem.hide().appendTo() to $elem.appendTo().hide() to get it to work?

I think the main point @markelog makes is that we're defining our behavior this way primarily to get around a Firefox bug, right? So, this line isn't trying to work around a FF bug, we changed our behavior on detached elements this way so that a FF bug wouldn't come in to play. At least that's the way I understood it.

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 16, 2016

Member

I'm not sure I follow this conversation thread, so let me explain the change directly. Our desire per gh-2654 is for .show() to override an empty or missing inline display when necessary to force an element out of (cascaded) display: none. Firefox has the inconvenient behavior of reporting a computed display value of "none" for disconnected elements, which logically don't have a cascaded value because there's nothing from which to cascade. In order to circumvent that bogus "none" and operate only upon real display: none elements, we strengthen the test to include document attachment.

So the line of code is specific to Firefox, because other browsers don't fake a display: none for disconnected elements.

@gibson042

gibson042 Jan 16, 2016

Member

I'm not sure I follow this conversation thread, so let me explain the change directly. Our desire per gh-2654 is for .show() to override an empty or missing inline display when necessary to force an element out of (cascaded) display: none. Firefox has the inconvenient behavior of reporting a computed display value of "none" for disconnected elements, which logically don't have a cascaded value because there's nothing from which to cascade. In order to circumvent that bogus "none" and operate only upon real display: none elements, we strengthen the test to include document attachment.

So the line of code is specific to Firefox, because other browsers don't fake a display: none for disconnected elements.

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 16, 2016

Member

@dmethvin well yeah, those are tickets i mentioned in that thread. I fixed those cases. I believe when we released fixes for them, logic was solid for detached elements.

But know we changed that behaviour, i assume because of the performance reasons (right @gibson042?) by not using the contains() method.

But now, after this commit, we again using the contains() method, we now worsen the performance of the show() method for the case when element is hidden by improving performance for the case when element is shown. Which looks as upside down.

@gibson042
yes, that what are discussing now, forget about the thread there is no thread :-). Just try to understand the fe05cf3#commitcomment-15480663 comment.

@timmywil

I might argue that supporting disconnected elements means not putting a display on it

We even falling that actually, if element is disconnected, but hide() is called, then we apply the display.

My opinion when i introduced support for detached elements three years ago, is that, if element is detached and user calls either hide() or show() on it, they know what they doing, they expect for element to be shown or hidden when they attach those elements to some context.

And that assumption worked well, we didn't ever had complains about it.

But now we expect, that only those users that apply hide() to disconnected elements know what they are doing, not those ppl who apply show()... and we worsen the performance... and size btw.

Granted contains() shouldn't reflect the performance that much, because jQuery.css() display already takes a lot of time, but if it doesn't matter, then again, why?

I hope my explanations is now more clear, i know it is hard to understand, no kidding, especially with my phrasing, but please try to grasp it.

@markelog

markelog Jan 16, 2016

Member

@dmethvin well yeah, those are tickets i mentioned in that thread. I fixed those cases. I believe when we released fixes for them, logic was solid for detached elements.

But know we changed that behaviour, i assume because of the performance reasons (right @gibson042?) by not using the contains() method.

But now, after this commit, we again using the contains() method, we now worsen the performance of the show() method for the case when element is hidden by improving performance for the case when element is shown. Which looks as upside down.

@gibson042
yes, that what are discussing now, forget about the thread there is no thread :-). Just try to understand the fe05cf3#commitcomment-15480663 comment.

@timmywil

I might argue that supporting disconnected elements means not putting a display on it

We even falling that actually, if element is disconnected, but hide() is called, then we apply the display.

My opinion when i introduced support for detached elements three years ago, is that, if element is detached and user calls either hide() or show() on it, they know what they doing, they expect for element to be shown or hidden when they attach those elements to some context.

And that assumption worked well, we didn't ever had complains about it.

But now we expect, that only those users that apply hide() to disconnected elements know what they are doing, not those ppl who apply show()... and we worsen the performance... and size btw.

Granted contains() shouldn't reflect the performance that much, because jQuery.css() display already takes a lot of time, but if it doesn't matter, then again, why?

I hope my explanations is now more clear, i know it is hard to understand, no kidding, especially with my phrasing, but please try to grasp it.

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Jan 16, 2016

Member

Compared to 2.1.2 (and assuming equal-cost defaultDisplay, which is usually true),

this .show() is

no inline display inline display: none other inline display
cascade-visible less work (no contains) unchanged unchanged
cascade-hidden more work (contains) unchanged unchanged
detached more work in Firefox (contains), less elsewhere (no contains) more work in Firefox (contains), less elsewhere (no contains) unchanged

(but doesn't get the performance benefits of permanently locking in default display)

and this .hide() is

no inline display inline display: none other inline display
cascade-visible much less work (no getComputedStyle or contains) less work (no getComputedStyle) much less work (no getComputedStyle or contains)
cascade-hidden less work (no getComputedStyle) less work (no getComputedStyle) much less work (no getComputedStyle or contains)
detached much less work (no getComputedStyle or non-Firefox contains) much less work (no getComputedStyle or non-Firefox contains) much less work (no getComputedStyle or non-Firefox contains)

(but doesn't get the performance benefits of permanently locking in computed display).

All in all, I don't think I can hope for anything better. But it seems like you disagree, @markelog... what would you like to see answered or changed here?

@gibson042

gibson042 Jan 16, 2016

Member

Compared to 2.1.2 (and assuming equal-cost defaultDisplay, which is usually true),

this .show() is

no inline display inline display: none other inline display
cascade-visible less work (no contains) unchanged unchanged
cascade-hidden more work (contains) unchanged unchanged
detached more work in Firefox (contains), less elsewhere (no contains) more work in Firefox (contains), less elsewhere (no contains) unchanged

(but doesn't get the performance benefits of permanently locking in default display)

and this .hide() is

no inline display inline display: none other inline display
cascade-visible much less work (no getComputedStyle or contains) less work (no getComputedStyle) much less work (no getComputedStyle or contains)
cascade-hidden less work (no getComputedStyle) less work (no getComputedStyle) much less work (no getComputedStyle or contains)
detached much less work (no getComputedStyle or non-Firefox contains) much less work (no getComputedStyle or non-Firefox contains) much less work (no getComputedStyle or non-Firefox contains)

(but doesn't get the performance benefits of permanently locking in computed display).

All in all, I don't think I can hope for anything better. But it seems like you disagree, @markelog... what would you like to see answered or changed here?

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 20, 2016

Member

So in order for us to be on the same page - let's consider the line in 3.0 -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
 jQuery.contains( elem.ownerDocument, elem)

And line in 2.x -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)

And check out what actually happens in alpha and 2.x for the case like this -

...
<style>.hidden {display: none}</style>
...
<div class="hidden"></div>
<script>
$(".hidden").show();
</script>
...

In 3.0, first of all we hit this -

elem.style.display === "" &&

No inline display, true, because of the && operator, we move on to -

jQuery.css( elem, "display" ) === "none" &&

Have computed display, but we go on, because of, again, && operator and hit -

jQuery.contains( elem.ownerDocument, elem)

Whereas before, in 2.x, we had this -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)

So third condition - !jQuery.contains( elem.ownerDocument, elem) wasn't executed because of the || operator.

Now if call show() for the case like this -

...
<div></div>
<script>
$("div").show();
</script>
...

with new alpha line -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" && 
jQuery.contains( elem.ownerDocument, elem)

We actually do not execute jQuery.contains( elem.ownerDocument, elem), because we bail on jQuery.css( elem, "display" ) === "none".

Whereas before, in 2.x, for the line -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" || 
!jQuery.contains( elem.ownerDocument, elem)

We actually still go to the contains(). Which means case of show() call for hidden element is now more expensive, whereas case for shown element is less expensive.

Sorry to be so verbose and repetitive, i would like to everyone get what are we talking about, if someone wasn't, that is.

In @gibson042 table, that shown in

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

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.

That is the first point. Now the second one - @timmywil writes -

I might argue that supporting disconnected elements means not putting a display on it (it has no effect until attached, and then it might be the wrong display based on the cascade).

Which is not true, because in 3.0 we have -

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

// But for the case
/*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 believe previous behaviour was correct and expected, since we never seen complaints about it.
So to answer your question @gibson042

what would you like to see answered or changed here?

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.

@markelog

markelog Jan 20, 2016

Member

So in order for us to be on the same page - let's consider the line in 3.0 -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" &&
 jQuery.contains( elem.ownerDocument, elem)

And line in 2.x -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)

And check out what actually happens in alpha and 2.x for the case like this -

...
<style>.hidden {display: none}</style>
...
<div class="hidden"></div>
<script>
$(".hidden").show();
</script>
...

In 3.0, first of all we hit this -

elem.style.display === "" &&

No inline display, true, because of the && operator, we move on to -

jQuery.css( elem, "display" ) === "none" &&

Have computed display, but we go on, because of, again, && operator and hit -

jQuery.contains( elem.ownerDocument, elem)

Whereas before, in 2.x, we had this -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" ||
 !jQuery.contains( elem.ownerDocument, elem)

So third condition - !jQuery.contains( elem.ownerDocument, elem) wasn't executed because of the || operator.

Now if call show() for the case like this -

...
<div></div>
<script>
$("div").show();
</script>
...

with new alpha line -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" && 
jQuery.contains( elem.ownerDocument, elem)

We actually do not execute jQuery.contains( elem.ownerDocument, elem), because we bail on jQuery.css( elem, "display" ) === "none".

Whereas before, in 2.x, for the line -

elem.style.display === "" && jQuery.css( elem, "display" ) === "none" || 
!jQuery.contains( elem.ownerDocument, elem)

We actually still go to the contains(). Which means case of show() call for hidden element is now more expensive, whereas case for shown element is less expensive.

Sorry to be so verbose and repetitive, i would like to everyone get what are we talking about, if someone wasn't, that is.

In @gibson042 table, that shown in

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

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.

That is the first point. Now the second one - @timmywil writes -

I might argue that supporting disconnected elements means not putting a display on it (it has no effect until attached, and then it might be the wrong display based on the cascade).

Which is not true, because in 3.0 we have -

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

// But for the case
/*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 believe previous behaviour was correct and expected, since we never seen complaints about it.
So to answer your question @gibson042

what would you like to see answered or changed here?

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.

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Jan 20, 2016

Member

Created ticket for this #2854, so we would stop discussing it in commit

@markelog

markelog Jan 20, 2016

Member

Created ticket for this #2854, so we would stop discussing it in commit

jQuery.contains( elem.ownerDocument, elem ) ) {

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jan 14, 2016

Member

One of the things to consider in #2404.

@mgol

mgol Jan 14, 2016

Member

One of the things to consider in #2404.

values[ index ] = getDefaultDisplay( elem );
}
} else {

0 comments on commit fe05cf3

Please sign in to comment.