CSS: Restore cascade-override behavior in .show #2810

gibson042 · 2016-01-11T09:46:01Z

Fixes gh-2654
Fixes gh-2308
Ref 86419b1

   raw     gz Compared to master @ fb9472c7fbf9979f48ef49aff76903ac130d0959    
  +954   +271 dist/jquery.js                                                   
  +310    +99 dist/jquery.min.js

Fixes jquerygh-2654 Fixes jquerygh-2308 Ref 86419b1

gibson042 · 2016-01-11T14:28:10Z

src/css/showHide.js

If the "force visibility" behavior is here to stay, we should consider a public jQuery.defaultDisplay( element ) interface so this function doesn't go the way of its predecessor.

That's because of the if none the block thing? So if there would be a case, when, say inline element would be added to the hidden body, user could re-define it.

I like it, i would even consider it to be a blocker for the release

Nah, such an extension could easily be 3.1.0.

To to be clear, without that hook we loosing two use-cases:

When body/html is hidden

When all specific node-types are hidden, like

li {display: none}

First might exist in, at least, worlds like SPA or iframes, there is probably more weird use-cases out there.
As of all specific elements being hidden, there might be some legit cases for very simple pages.

I think it would be good to provide some workarounds for it.

markelog · 2016-01-13T13:30:45Z

How this correlates with your table there - https://gist.github.com/scottgonzalez/5aadcd7e37cccced1239

In the meeting we discuss the possibility of documenting it. I'd say we need to tests for every value of it.
Not a blocker though, but i think we shouldn't put it under the bed

markelog · 2016-01-13T13:32:34Z

src/css/showHide.js

So setDefaultDisplay is a getter? I would suggest to write it like

values[ index ] = getDefaultDisplay( elem );

And incapsulate everything else

I suppose the clarity is worth 4 bytes. Updating.

markelog · 2016-01-13T13:35:40Z

With your

if ( display === "none" ) {
  display = "block";
}

you drained all fun from it :-), but i guess we can check out users reaction of the beta release.

markelog · 2016-01-13T16:15:43Z

test/unit/css.js

Yeah, please make it more simpler, pretty convoluted. I'd say tests is not the place where we need to be clever

Not sure what these lines supposed to do - className and style are always empty here

Copypasta. They are the same in each test of this family.

gibson042 · 2016-01-13T18:27:03Z

@markelog Updated.

timmywil · 2016-01-13T18:28:16Z

LGTM

timmywil · 2016-01-13T18:33:59Z

I'd like to land this today if possible.

gibson042 · 2016-01-13T19:03:50Z

How this correlates with your table there - https://gist.github.com/scottgonzalez/5aadcd7e37cccced1239

In the meeting we discuss the possibility of documenting it. I'd say we need to tests for every value of it.
Not a blocker though, but i think we shouldn't put it under the bed

I intentionally excluded elements like <style> and <script> that are display: none by default, but tried to include everything else. Did I miss something?

markelog · 2016-01-13T19:38:33Z

@gibson042 Oh, yeah, this table includes only non-animated elements, can't find the full one though, which includes animated elements too. I meant those more then others.

@timmywil if everyone okay with jQuery.defaultDisplay being added only in 3.1 it is okay with me

markelog · 2016-01-13T19:40:13Z

src/css/showHide.js

So this could became jQuery.defaultDisplay right?

Correct. And notably, the input is an element rather than a tag name—it allows for context-sensitivity, but doesn't incur any significant extra cost for a simple cached global treatment like ours.

timmywil · 2016-01-13T20:24:58Z

http://jsperf.com/old-vs-new-show-hide

markelog · 2016-01-14T08:47:19Z

@timmywil aside from the benchmark being incorrect (after the first run, you hide already hidden elements), the reason for speed bump, is that we are dropping the third case - all re-attached elements will have incorrect display if show()/hide() applied while they were detached, for reference -
https://bugs.jquery.com/ticket/10416
https://bugs.jquery.com/ticket/10006

Now those are legit, no question, if you feel strongly about dropping them, i think we should explicitly notice all the no longer supported use-cases, why we are dropping them and provide documentation for it too.

Gotta say, this is reason why should have brought back previous tests, without claiming them "bad", since we didn't know what we are trading for performance, they are regressions by definition.

markelog · 2016-01-14T09:20:52Z

fixed http://jsperf.com/old-vs-new-show-hide/2, but this also doesn't reflect real use-case when perf questions were started, "wikipedia" one, when elements changed their display on the first run, when defaultDisplay property wasn't set

timmywil · 2016-01-14T15:10:20Z

@markelog It's not invalid. You missed the point. It wasn't so much testing what happens when hiding elements, it was testing the codepath for hiding, which is faster. It doesn't matter whether they were already hidden or not. We were checking display regardless before. That's the reason for the speed boost.

