Skip to content

Update attr.xml #533

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
wants to merge 2 commits into from
Closed

Update attr.xml #533

wants to merge 2 commits into from

Conversation

AurelioDeRosa
Copy link
Member

Removed incorrect sentence (possibly something that was valid in older versions of jQuery?) as proved here: http://jsfiddle.net/73cKp/1/

Removed incorrect sentence (possibly something that was valid in older versions of jQuery?) as proved here: http://jsfiddle.net/73cKp/1/
@AurelioDeRosa
Copy link
Member Author

On a side note, I'm not sure of the correctness of this quote:

Note: Attempting to change the type attribute (or property) of an input element created via HTML or already in an HTML document will result in an error being thrown by Internet Explorer 6, 7, or 8.

I ran the same code on Internet Explorer 6-7, although using the F12 Developer Tools, and the attribute was changed.

@dmethvin
Copy link
Member

A fresh document.createElement("input") lets you set the attribute once in old IE since it's initially undefined. Any further attempts (i.e., changing the type attribute once set) throw an exception.

@AurelioDeRosa
Copy link
Member Author

Thank you for the heads up @dmethvin. I've been able to reproduce the issue with your hint, so the second note is valid. Any thought about the PR?

@dmethvin
Copy link
Member

Yeah, this does need rewriting since we took off the prohibition in jQuery 1.9. How about this?

Note: Attempting to change the type attribute on an input or button element will throw an exception on Internet Explorer 8 or older.

@AurelioDeRosa
Copy link
Member Author

LGTM. I also want to highlight that a similar note is present when discussing the method as a getter, which seems out of place to me. Should we remove the note in the getter section and update my PR to use your text?

@dmethvin
Copy link
Member

Yeah, the getter note doesn't belong there I think.

@AurelioDeRosa
Copy link
Member Author

Hi @dmethvin. I've updated the PR based on our discussion.

@AurelioDeRosa
Copy link
Member Author

Any news on this PR?

@gnarf
Copy link
Member

gnarf commented Dec 22, 2014

Hey @AurelioDeRosa - going through some old pulls this week and ran into this one:

Hi @dmethvin. I've updated the PR based on our discussion.

I don't see an update, this pull still shows removing the line entirely. Would you double check that you did actually update it? I'd be happy to merge this in!

@AurelioDeRosa
Copy link
Member Author

Hi @gnarf. I've updated the PR based on the discussion. I don't why it wasn't already changed, but I guess I posted the message in the wrong thread.

@arthurvr
Copy link
Member

Thanks, @AurelioDeRosa! Does this PR look good now, @gnarf @dmethvin?

@AurelioDeRosa
Copy link
Member Author

@arthurvr No objections in a month, I think you can merge this PR.

@arthurvr arthurvr closed this in 1f2d27f Mar 26, 2015
@AurelioDeRosa AurelioDeRosa deleted the patch-4 branch March 26, 2015 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants