angular / material

Material design for AngularJS
https://material.angularjs.org/
MIT License
16.55k stars 3.39k forks source link

$mdDialog doesn't play well with forms #1190

Closed wmertens closed 9 years ago

wmertens commented 9 years ago

See http://jsfiddle.net/p02Ls1dh/3/

mdDialog would be perfect for a quick entry panel, but it sets focus on the last button by default and if you try to use <form> for handling submit it breaks.

So maybe $mdDialog should:

PS: I needed to put type=button on the cancel md-button so the form wouldn't click Cancel but Submit when typing <Enter>. Is that a bug with md-button or is that expected?

marcysutton commented 9 years ago

Focus is set in the modal for accessibility purposes, but it does make sense to make the focusable element configurable. Maybe people want to send focus to a heading with tabIndex="-1" or some other element.

submit is the default type of button, so having to include type="button" to not submit a form is expected. https://developer.mozilla.org/ru/docs/Web/HTML/Element/button (Also, space bar is the expected key when working with buttons.)

wmertens commented 9 years ago

Alright, supporting tabIndex="-1" sounds good to me, or maybe even the element with the lowest tabIndex.

As for the key, I meant using the Enter key while the focus is elsewhere in the dialog. It would obviate the need of a <form> tag. The Esc key is already captured, so Enter would be nice too. Right now it only works because the close button is getting focus on show.

wmertens commented 9 years ago

I thought about this some more and I think it would be great if angular could facilitate the HTML5 http://caniuse.com/#feat=autofocus attribute. This means that on element create, it sets focus on that element, and directives like $mdDialog should not steal focus if there's an element with autofocus set.

The directive is very simple: https://gist.github.com/mlynch/dd407b93ed288d499778 - should this be part of Angular proper? That leaves just the not-stealing-focus in $mdDialog.

wmertens commented 9 years ago

I just created https://github.com/angular/angular/issues/455 for the support in Angular proper.

gustavohenke commented 9 years ago

Beware that angular/angular is Angular 2.0 ;)

wmertens commented 9 years ago

@gustavohenke argh thanks, I was wondering why there was no other issue mentioning it :sweat_smile:. https://github.com/angular/angular.js/issues/10833 is the new one.

rschmukler commented 9 years ago

Deferring to this 0.9 while we wait to hear back from the angular team. @robertmesserle any word?

mvidailhet commented 9 years ago

In the meantime, I think it would be great to have an option to deactivate the autofocus :)

mvidailhet commented 9 years ago

After some thoughts, I think the easiest and more generic way to handle this is to broadcast an event 'scope.$broadcast('dialog-showed');' at the end of the onShow function of the dialog. After catching this event, we would be able to change the focus or do whatever we want on the dialog. What do you think ?

the line to write this broadcast: https://github.com/angular/material/blob/master/src/components/dialog/dialog.js#L441

I can do a PR with the modifications if you want.

wmertens commented 9 years ago

I would still prefer declarative focus, either by the workaround of tab order or by the autofocus attribute... Listening for events is so kludgy :-(

On Mon, Feb 16, 2015, 7:20 PM Michel Vidailhet notifications@github.com wrote:

After some thoughts, I think the easiest and more generic way to handle this is to broadcast an event 'scope.$broadcast('dialog-showed');' at the end of the onShow function of the dialog. After catching this event, we would be able to change the focus or do whatever we want on the dialog. What do you think ?

the line to write this broadcast: https://github.com/angular/material/blob/master/src/components/dialog/dialog.js#L441

I can do a PR with the modifications if you want.

— Reply to this email directly or view it on GitHub https://github.com/angular/material/issues/1190#issuecomment-74551443.

sysupbda commented 9 years ago

I am with wmertens on this. I would also prefer declarative.

Until we actually agree on the proper solution, could we maybe already all agree that it is not ok for the closeButton to steal the focus? At least each person could implement their solution until there is a "beautiful" way of setting this up. Right now it just steals away the focus and work arounds seem truly ugly. Mean-time I am just commenting out line 375 of dialog/dialog.js.. :)

Thanks for the beautiful work. I really am enjoying using md-angular.

marcysutton commented 9 years ago

There is already an option in dialog setup called focusOnOpen, which you can set to false if you want to change the behavior. We recently added a md-sidenav-focus child directive to sidenavs, sounds like it would be a great addition to dialogs and bottom sheets as well.

ThomasBurleson commented 9 years ago

Reference #2313.

sysupbda commented 9 years ago

Thank you marcysutton. Happy there is focusOnOpen and that we are looking at a md-sidenav-focus like solution. A small side note is that I could not find the focusOnOpen change in the CHANGELOG.md. Should it maybe go in there?

marcysutton commented 9 years ago

Not a bad idea. It was documented in a commit, although explicitly calling out focusOnOpen could have made it more clear. It is also documented in the Dialog Service API doc. https://github.com/angular/material/commit/0b0ed2339e1ec32e34d20daa18cadd4ef89d5f86

ThomasBurleson commented 9 years ago

We will be adding a md-autofocus directive to allow SideNav, DIalog, and Bottomsheet to specify a custom autofocus target.

ThomasBurleson commented 9 years ago

Updated with SHA https://github.com/angular/material/commit/5fc572d35557197f2f9c3b610ebbfcd066dbdff9