#12915 propHook for src property #1035

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
@aFarkas
Contributor

aFarkas commented Nov 18, 2012

Here is a simple fix for issue #12915

@dmethvin dmethvin closed this in ca5e06a Nov 19, 2012

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Nov 19, 2012

Owner

This is not correct. Neither the src property nor the href property should return an absolute URL. They should return unaltered strings, which is why we used Microsoft's proprietary second argument. Should be reverted.

Owner

timmywil commented Nov 19, 2012

This is not correct. Neither the src property nor the href property should return an absolute URL. They should return unaltered strings, which is why we used Microsoft's proprietary second argument. Should be reverted.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Nov 19, 2012

Owner

I take that back. I'm thinking attributes when this is about properties.

Owner

timmywil commented Nov 19, 2012

I take that back. I'm thinking attributes when this is about properties.

@@ -94,6 +94,9 @@ test( "attr(String)", function() {
equal( jQuery("#tAnchor5").attr("href"), "#5", "Check for non-absolute href (an anchor)" );
jQuery("<a id='tAnchor6' href='#5' />").appendTo("#qunit-fixture");
equal( jQuery("#tAnchor5").prop("href"), jQuery("#tAnchor6").prop("href"), "Check for absolute href prop on an anchor" );
+
+ $('<script type="jquery/test" src="#5" id="scriptSrc"></script>').appendTo('#qunit-fixture');

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Nov 20, 2012

Member

This test couldn't have been run, let alone pass—$ is undefined.

@rwaldron

rwaldron Nov 20, 2012

Member

This test couldn't have been run, let alone pass—$ is undefined.

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Nov 20, 2012

Member

Also, the quotes need to be flipped around, double on the outside and single on the inside

@rwaldron

rwaldron Nov 20, 2012

Member

Also, the quotes need to be flipped around, double on the outside and single on the inside

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 20, 2012

Member

JSHint checks for double quotes. Please make sure to run grunt before submitting pull requests.

@mikesherov

mikesherov Nov 20, 2012

Member

JSHint checks for double quotes. Please make sure to run grunt before submitting pull requests.

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Nov 20, 2012

Owner

I landed this in ca5e06a, with the quotes flipped around ... not sure why $ is defined now but I missed that!

@dmethvin

dmethvin Nov 20, 2012

Owner

I landed this in ca5e06a, with the quotes flipped around ... not sure why $ is defined now but I missed that!

This comment has been minimized.

Show comment Hide comment
@jaubourg

jaubourg Nov 20, 2012

Member

Because we use the file resulting from the build process now? ;)

@jaubourg

jaubourg Nov 20, 2012

Member

Because we use the file resulting from the build process now? ;)

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Nov 20, 2012

Owner

I'm getting my repos all mixed up now, I have a jQuery.noConflict() in the jquery-compat plugin tests. 🙈

@dmethvin

dmethvin Nov 20, 2012

Owner

I'm getting my repos all mixed up now, I have a jQuery.noConflict() in the jquery-compat plugin tests. 🙈

This comment has been minimized.

Show comment Hide comment
@aFarkas

aFarkas Nov 20, 2012

Contributor

I did indeed run the test outside of the jQuery test suite. Because haven't installed all grunt plugins and was eager to make this pr. Should have known better. If something can go wrong, it goes wrong.

@aFarkas

aFarkas Nov 20, 2012

Contributor

I did indeed run the test outside of the jQuery test suite. Because haven't installed all grunt plugins and was eager to make this pr. Should have known better. If something can go wrong, it goes wrong.

mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment