Manipulation: Restrict the tbody search to child nodes #3463

Merged
merged 5 commits into from Jan 9, 2017

Projects

None yet

5 participants

@gibson042
Member

Fixes gh-3439

   raw     gz Sizes                                                            
267269  79153 dist/jquery.js                                                   
 86863  30052 dist/jquery.min.js                                               

   raw     gz Compared to master @ bf3a43eff8682b59cec785be6003753fa4b93706    
   +59    +19 dist/jquery.js                                                   
    -1     +9 dist/jquery.min.js
gibson042 added some commits Dec 20, 2016
@gibson042 gibson042 Manipulation: Test appending table rows when there is an inner tbody 71a0a85
@gibson042 gibson042 Manipulation: Restrict the tbody search to child nodes
Fixes gh-3439
1f614ed
@gibson042 gibson042 Manipulation: Describe manipulationTarget
8627ab2
@mention-bot

@gibson042, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @timmywil and @rwaldron to be potential reviewers.

src/manipulation.js
function manipulationTarget( elem, content ) {
if ( jQuery.nodeName( elem, "table" ) &&
jQuery.nodeName( content.nodeType !== 11 ? content : content.firstChild, "tr" ) ) {
- return elem.getElementsByTagName( "tbody" )[ 0 ] || elem;
+ return jQuery.filter( "tbody", elem.childNodes )[ 0 ] || elem;
@markelog
markelog Dec 23, 2016 Member

Clever, maybe a comment though? You know I like comments :)

@gibson042
gibson042 Dec 23, 2016 Member

Hmm, what are you looking for that's not covered by the new function comment? Should I update it to something like "New rows for a table belong in its child tbody when one is present", or move it above this line, or something else?

@dmethvin
dmethvin Dec 23, 2016 Member

Actually, the code itself looks pretty clear and definitely has the benefit of being more correct. Funny that nobody thought about nested tables for this case.

@gibson042
gibson042 Dec 23, 2016 Member

It only comes up when the target doesn't have a tbody but a descendant does, or when such a descendant is in the thead, both of which I imagine are rare. And the logic itself goes waaaaaay back.

@markelog
Member

@timmywil just FYI (in case you didn't know that): you can now approve pull requests, it seems a bit better then "reaction" :)

gibson042 added some commits Dec 20, 2016
@gibson042 gibson042 Manipulation: Better describe manipulationTarget d852cf7
@gibson042 gibson042 Manipulation: Improve manipulationTarget performance by about 10x
Exercise a code path that uses querySelectorAll instead of Javascript iteration.
http://codepen.io/anon/pen/vywJjx?editors=1010
4fb61af
@dmethvin

Since this is a rooted query, won't it potentially provoke a forced layout if the table doesn't have its own ID and we have to put a temporary one there to make qSA work right? I suppose for this context it may not matter since we're already manipulating the DOM.

Could we go back to elem.gEBTN("tbody") and check whether the first result (if any) is a child of elem? That's probably a few more bytes though.

Member

I suppose for this context it may not matter since we're already manipulating the DOM.

Exactly, and this code immediately precedes said manipulation.

Could we go back to elem.gEBTN("tbody") and check whether the first result (if any) is a child of elem? That's probably a few more bytes though.

Worse, because the correct tbody is not necessarily first in DOM order. I actually tried out two flavors of breakable loop to address the situation, but both were far too big for the value. Our choice is basically between the before and after here, and of the two the new one really is better AFAICT.

@timmywil timmywil self-requested a review Jan 9, 2017
@timmywil timmywil added this to the 3.2.0 milestone Jan 9, 2017
@gibson042 gibson042 merged commit efdb8a4 into jquery:master Jan 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment