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

fix(jqLite): make jqLite('<iframe src="someurl">').contents() return iframe document, as in jQuery #6323

Closed
wants to merge 1 commit into from

Conversation

caitp
Copy link
Contributor

@caitp caitp commented Feb 18, 2014

This is a very tiny change to make behaviour consistent with jQuery.

Closes #6320

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#6323)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@caitp
Copy link
Contributor Author

caitp commented Feb 18, 2014

This might fail on IE8 or something because of the way the test is written, I am curious to see if it passes =)

@caitp
Copy link
Contributor Author

caitp commented Feb 18, 2014

Hmm, it does fail in IE8 and IE9 :( garr

@caitp
Copy link
Contributor Author

caitp commented Feb 18, 2014

Well my vote is to check this in after support for IE8 is dropped. In the mean time, folks should continue to use jQuery when they need this functionality

@IgorMinar IgorMinar self-assigned this Feb 18, 2014
iframe = jqLite(iframe_);
function test() {
var contents = iframe.contents();
expect(contents[0]).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, don't use toBeTruthy() and toBeFalsy() matchers, there are always better more constrained matchers available. in this case maybe using toBeDefined() would be the best.

@IgorMinar
Copy link
Contributor

please apply the small changes requested, disable the test on ie8 and merge. thanks!

@IgorMinar IgorMinar added this to the 1.2.14 milestone Feb 18, 2014
…iframe document, as in jQuery

This is a very tiny change to make behaviour consistent with jQuery.

Closes angular#6320
@caitp caitp closed this in 05fbed5 Feb 18, 2014
@caitp
Copy link
Contributor Author

caitp commented Feb 18, 2014

Made the changes, added some extra wait time due to flakiness, and merged. If it needs a revert later on, I won't be mad (passing build: https://travis-ci.org/caitp/angular.js/builds/19149636)

khepin pushed a commit to khepin/angular.js that referenced this pull request Feb 19, 2014
…iframe document, as in jQuery

This is a very tiny change to make behaviour consistent with jQuery.

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

Successfully merging this pull request may close these issues.

Bug: angular.contents() not working with iframes
3 participants