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(cdk/a11y): visually hidden element affecting scrolling #24620

Merged

Conversation

crisbeto
Copy link
Member

We had a position: absolute on the visually-hidden element, but it didn't have an actual position which can cause unnecessary scrolling in some cases.

Fixes #24597.

@crisbeto crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release labels Mar 18, 2022
@crisbeto
Copy link
Member Author

Caretaker note: we should keep an eye out for any scrollbar changes in screenshot tests.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Mar 18, 2022
@crisbeto crisbeto force-pushed the 24597/visually-hidden-overflow branch from b6623c3 to 5b1699f Compare March 18, 2022 18:08
@andrewseguin andrewseguin removed the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Mar 28, 2022
@andrewseguin
Copy link
Contributor

This impacted a single screenshot but it seems to indicate an edge case where the visually hidden element is in an overflow: auto element. I think that this should be left: 1px to offset the margin: -1px, otherwise it attempts to escape the parent container.

@crisbeto
Copy link
Member Author

Overflows on the left shouldn't affect the scrollbar though.

@andrewseguin
Copy link
Contributor

Normally yes, but in RTL it will: https://stackblitz.com/edit/web-platform-5zqxay?file=index.html

Just learned this myself. The test itself was testing RTL mode

@crisbeto crisbeto force-pushed the 24597/visually-hidden-overflow branch from 5b1699f to d8e860f Compare March 31, 2022 17:20
@crisbeto
Copy link
Member Author

Ah, that makes sense. I've changed it to left: 1px.

@andrewseguin
Copy link
Contributor

andrewseguin commented Apr 1, 2022

Turns out that caused about a dozen applications to fail due to screenshot failures showing the scrollbar changing. I locally debugged them and it did seem to be making it worse.

If we go back to the original left: 0, we can fix that one test with:

@mixin a11y-visually-hidden() {
    ...
    left: 0;

    [dir=rtl] & {
      left: auto;
      right: 0;
    }
}

We had a `position: absolute` on the visually-hidden element, but it didn't have an actual position which can cause unnecessary scrolling in some cases.

Fixes angular#24597.
@crisbeto crisbeto force-pushed the 24597/visually-hidden-overflow branch from d8e860f to 82e5af6 Compare April 1, 2022 12:34
@crisbeto
Copy link
Member Author

crisbeto commented Apr 1, 2022

Changed again.

@andrewseguin andrewseguin merged commit 38946e6 into angular:master Apr 1, 2022
andrewseguin pushed a commit that referenced this pull request Apr 1, 2022
We had a `position: absolute` on the visually-hidden element, but it didn't have an actual position which can cause unnecessary scrolling in some cases.

Fixes #24597.

(cherry picked from commit 38946e6)
forsti0506 pushed a commit to forsti0506/components that referenced this pull request Apr 3, 2022
…4620)

We had a `position: absolute` on the visually-hidden element, but it didn't have an actual position which can cause unnecessary scrolling in some cases.

Fixes angular#24597.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Apr 19, 2022
This PR contains the following updates:

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

---

### Release Notes

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

### [`v13.3.3`](https://github.com/angular/components/blob/HEAD/CHANGELOG.md#&#8203;1333-tweed-table-2022-04-13)

[Compare Source](angular/components@13.3.2...13.3.3)

##### cdk

| Commit | Type | Description |
| -- | -- | -- |
| [949e3c7fbc](angular/components@949e3c7) | fix | **a11y:** visually hidden element affecting scrolling ([#&#8203;24620](angular/components#24620)) |

##### material

| Commit | Type | Description |
| -- | -- | -- |
| [1e010a3624](angular/components@1e010a3) | fix | **checkbox:** add the boolean property coercion for checked input ([#&#8203;20645](angular/components#20645)) |
| [6c65b1d703](angular/components@6c65b1d) | fix | **chips:** prevent default behavior on remove button ([#&#8203;24722](angular/components#24722)) |
| [4501b2518d](angular/components@4501b25) | fix | **datepicker:** avoid rerender when min/maxDate changes to different time on the same day ([#&#8203;24434](angular/components#24434)) |
| [aae60833eb](angular/components@aae6083) | fix | **list:** wrong order of arguments when calling custom compareWith function ([#&#8203;24743](angular/components#24743)) |
| [68c5e870bd](angular/components@68c5e87) | fix | **select:** empty space read out by VoiceOver on Chrome ([#&#8203;24741](angular/components#24741)) |
| [0d1755d566](angular/components@0d1755d) | fix | **snack-bar:** update generic types for openFromComponent ([#&#8203;24634](angular/components#24634)) |
| [b83d225b33](angular/components@b83d225) | fix | **tabs:** wrong scroll distance if selected tab is removed ([#&#8203;24118](angular/components#24118)) |
| [ca30f426a9](angular/components@ca30f42) | perf | **progress-bar:** do not run change detection if there are no `animationEnd` listeners ([#&#8203;24673](angular/components#24673)) |

#### Special Thanks

Artur Androsovych, Georgian Stan, Klemen Oslaj, Kristiyan Kostadinov, Michael Doner, Pascal Weyrich, Paul Gschwendtner, RobStrader and Zach Arend

<!-- 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/1306
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 May 2, 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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release
Projects
None yet
3 participants