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.

Sign up for GitHub

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

Jump to bottom

[CL-104] fix overlay + virtual scroll view recycling bug #6179

Merged
merged 4 commits into from
Oct 12, 2023
Merged

[CL-104] fix overlay + virtual scroll view recycling bug #6179

merged 4 commits into from
Oct 12, 2023

Conversation

willmartian
Copy link
Contributor

@willmartian willmartian commented Sep 3, 2023 •
edited
Loading

Type of change

- [x] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

This PR disables view recycling in vault-items. This prevents a bug where the overlay menu would reattach onto new elements after scrolling in a virtual scroll viewport.

While testing this change on the vault-items story in Storybook, a bug was also found with the fallback-src directive. If the fallback itself is missing, the directive will request the resource in an infinite loop. This was fixed by only trying to set the fallback image once.

Code changes

  • file.ext: Description of what was changed and why

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Sep 3, 2023
@bitwarden-bot
Copy link

bitwarden-bot commented Sep 3, 2023 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – 0f34e7ac-410d-4790-a242-e5015e739d95

New Issues

Severity Issue Source File / Package Checkmarx Insight
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 63 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 51 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 81 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/password-login.strategy.spec.ts: 206 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 63 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 51 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 81 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/password-login.strategy.spec.ts: 206 Attack Vector
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/content/notification-bar.ts: 542 Attack Vector
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/content/notification-bar.ts: 542 Attack Vector
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/content/notification-bar.ts: 536 Attack Vector
LOW Use_Of_Hardcoded_Password /libs/common/src/vault/services/cipher.service.ts: 132 Attack Vector
LOW Use_Of_Hardcoded_Password /apps/cli/src/tools/generate.command.ts: 60 Attack Vector

Fixed Issues

Severity Issue Source File / Package
HIGH Client_DOM_Code_Injection /apps/browser/src/autofill/services/collect-autofill-content.service.ts: 953
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 171
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 170
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 169
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 82
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 81
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 80
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 177
MEDIUM Client_Privacy_Violation /apps/browser/src/autofill/services/collect-autofill-content.service.spec.ts: 87
LOW Client_Password_In_Comment /apps/browser/src/autofill/services/insert-autofill-content.service.ts: 51
LOW Use_Of_Hardcoded_Password /libs/common/src/vault/services/cipher.service.ts: 998
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 21
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 20
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 19
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 18
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 17
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 16
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 13
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 21
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 20
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 19
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 18
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 17
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 16
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 13
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 21
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 20
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 19
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 18
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 17
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 16
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 13
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 60
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 48
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 74
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/password-login.strategy.spec.ts: 206
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 60
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 48
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/login.strategy.spec.ts: 74
LOW Use_Of_Hardcoded_Password /libs/common/src/auth/login-strategies/password-login.strategy.spec.ts: 206
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 21
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 20
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 19
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 18
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 17
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 16
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 13
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 21
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 20
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 19
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 18
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 17
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 16
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 13
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 32
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 36
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 33
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 28
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 27
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 26
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 25
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 24
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 23
LOW Use_Of_Hardcoded_Password /apps/browser/src/autofill/jest/autofill-mocks.ts: 22
LOW Use_Of_Hardcoded_Password

More results are available on AST platform

@willmartian
Copy link
Contributor Author

@Hinton: I talked with @jtouchstonebw and we aligned on the behavior of blocking scrolling when the menu is open.
My prior approach of autoclosing the menus when they exited the viewport did not work if the user scrolled very quickly--I think there is a race condition between the view being recycled and the CDK intersection observer firing.

@willmartian willmartian marked this pull request as ready for review September 6, 2023 18:38
@willmartian willmartian requested review from a team as code owners September 6, 2023 18:38
Hinton
Hinton previously approved these changes Sep 7, 2023
View reviewed changes
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

LGTM, tested and seems to behave as expected. It's a bit odd that you can't see the position in the scrollbar while a menu is open, but that's a side effect of how browsers works.

@willmartian willmartian requested a review from a team as a code owner September 28, 2023 19:57
@willmartian
Copy link
Contributor Author

We can't combine overlay scroll blocking with virtual scroll--they don't work well together in Chrome & Safari. 😭

menu_issue.mp4

Instead, we will disable view recycling for now as we investigate a more long term solution.

LRNcardozoWDF
LRNcardozoWDF approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@LRNcardozoWDF LRNcardozoWDF left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM!

@willmartian willmartian removed the needs-qa Marks a PR as requiring QA approval label Oct 12, 2023
@willmartian willmartian merged commit 84bafe5 into master Oct 12, 2023
59 of 62 checks passed
@willmartian willmartian deleted the ps/CL-104/overlay-virtual-scroll-bug branch October 12, 2023 14:34
BlackDex pushed a commit to BlackDex/bitwarden-clients that referenced this pull request Nov 21, 2023
@willmartian @BlackDex
[CL-104] fix overlay + virtual scroll view recycling bug (bitwarden#6179
4622dfe 
)

* close menu overlay when no longer visible

* prevent infinite loop in fallback-src directive

* block scrolling when menu is open

* disable view recycling; use reposition strategy
quexten pushed a commit to quexten/clients that referenced this pull request Feb 10, 2024
@willmartian @quexten
[CL-104] fix overlay + virtual scroll view recycling bug (bitwarden#6179
8445a8e 
)

* close menu overlay when no longer visible

* prevent infinite loop in fallback-src directive

* block scrolling when menu is open

* disable view recycling; use reposition strategy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hinton Hinton Hinton approved these changes

@LRNcardozoWDF LRNcardozoWDF LRNcardozoWDF approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@willmartian @bitwarden-bot @Hinton @LRNcardozoWDF