fix(button-toggle): use native button and aria-pressed for button-toggle #10990

tinayuangao · 2018-04-25T00:26:03Z

No description provided.

crisbeto · 2018-04-25T12:08:46Z

src/lib/button-toggle/button-toggle.html

-         (change)="_onInputChange($event)"
-         (click)="_onInputClick($event)">
-
+<button #button class="mat-button-toggle-button"


This needs type="button", otherwise it'll submit any parent form elements when it's toggled.

crisbeto · 2018-04-25T12:09:33Z

src/lib/button-toggle/button-toggle.spec.ts

    });

+    it('should have correct aria-pressed attribute', () => {
+      expect(buttonToggleNativeElement.querySelector('button')!.getAttribute('aria-pressed'))


You can move out the buttonToggleNativeElement.querySelector('button') into a variable since it'll always point to the same element.

tinayuangao · 2018-04-25T17:42:52Z

Comments addressed. Thanks for review!

crisbeto

LGTM

jelbourn

The role on the parent element also needs to change. Right now it will use radiogroup for single-selection, which have to contain role="radio". I guess it should just be group all the time now.
Is there anything that now communicates that the group supports single or multiple selection?

jelbourn · 2018-04-25T18:16:42Z

src/lib/button-toggle/button-toggle.html

+        [id]="buttonId"
+        [attr.aria-pressed]="checked"
+        [disabled]="disabled || null"
+        [attr.name]="name"


You should be able to bind name without attr. because it is a DOM property

tinayuangao · 2018-04-25T18:57:19Z

Changed to group role.
Now we don't have a way to communicate whether it supports single or multiple selection.

jelbourn

LGTM

Just tried it out in VoiceOver and it works great.

ngbot · 2018-04-26T17:59:00Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
missing required label: "release: *"
status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

josephperrott · 2018-05-01T14:46:34Z

src/lib/button-toggle/button-toggle.ts

-  /** Unique ID for the underlying `input` element. */
-  get inputId(): string { return `${this.id}-input`; }
+  /** Unique ID for the underlying `button` element. */
+  get buttonId(): string { return `${this.id}-button`; }


This is a change to the public API and needs to be aliased and marked deprecated.

Good catch, I missed that

Thanks.

Updated and marked deprecated.

ngbot · 2018-05-10T04:01:51Z

I see that you just added the pr: merge ready label, but the following checks are still failing:
status "continuous-integration/travis-ci/pr" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

jelbourn · 2018-05-10T04:02:26Z

@tinayuangao looks like a small lint error (missing semi-colon)

jelbourn · 2018-05-29T16:28:52Z

@tinayuangao can you look at the failures on this PR?

…gle (#10990)

angular-automatic-lock-bot · 2019-09-09T03:02:04Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 25, 2018

tinayuangao requested review from jelbourn and crisbeto April 25, 2018 00:26

tinayuangao added Accessibility This issue is related to accessibility (a11y) pr: needs review labels Apr 25, 2018

crisbeto reviewed Apr 25, 2018

View reviewed changes

tinayuangao force-pushed the button-toggle-aria branch from 8eb1dd7 to d531fad Compare April 25, 2018 17:41

crisbeto approved these changes Apr 25, 2018

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs review labels Apr 25, 2018

jelbourn reviewed Apr 25, 2018

View reviewed changes

jelbourn removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 25, 2018

tinayuangao force-pushed the button-toggle-aria branch from d531fad to 747bc0f Compare April 25, 2018 18:55

jelbourn approved these changes Apr 26, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Apr 26, 2018

josephperrott reviewed May 1, 2018

View reviewed changes

josephperrott removed pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 1, 2018

tinayuangao force-pushed the button-toggle-aria branch from 747bc0f to 92b5e9d Compare May 10, 2018 02:19

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 10, 2018

jelbourn removed the action: merge The PR is ready for merge by the caretaker label May 10, 2018

tinayuangao force-pushed the button-toggle-aria branch from 92b5e9d to 2d3ea59 Compare May 10, 2018 05:15

tinayuangao force-pushed the button-toggle-aria branch from 2d3ea59 to 18bbb93 Compare June 4, 2018 18:40

fix(button-toggle): use native button and aria-pressed for button-toggle

8ffb4ef

tinayuangao force-pushed the button-toggle-aria branch from 18bbb93 to 8ffb4ef Compare June 4, 2018 20:59

jelbourn added the action: merge The PR is ready for merge by the caretaker label Jun 19, 2018

tinayuangao merged commit 1ff9cf0 into angular:master Jun 20, 2018

tinayuangao added a commit that referenced this pull request Jun 20, 2018

fix(button-toggle): use native button and aria-pressed for button-tog…

b959e18

…gle (#10990)

victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018

fix(button-toggle): use native button and aria-pressed for button-tog…

537fcfb

…gle (#10990)

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019

fix(button-toggle): use native button and aria-pressed for button-toggle #10990

fix(button-toggle): use native button and aria-pressed for button-toggle #10990

Uh oh!

Conversation

tinayuangao commented Apr 25, 2018

Uh oh!

crisbeto Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

crisbeto Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Apr 25, 2018

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn Apr 25, 2018

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Apr 25, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented Apr 26, 2018

Uh oh!

josephperrott May 1, 2018

Choose a reason for hiding this comment

Uh oh!

jelbourn May 1, 2018

Choose a reason for hiding this comment

Uh oh!

tinayuangao May 10, 2018

Choose a reason for hiding this comment

Uh oh!

ngbot bot commented May 10, 2018

Uh oh!

jelbourn commented May 10, 2018

Uh oh!

jelbourn commented May 29, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 9, 2019

Uh oh!

Uh oh!