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

fix(material/radio): not checked on first click if partially visible #19505

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 2, 2020

Currently the native input inside the radio button is collapsed down to 1px by 1px and moved to the bottom of the element which causes browsers to scroll the window down, rather than change the value, on the first click when the input is partially hidden. These changes fix the issue by having the input cover the entire button and hiding it with opacity. From my testing, using opacity versus cdk-visually-hidden doesn't make a difference for screen readers.

Fixes #19397.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jun 2, 2020
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 2, 2020
@@ -5,7 +5,7 @@
<div class="mat-radio-container">
<div class="mat-radio-outer-circle"></div>
<div class="mat-radio-inner-circle"></div>
<input #input class="mat-radio-input cdk-visually-hidden" type="radio"
<input #input class="mat-radio-input" type="radio"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jelbourn I don't remember why we ended up with cdk-visually-hidden in the first place, but from my testing, screen readers treat it exactly the same as when the input is opacity: 0. Given that we have the same issue in the other input-based components (slide toggle and checkbox), would it make sense to stop using it on native inputs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK we can't rely on just opacity: 0. I looked through my notes with Google a11y folks and had this from 2018:

Accessibility tree visibility of elements with `opacity: 0` across screen readers?
- Cannot rely on this.
- Possibly could set to .001
- Probably JAWS also, potentially in IE only

Which I interpret as JAWS treating opacity: 0 as fully hidden.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that IE used to have some security restrictions if an input[type="file"] has opacity: 0. I wonder whether that's not related to your notes.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the MDC radio do here?

@@ -5,7 +5,7 @@
<div class="mat-radio-container">
<div class="mat-radio-outer-circle"></div>
<div class="mat-radio-inner-circle"></div>
<input #input class="mat-radio-input cdk-visually-hidden" type="radio"
<input #input class="mat-radio-input" type="radio"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK we can't rely on just opacity: 0. I looked through my notes with Google a11y folks and had this from 2018:

Accessibility tree visibility of elements with `opacity: 0` across screen readers?
- Cannot rely on this.
- Possibly could set to .001
- Probably JAWS also, potentially in IE only

Which I interpret as JAWS treating opacity: 0 as fully hidden.

@crisbeto
Copy link
Member Author

crisbeto commented Jun 5, 2020

MDC does the same for their radio button: opacity: 0 + position: absolute. I was testing it with NVDA and it worked fine.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish I could test JAWS, but that machine is in the office. Since MDC does it, though, I guess it doesn't really matter.

LGTM

@jelbourn jelbourn added lgtm action: merge The PR is ready for merge by the caretaker labels Jun 9, 2020
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
Currently the native `input` inside the radio button is collapsed down to 1px by 1px
and moved to the bottom of the element which causes browsers to scroll the window
down, rather than change the value, on the first click when the input is partially
hidden. These changes fix the issue by having the input cover the entire button and
hiding it with `opacity`. From my testing, using `opacity` versus `cdk-visually-hidden`
doesn't make a difference for screen readers.

Fixes angular#19397.
@crisbeto crisbeto force-pushed the 19397/radio-partially-visible branch from 1159022 to ff1564e Compare November 20, 2020 20:47
@crisbeto crisbeto changed the title fix(radio): not checked on first click if partially visible fix(material/radio): not checked on first click if partially visible Nov 20, 2020
@devversion devversion removed their request for review August 18, 2021 12:58
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 28, 2021
@andrewseguin andrewseguin merged commit f80403c into angular:master Feb 18, 2022
andrewseguin pushed a commit that referenced this pull request Feb 18, 2022
…19505)

Currently the native `input` inside the radio button is collapsed down to 1px by 1px
and moved to the bottom of the element which causes browsers to scroll the window
down, rather than change the value, on the first click when the input is partially
hidden. These changes fix the issue by having the input cover the entire button and
hiding it with `opacity`. From my testing, using `opacity` versus `cdk-visually-hidden`
doesn't make a difference for screen readers.

Fixes #19397.

(cherry picked from commit f80403c)
crisbeto added a commit to crisbeto/material2 that referenced this pull request Feb 21, 2022
Fixes a regression caused by angular#19505 where clicking directly on the `input` element will prevent the click from reaching custom `click` listeners outside the radio button.
crisbeto added a commit that referenced this pull request Feb 21, 2022
Fixes a regression caused by #19505 where clicking directly on the `input` element will prevent the click from reaching custom `click` listeners outside the radio button.

