[PM-13388] Extension: Persist Scroll from Vault #12325
Conversation
- Allows query selector to focus on the specific element
|
Great job, no security vulnerabilities found in this Pull Request |
…t/pm-13888/persist-scroll-from-vault
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12325 +/- ##
==========================================
+ Coverage 35.01% 35.22% +0.21%
==========================================
Files 2973 2993 +20
Lines 90457 90716 +259
Branches 16955 16974 +19
==========================================
+ Hits 31670 31959 +289
+ Misses 56330 56289 -41
- Partials 2457 2468 +11 ☔ View full report in Codecov by Sentry. |
|
@nick-livefront this looks great! One nit, it is a bit odd that the focus ring flashes on search and then moves down to the item. It feels like a bug. When navigating with a screen reader on, the UX isn't terrible but I can hear the SR trying to read the first few syllables of "search" before focus is moved on. Is there a way for us to keep auto-focus on search just when initially opening the popup so we don't have the flash of the search focus in this flow? |
@danielleflinn The focus only moves when navigating back from a cipher view screen. Which I agree, not ideal. Initially opening the extension or navigating from another page the focus stays on the search field. Do you want to remove the focus switch when navigating back from the view page? Screen.Recording.2024-12-11.at.11.29.06.AM.mov |
nick-livefront
requested review from
Jingo88,
shane-melton,
gbubemismith and
danielleflinn
|
@nick-livefront yes, let's go ahead and remove the focus redirect, so after returning to the Vault view in this flow focus will remain on "Search". This matches the behavior of the legacy extension. I'll update the jira to reflect the scope change. Thanks for the discussion and flexibility! |
…t/pm-13888/persist-scroll-from-vault
| @@ -85,10 +88,18 @@ export class VaultV2Component implements OnInit, OnDestroy { | |||
|
|
|||
| protected VaultStateEnum = VaultState; | |||
|
|
|||
| private allFilters$ = combineLatest([ | |||
There was a problem hiding this comment.
⛏️ (non-blocking): We could probably just expose / re-use the allFilters$ we defined on the filter service.
There was a problem hiding this comment.
I'll raise a separate PR to go back and fix the filter component once this merges in
| // Use `setTimeout` to scroll after rendering is complete | ||
| setTimeout(async () => { | ||
| // `?? 0` is only to make typescript happy. It shouldn't happen with the above truthy check for `this.scrollPosition`. | ||
| virtualScrollElement.scrollTo({ top: this.scrollPosition ?? 0, behavior: "instant" }); |
There was a problem hiding this comment.
⛏️ (non-blocking): We can also use the non-null assertion operator (!) and avoid the ?? 0 and explanatory comment.
| virtualScrollElement.scrollTo({ top: this.scrollPosition ?? 0, behavior: "instant" }); | |
| virtualScrollElement.scrollTo({ top: this.scrollPosition!, behavior: "instant" }); |
nick-livefront
dismissed stale reviews from shane-melton and cd-bitwarden
via
bf707cd
|
@cd-bitwarden @shane-melton Fixed Shane's two nits and conflicts from main, re-requesting your review! |
…t/pm-13888/persist-scroll-from-vault
…t/pm-13888/persist-scroll-from-vault
|
@bitwarden/team-vault-dev For large vaults there was a "jump" occurring for the user, this starts occurring at around 500+ ciphers. I tried a couple different options of trying to solve for the jump but the straight forward approach felt the most reasonable. Simply hiding the scroll element until after the scroll takes place.
|
I think this solution is nice. Thanks for pointing that out. I think you should also add a comment for why we did this. |
…t/pm-13888/persist-scroll-from-vault
@gbubemismith Yes! I added two comments, one at the implementation and at the source |
| @@ -130,7 +130,7 @@ export class VaultV2Component implements OnInit, AfterViewInit, OnDestroy { | |||
| ngAfterViewInit(): void { | |||
| if (this.virtualScrollElement) { | |||
| // Hide the virtual scroll element briefly before the `vaultScrollPositionService` starts. | |||
| // This avoids having the scrolling element "jump" visually for the user and | |||
| // This avoids having the scrolling element "jump" visually for the user when they have larger vaults. | |||
…t/pm-13888/persist-scroll-from-vault
🎟️ Tracking
PM-13388
📔 Objective
Persist the scroll position on the vault screen within the extension.
CdkVirtualScrollableElementis stored withinVaultPopupScrollPositionServicesetTimeoutto ensure that the next render cycle is hit.Navigation events are tracked to determine which cipher was viewed. The service then uses an addeddata-idattribute to focus on the given cipher that was viewed.Because the items are virtualized this needs the scroll to be complete in order for the item to be in the DOM.❓ I added a method to track the scroll position on an interval. In my testing anothersetTimeoutwould work because the page is "instantly" scrolled but it felt less intentional. I'm open to thoughts though!/tabs/generator📸 Screenshots
preserve-scroll.mov
reset-for-other-tabs.mov
delete-reset-scroll.mov
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes