fix(radio): radio buttons not being a tab stop and missing focus indication #1731
Conversation
googlebot
added
the
cla: yes
| @@ -239,6 +239,9 @@ export class MdRadioButton implements OnInit { | |||
| @HostBinding('class.md-radio-focused') | |||
| _isFocused: boolean; | |||
|
|
|||
| /** Tabindex for the input element. */ | |||
| @Input() tabindex: number = 0; | |||
There was a problem hiding this comment.
The property should be tabIndex
I'm confused why this wasn't working, though- focus should have gone to the native radio without having to explicitly set a tabindex. Where were you seeing the problem?
There was a problem hiding this comment.
I don't remember right now, but I was testing against the local demo app. I'm retrying now and it works fine without the tabindex, so I assume it was because I wasn't seeing the ripple. I've removed the tabindex changes from the PR.
|
|
||
| it('should make the native input element a tab stop', () => { | ||
| expect(fruitRadioNativeInputs[0].tabIndex).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Being a tab stop is something better covered by an e2e test where we can actually press the tab key, but it is good to have a unit test that asserts we're forwarding the tabIndex property.
| @@ -1,6 +1,6 @@ | |||
| <!-- TODO(jelbourn): render the radio on either side of the content --> | |||
| <!-- TODO(mtlin): Evaluate trade-offs of using native radio vs. cost of additional bindings. --> | |||
| <label [attr.for]="inputId" class="md-radio-label"> | |||
| <label [attr.for]="inputId" class="md-radio-label" [class.md-ripple-focused]="_isFocused"> | |||
There was a problem hiding this comment.
Can you add a TODO to only show this focus indicator when the focus came from a keyboard event?
crisbeto
force-pushed
the
421/radio-focus-indication
branch
from
1e5759a
to
fcb3a26
Compare
|
Updated the PR to address the comments. |
| @@ -1,6 +1,7 @@ | |||
| <!-- TODO(jelbourn): render the radio on either side of the content --> | |||
| <!-- TODO(mtlin): Evaluate trade-offs of using native radio vs. cost of additional bindings. --> | |||
| <label [attr.for]="inputId" class="md-radio-label"> | |||
| <!-- TODO(crisbeto): only show focus indication if focus came from a keyboard event --> | |||
| <label [attr.for]="inputId" class="md-radio-label" [class.md-ripple-focused]="_isFocused"> | |||
There was a problem hiding this comment.
I pulled down the PR and tested it out; I found that changing the selected radio within a group somewhat quickly (such as when using the keyboard) leaves a trailing grey circle on the radio.
(doesn't occur on master)
crisbeto
force-pushed
the
421/radio-focus-indication
branch
from
fcb3a26
to
2b704bb
Compare
…cation * Fixes the radio buttons not being tabable. * Adds focus indication to the radio buttons. Referencing angular#421.
crisbeto
force-pushed
the
421/radio-focus-indication
branch
from
2b704bb
to
084add4
Compare
|
@crisbeto what's the status on this one? |
|
Sorry, @jelbourn I missed this one in between all of the notifications. The issue is that the ripple still animates for a little after you move focus away from the radio button. It can be fixed by changing this line to I tried setting up the the Alternatively this could become easier to fix once we have the ability to know where the last focus event came from (whether it was a click or a keyboard event). |
|
Please rebase |
|
Looks like much of the setup for this and #1730 has changed @andrewseguin. I'll probably end up closing them and redoing. |
|
Sounds good, apologies for the delay in getting this merged when you sent it out |
|
Closing and will re-do based on the current APIs. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Referencing #421.