(cherry picked from commit e0b76ed)
crisbeto added a commit that referenced this pull request Feb 21, 2022
Fixes a regression caused by #19505 where clicking directly on the `input` element will prevent the click from reaching custom `click` listeners outside the radio button.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 8, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/cdk](https://github.com/angular/components) | dependencies | patch | [`13.2.3` -> `13.2.4`](https://renovatebot.com/diffs/npm/@angular%2fcdk/13.2.3/13.2.4) |
| [@angular/material](https://github.com/angular/components) | dependencies | patch | [`13.2.3` -> `13.2.4`](https://renovatebot.com/diffs/npm/@angular%2fmaterial/13.2.3/13.2.4) |

---

### Release Notes

<details>
<summary>angular/components</summary>

### [`v13.2.4`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1324-plastic-mug-2022-02-23)

[Compare Source](angular/components@13.2.3...13.2.4)

##### cdk

| Commit | Type | Description |
| -- | -- | -- |
| [74bae85bc5](angular/components@74bae85) | fix | **drag-drop:** incorrectly sorting element inside dialog with blocked scrolling ([#&#8203;14806](angular/components#14806)) |
| [81898ca5f6](angular/components@81898ca) | fix | **drag-drop:** stop pointer events on placeholder ([#&#8203;24404](angular/components#24404)) |

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [6b76469b4a](angular/components@6b76469) | fix | **autocomplete:** closing immediately when input is focused programmatically ([#&#8203;21081](angular/components#21081)) |
| [3ea76419c8](angular/components@3ea7641) | fix | **autocomplete:** use narrow value for aria-haspopup ([#&#8203;15361](angular/components#15361)) |
| [9a12eabf6b](angular/components@9a12eab) | fix | **button-toggle:** unable to override elevation and high contrast styling applied incorrectly ([#&#8203;14722](angular/components#14722)) |
| [cbd4b0ce4f](angular/components@cbd4b0c) | fix | **checkbox:** clear static aria attributes from host nodes ([#&#8203;17092](angular/components#17092)) |
| [f6eaa7c1cf](angular/components@f6eaa7c) | fix | **form-field:** use correct color for form fields in high contrast mode ([#&#8203;24422](angular/components#24422)) |
| [39d7834797](angular/components@39d7834) | fix | **radio:** clicks not propagating to wrapper elements ([#&#8203;24459](angular/components#24459)) |
| [5988b8f77b](angular/components@5988b8f) | fix | **radio:** not checked on first click if partially visible ([#&#8203;19505](angular/components#19505)) |
| [33716f124b](angular/components@33716f1) | fix | **select:** arrow highlighted state not updating in Safari ([#&#8203;15281](angular/components#15281)) |
| [fc204e4f4d](angular/components@fc204e4) | fix | **sidenav:** prevent focus from entering hidden sidenav if child element has a visibility |
| [5e41a0ad09](angular/components@5e41a0a) | fix | **tabs:** use buttons for paginator also tab-header and mdc ([#&#8203;24338](angular/components#24338)) |

##### material-experimental

| Commit | Type | Description |
| -- | -- | -- |
| [4198f5b5dc](angular/components@4198f5b) | fix | **mdc-dialog:** align change detection with non-MDC version ([#&#8203;24451](angular/components#24451)) |
| [45836f924d](angular/components@45836f9) | fix | **mdc-list:** fix typo in action-list css class ([#&#8203;24448](angular/components#24448)) |
| [7ca02495cd](angular/components@7ca0249) | fix | **mdc-list:** use body-1 rather than subtitle-1 typography for list items ([#&#8203;24417](angular/components#24417)) |
| [c9a15476e8](angular/components@c9a1547) | fix | **mdc-select:** target correct element with typography ([#&#8203;24258](angular/components#24258)) |
| [bd3f39fb15](angular/components@bd3f39f) | perf | **mdc-table:** reduce bundle size ([#&#8203;24309](angular/components#24309)) |

#### Special Thanks

Alireza Ebrahimkhani, Arthur Ming, Jeri Peier, Kristiyan Kostadinov, Miles Malerba, Paul Gschwendtner and renovate\[bot]

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1188
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(Radio): Option is not selected when clicked while only partially visible.
5 participants