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

Y.Button can render as non-submit #968

Closed
wants to merge 2 commits into from
Closed

Y.Button can render as non-submit #968

wants to merge 2 commits into from

Conversation

drjayvee
Copy link

@drjayvee drjayvee commented Jul 5, 2013

new Y.Buttons always render as submit buttons. That's bad. Especially for a ToggleButton inside of a form.
See this stackoverflow thread, where @juandopazo commented "Yeah, that's probably a bug."

From my change to HISTORY.md:
Added type ATTR to ButtonCore to enable Button nodes to be rendered with "type" attribute
The default type for <button> is submit, which is not always desired, especially for ToggleButton.

  • Button now supports submit (backwards-compatible default), button and reset
  • ToggleButton is always rendered with type="button"

This isn't done yet! IE 10 fails one test, and IE in compat mode fails even more.

Jeroen Versteeg added 2 commits July 5, 2013 10:08
@drjayvee drjayvee mentioned this pull request Jul 8, 2013
5 tasks
@ghost ghost assigned derek Jul 8, 2013
@drjayvee
Copy link
Author

As I mentioned in #973, type="button" is the most intuitive default (imo).

@onlywei
Copy link

onlywei commented Jul 25, 2013

I think I have tried to work with this issue before. I remember encountering an issue where IE would not allow me to change the type of a button after it was already rendered, but I was not able to get that to happen anymore today in IE8, 9, and 10.

@drjayvee
Copy link
Author

Maybe we should make the type attr writeOnce: 'initOnly'

@derek
Copy link
Contributor

derek commented Aug 2, 2013

Hi Jeroen,

I spent some time tonight looking into this PR, and I agree with the overall approach here. Since browsers default type to submit, we should leave that as the default in core.js, but it certainly makes sense to default type to button for Toggle Buttons. The higher up we get in the button stack, the more liberty we can take with overriding default behavior to meet expectations and convinience.

I'll dig in a little deeper into this and your other reported Button issues on Monday & Tuesday. Good stuff! And thanks for your patience while I wrapped up some other work.

@derek
Copy link
Contributor

derek commented Aug 2, 2013

Maybe we should make the type attr writeOnce: 'initOnly'

👍

@derek
Copy link
Contributor

derek commented Aug 7, 2013

Still working on this. While introducing a type ATTR could solve the problem, I'd like to start a little lower and resolve the lingering issue of not preserving the innerHTML of the button. That will give us a little clearer picture of what needs to be done here (if anything). If that were solved and the DOM node's type attribute were respected, then we might not need this workaround at all and defaulting a new Button to type=button would just be convenience sugar (possibly in Y.ButtonCore.prototype.TEMPLATE). This would also solve item 4 @ #973.

Oh, and I realized a type ATTR in Y.Button conflicts with Y.ToggleButton's type ATTR, so we'd have to address that collision. FWIW, Button is still in "beta", so breaking backwards compatibility is acceptable (though not ideal).

@derek
Copy link
Contributor

derek commented Oct 10, 2013

While the patch included in this PR will certainly address the issue of default button types, I think #1296 is a more ideal fix that addresses the root issue. Let me know if you disagree and we can revisit this pull request. Thanks again for this contribution and bringing all the button issues to my attention. As a result, the last few releases have really helped improve Button's stability and quality.

@triptych
Copy link
Contributor

Closing on behalf of @derek

@triptych triptych closed this Oct 10, 2013
@drjayvee
Copy link
Author

Hey @derek! I just wanted to let you know I agree with the changes to Button. Good work!

@derek
Copy link
Contributor

derek commented Oct 16, 2013

@drjayvee Thanks!

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

Successfully merging this pull request may close these issues.

4 participants