Manipulation: Detect sneaky no-content replaceWith input #2206
Conversation
|
Wow, that was harder than it would have first appeared. Is there a reason to keep the list of elements versus just a flag? |
|
I know. 😞 This is essentially implementing a second output from |
|
I looked at it for a while and couldn't figure out a better way. |
| @@ -201,7 +201,7 @@ jQuery.extend({ | |||
| return clone; | |||
| }, | |||
|
|
|||
| buildFragment: function( elems, context, scripts, selection ) { | |||
| buildFragment: function( elems, context, scripts, selection, ignored ) { | |||
There was a problem hiding this comment.
Should we make buildFragment private and change it signature, 5 arguments usually considered as bad API, even for helpers. One object as an argument?
There was a problem hiding this comment.
This is performance-critical code, I'm afraid wrapping it in an object might have a perf penalty (and it'll almost certainly be larger).
As for making it fully private, it seems like a good idea. We should do that with almost everything that's not documented IMO.
There was a problem hiding this comment.
This is performance-critical code, I'm afraid wrapping it in an object might have a perf penalty (and it'll almost certainly be larger).
Need proof for that statement
There was a problem hiding this comment.
The burden of proof is on the person that wants to introduce a change, i.e. here it's you. ;)
I just said it might introduce a perf penalty, I don't know.
There was a problem hiding this comment.
I think the bigger issue is why buildFragment is so complex that it needs that many args, and why we had to add another one. I thought for sure there would be an easier way to deal with this crazy edge case, but couldn't find a reasonable way around it. It is a pretty hot path in our code.
There was a problem hiding this comment.
In short, because of self-manipulation. .replaceWith is the worst offender, but it's basically a problem any time there's a nonempty intersection between the input and the context collection, and exacerbated by the fact that .domManip lies in between the outer method and buildFragment.
That said, the scripts parameter is probably unnecessary and safe to remove when buildFragment is made private. In fact, that's an opportunity to look at more sweeping changes... why exactly do we pluck nodes from the DOM and into a fragment in buildFragment instead of .domManip anyway? It's an operation that parseHTML has to undo!
How about #2223 |
Fixes gh-2204