Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(removePatch): fire $destroy event when full jQuery is used #1512

Closed
wants to merge 2 commits into from

Conversation

PeteAppleton
Copy link

We found an issue where jQuery 1.8.2 is used where form controls created by ngRepeat are never removed from the parent form because the $destroy custom event is not fired. As a result, if a control was invalid (due to the model binding) and then destroyed the form's validity state became out-of-sync with the model and impossible to restore. This patch corrects the issue.

The previous code relied upon internal jQuery structures which are not
present in v.1.8.2 (current). This code uses the public API to fire the
$destroy custom event, and so should work with any post-1.4 jQuery
version.

PeteAppleton and others added 2 commits October 30, 2012 13:41
The previous code relied upon internal jQuery structures which are not
present in v.1.8.2 (current).  This code uses the public API to fire the
$destroy custom event, and so should work with any post-1.4 jQuery
version.
@IgorMinar
Copy link
Contributor

is there any failing test for this issue? if not can you please write one in jqLiteSpec.js

@IgorMinar
Copy link
Contributor

I see that our tests are failing when we run them against jquery 1.8.2

@IgorMinar
Copy link
Contributor

I like the use of triggerHandler, that's what we need. but in order to be able to merge this we need to add triggerHandler to jqLite. can you implement it please?

@IgorMinar
Copy link
Contributor

I added jquery 1.8.2 to our repo in this branch: https://github.com/IgorMinar/angular.js/tree/jquery-1.8.2

can you rebase your branch on top of it?

@IgorMinar
Copy link
Contributor

for instructions on how to run our tests please check out: docs.angularjs.org/misc/contribute

@PeteAppleton
Copy link
Author

Sure, no problem - it'll probably be tomorrow now as it's getting late over here now

@PeteAppleton
Copy link
Author

More likely to be the weekend in reality, sorry.

@PeteAppleton
Copy link
Author

OK, I'm clearly stupid as I'm unable to work out how to rebase my repository. Given my git config pasted below, could you please give me a pointer as to what I need to do to achieve the rebase? My unsuccessful attempts consisted of

git rebase "https://github.com/IgorMinar/angular.js.git",
git rebase "https://github.com/IgorMinar/angular.js.git" "jquery-1.8.2" and
git rebase "git://github.com/IgorMinar/angular.js.git" "jquery-1.8.2"

all of which give errors along the lines of
fatal: Needed a single revision
invalid upstream https://github.com/IgorMinar/angular.js.git

git config output:

[core]
repositoryformatversion = 0
filemode = true
bare = false
logallrefupdates = true
[remote "origin"]
url = https://github.com/PeteAppleton/angular.js
fetch = +refs/heads/:refs/remotes/origin/
[branch "master"]
remote = origin
merge = refs/heads/master
[remote "upstream"]
url = https://github.com/angular/angular.js.git
fetch = +refs/heads/:refs/remotes/upstream/

@IgorMinar
Copy link
Contributor

git remote add IgorMinar https://github.com/IgorMinar/angular.js.git
git pull IgorMinar
git rebase IgorMinar/jquery-1.8.2

IgorMinar added a commit to IgorMinar/angular.js that referenced this pull request Nov 24, 2012
in jQuery 1.8.x the data() data structure is changed and events are
not accessible via data().events. Since all we need is to trigger
all event handlers, we can do so via triggerHandler() api instead of
mocking with the internal jQuery data structures.

This fix was originally proposed by PeteAppleton via PR angular#1512.

Closes angular#1512
@IgorMinar IgorMinar closed this in b9a9f91 Nov 26, 2012
IgorMinar added a commit that referenced this pull request Nov 26, 2012
in jQuery 1.8.x the data() data structure is changed and events are
not accessible via data().events. Since all we need is to trigger
all event handlers, we can do so via triggerHandler() api instead of
mocking with the internal jQuery data structures.

This fix was originally proposed by PeteAppleton via PR #1512.

Closes #1512
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants