Bug Tracker

Ticket #3552 (closed bug: fixed)

Opened 6 years ago

Last modified 5 years ago

.wrapInner() won't work on empty elements

Reported by: dtetto Owned by: flesler
Priority: minor Milestone: 1.4
Component: core Version: 1.4a2
Keywords: wrapInner Cc:
Blocking: Blocked by:

Description

$(elem).wrapInner('<div/>') does nothing if the original element is empty.

There is some logical elbow-room here -- one could argue that the wrap shouldn't be performed if there's nothing to wrap -- but the current behavior feels like a bug to me.

Based on the syntax, the transformation is being performed on the original element, not on its children: $(elem).wrapInner('<div/>'); // wrap the inside of this element with a div vs. $(elem).contents().wrapAll('<div/>'); // wrap this elements contents (if any) with a div

It's easy to get around this:

$.fn._wrapInner = $.fn.wrapInner;

$.fn.wrapInner = function(wrapper){

if(this.contents().length) return this._wrapInner(wrapper);

else return this.append(wrapper);

}

But I'd argue it ought to be changed in the library itself unless there's reason to believe a lot of folks are relying on the current behavior.

Attachments

wrapInner.testcase.html Download (1.3 KB) - added by Marc Diethelm 6 years ago.
Testcase
core.js.r6267.diff Download (390 bytes) - added by Marc Diethelm 6 years ago.
diff: fixes bug against r6267 (core.js)
wrapInner.diff Download (566 bytes) - added by ricardobeat 6 years ago.
alternative patch (core.js r6270)

Change History

Changed 6 years ago by Marc Diethelm

Testcase

comment:1 Changed 6 years ago by Marc Diethelm

IMO wrapInner should behave consistently whether there are actual nodes to enclose or not.

I'm attaching a simple testcase (uses  http://code.jquery.com/nightlies/jquery-nightly.js).

comment:2 Changed 6 years ago by Marc Diethelm

And here's a "patch" (a diff actually) that fixes the problem. I hope it's ok style wise...

Changed 6 years ago by Marc Diethelm

diff: fixes bug against r6267 (core.js)

Changed 6 years ago by ricardobeat

alternative patch (core.js r6270)

comment:3 Changed 6 years ago by ricardobeat

Added an alternative patch with a simpler/faster check for .firstChild and append instead of html (faster). Hope it's useful.

comment:4 Changed 5 years ago by dmethvin

  • Component changed from unfilled to core

comment:5 Changed 5 years ago by john

  • Status changed from new to closed
  • Version changed from 1.2.6 to 1.4a2
  • Resolution set to fixed
  • Milestone changed from 1.3 to 1.4
Note: See TracTickets for help on using tickets.