markelog · 2016-01-14T15:18:30Z

I don't understand how it could be correct, if you test the code path that would never be taken in real application. It doesn't matter what code path would be taken and how fast it is for the bogus use-case.

We were checking display regardless before. That's the reason for the speed boost.

I don't understand this statement. And you didn't answer to my suggestion in #2810 (comment)

timmywil · 2016-01-14T15:24:57Z

The use case is not that relevant. It was testing the hide path in showHide accurately. And I can't view the tickets you linked (502 Bad Gateway).

markelog · 2016-01-14T15:27:19Z

The use case is not that relevant.

I really don't get it, how it could be "not relevant" if that code path would never be taken? How in the world does it not matter?

And I can't view the tickets you linked (502 Bad Gateway).

Oh, come on -

http://webcache.googleusercontent.com/search?q=cache:3FSB7KkOQyAJ:https://bugs.jquery.com/ticket/10006+&cd=1&hl=en&ct=clnk&gl=ru

http://webcache.googleusercontent.com/search?q=cache:puR0v9SuaoUJ:https://bugs.jquery.com/ticket/10416+&cd=1&hl=en&ct=clnk&gl=ru

timmywil · 2016-01-14T15:31:42Z

I really don't get it

It is hitting the codepath whether the elements are hidden or not. http://jsperf.com/old-vs-new-show-hide/3 Re-show the elements in teardown instead and you can see that you get the same results.

As for the links, I always forget about webcache.

timmywil · 2016-01-14T15:38:03Z

Ok, for the cases you mentioned, I think I agree that they seem rare enough. Let's discuss in chat, either in -dev or in a meeting, but do you agree they could be addressed after the beta?

timmywil · 2016-01-14T16:01:17Z

@markelog and I talked this over in irc. We probably would have been commenting here for a while. We agreed that hide is faster and while the perf demonstrates that, what I didn't realize was that show might be faster for the reasons @markelog mentioned. We're figuring out what to do about the other tickets in -dev.

timmywil · 2016-01-14T17:44:16Z

@gibson042 There's an issue with disconnected elements in FF. It looks like it's respecting the cascade even when disconnected. For instance, this test fails because FF gets a computed display of "none" instead of empty string, gets a proper default display of block and applies it.

gibson042 · 2016-01-14T17:57:47Z

Confirmed the failing tests. I'll take a look later today.

Fixes gh-2833 Ref dba93f7 Close gh-2835

gibson042 added 4 commits January 11, 2016 04:38

CSS: Restore cascade-override behavior in .show

aabf960

Fixes jquerygh-2654 Fixes jquerygh-2308 Ref 86419b1

fixup! CSS source changes

2f0703a

fixup! Effects test changes

f54a55d

fixup! Effects source changes

4de7076

jquerybot added the CLA: Valid label Jan 11, 2016

gibson042 reviewed Jan 11, 2016
View reviewed changes

markelog reviewed Jan 13, 2016
View reviewed changes

gibson042 added 3 commits January 13, 2016 13:19

fixup! Improve defaultDisplay abstraction

c909f2a

fixup! Be more explicit in test logic

e74d66d

fixup! Add test from 60f546a

2f94199

markelog reviewed Jan 13, 2016
View reviewed changes

timmywil closed this in dba93f7 Jan 13, 2016

gibson042 mentioned this pull request Jan 14, 2016

New show/hide logic fails for disconnected elements in Firefox #2833

Closed

markelog referenced this pull request Jan 15, 2016

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

fe05cf3

Fixes gh-2833 Ref dba93f7 Close gh-2835

markelog mentioned this pull request Jan 29, 2016

Consider increasing modularization of test entities #2886

Closed

markelog mentioned this pull request Mar 16, 2016

Attributes: strip/collapse whitespace for set values on selects #3002

Closed

catrope mentioned this pull request Nov 20, 2017

Cross-origin frame error popcodeorg/popcode#581

Closed

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

CSS: Restore cascade-override behavior in .show #2810

CSS: Restore cascade-override behavior in .show #2810

Uh oh!

Conversation

gibson042 commented Jan 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented Jan 13, 2016

Uh oh!

timmywil commented Jan 13, 2016

Uh oh!

timmywil commented Jan 13, 2016

Uh oh!

gibson042 commented Jan 13, 2016

Uh oh!

markelog commented Jan 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timmywil commented Jan 13, 2016

Uh oh!

markelog commented Jan 14, 2016

Uh oh!

markelog commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

markelog commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

markelog commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

timmywil commented Jan 14, 2016

Uh oh!

gibson042 commented Jan 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants