Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

closed.bs.alert fires before remove() #12379

Closed
EnsignR opened this issue Jan 25, 2014 · 11 comments
Closed

closed.bs.alert fires before remove() #12379

EnsignR opened this issue Jan 25, 2014 · 11 comments
Labels
Milestone

Comments

@EnsignR
Copy link
Contributor

EnsignR commented Jan 25, 2014

As mentioned in #10199 the event closed.bs.alert fires before elements are removed from the DOM.

I have made a jsfiddle to illustrate this here http://jsfiddle.net/ensignr/NFrFc/

  1. 3x div.alert are created in a div#cont
  2. event listener for 'close.bs.alert' and 'closed.bs.alert' attached to div.alert's
  3. the event listener counts the number of elements in #cont and reports them
  4. it's expected for the coupled close/closed events the count should be one less for closed. This is not the case.
  5. the count can also be checked by clicking the div.out, which shows the count is actually reduced if you click after closed event fires.

I believe the problem lies in Alert.removeElement() in /js/alert.js:45
Here remove() is called after the trigger() in the chain.
The following change, where I detach() first, seem to fix things.

function removeElement() {
      $parent.detach().trigger('closed.bs.alert')
    }

I managed to quickly patch the 3.0.3 minified version bootrap.min.js which seems to work. You can see it here http://jsfiddle.net/ensignr/6VjPP/ (for as long as I keep the modified code hosted)

I've tried both the patched and non patched jsfiddles (as well as the project that lead me to discover this) in both Chrome 32.0.1700.76 and IE11 on Win7.

[Apols if I've missed/messed up the markup or something else, but I've had to join all the things to submit this bug report. It's been quite an effort :) ]

@cvrebert cvrebert modified the milestones: v3.2.0, v3.1.1 Feb 9, 2014
@cvrebert cvrebert assigned cvrebert and unassigned cvrebert Apr 20, 2014
@cvrebert
Copy link
Collaborator

@fat What do you think?

@EnsignR
Copy link
Contributor Author

EnsignR commented Apr 23, 2014

FYI I've been using a modified .min.js on something live for a while with the above change. Have not seen any undesired effects.

@cvrebert
Copy link
Collaborator

@EnsignR I think a pull request implementing this change would be welcome.

@EnsignR
Copy link
Contributor Author

EnsignR commented Apr 23, 2014

I did fork and edit, but I couldn't figure out how to create a pull req. for just that one change (rather than include some I also made to dropdown.js as well). :\

@fat
Copy link
Member

fat commented Apr 25, 2014

first reaction is, using detach doesn't clean up the memory… and jquery keeps all the data, etc around on that object, so it's not quite ideal

@fat
Copy link
Member

fat commented Apr 25, 2014

that said, i could see wanting to wait for the element to be removed first

@fat
Copy link
Member

fat commented Apr 25, 2014

we could do

jQuery.cleanData($parent.detach().trigger('closed.bs.alert'))

even though cleanData is undocumented… and could change/disappear…

@EnsignR
Copy link
Contributor Author

EnsignR commented Apr 26, 2014

.empty() calls jQuery.cleanData.and is documented.
Therefore what about the following?

$parent.detach().trigger('closed.bs.alert').empty()

@fat
Copy link
Member

fat commented Apr 26, 2014

actually we can just do $parent.detach().trigger('closed.bs.alert').remove()… and maybe leave a note about why that looks weird.

empty isn't quite right because it wont call cleanData on the parent i dont think

EnsignR added a commit to EnsignR/bootstrap that referenced this issue Apr 30, 2014
Added call to remove() after event fires to clean up data as per discussion at twbs#12379
cvrebert added a commit that referenced this issue May 6, 2014
fires closed.bs.alert *after* DOM detach() as per #12379
@fat
Copy link
Member

fat commented May 13, 2014

fixed

@fat fat closed this as completed May 13, 2014
@mdo mdo modified the milestones: v3.2.1, v3.2.0 May 13, 2014
@mix3d
Copy link

mix3d commented Dec 8, 2016

Commenting because this fix ended up breaking the ability for a parent view to listen to the alert object. The core of the problem I think is the "fires after element is removed from DOM" directive; which conflicts with the desire to listen for the event in the same manner that one can listen to tooltips, modals, etc, where the owner of the eventHandler is not the Bootstrap object in question.

If the answer is just "works as designed, work around that problem", that is fine, but I wanted to check if there might be a solution first.

I made an example is with Backbone/Marionette, but could be anything where the alert is a child of a view listening for the bootstrap events.

http://codepen.io/mix3d/full/LbreBN/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants