Skip to content

Keep scroll position when dismissing a notification#3200

Open
jancborchardt wants to merge 1 commit into
masterfrom
fix/dismiss-scroll-jump
Open

Keep scroll position when dismissing a notification#3200
jancborchardt wants to merge 1 commit into
masterfrom
fix/dismiss-scroll-jump

Conversation

@jancborchardt

Copy link
Copy Markdown
Member

When clicking the Dismiss button on a notification, the list jumps to the top. This is especially annoying with a long list of notifications.
There should be no jump, only the nice short animation when dismissing the notification.

Seemingly regression from #2959

Before: jumps to top on dismissing notification

Screencast.From.2026-07-15.18-23-54.webm

After: no jump

Screencast.From.2026-07-15.18-19-55.webm

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@Antreesy

Antreesy commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

A bit hacky
Maybe you can try asking agent to read nextcloud/vue library, if that can be an upstream fix instead?

If I'm not mistaken, focus-trap should keep somewhere in memory the list of all tabbable nodes, and correctly handle it on losing one

@jancborchardt
jancborchardt force-pushed the fix/dismiss-scroll-jump branch from c44cb95 to d3e71c4 Compare July 22, 2026 12:20
Assisted-by: ClaudeCode:claude-opus-4-8
Signed-off-by: Jan C. Borchardt <925062+jancborchardt@users.noreply.github.com>
@jancborchardt
jancborchardt force-pushed the fix/dismiss-scroll-jump branch from d3e71c4 to 9814317 Compare July 22, 2026 12:27
@jancborchardt

Copy link
Copy Markdown
Member Author

@Antreesy

A bit hacky

Adjusted that

Maybe you can try asking agent to read nextcloud/vue library, if that can be an upstream fix instead?

Tried, but apparently:

Focus-trap intentionally jumps to the first item on removal (not a neighbor), so only the app knows which adjacent item should get focus. a11y-correct focus placement can't live upstream.

@Antreesy Antreesy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Otherwise works just fine. See no clearer way with focus-trap library limitations

Comment thread src/NotificationsApp.vue
},

setItemRef(el, notificationId) {
this.itemRefs ??= {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

let's add following:

  • declare it, so we don't introduce any hidden logic;
  • convert to Map (slightly better lookup code-wise);
  • pave the way for future maintenance and migrations
   setup() {
   	// comment what is this
       const itemRefs = new Map()
       const setItemRef = (el, id) => el ? itemRefs.set(id, el) : itemRefs.delete(id)

   	return {
   		itemRefs,
   		setItemRef ,
   		fairUsePolicyNotification,
   		hasThrottledPushNotifications,
   	}
   },

Comment thread src/NotificationsApp.vue
// Otherwise the header menu focus trap catches the lost focus and jumps
// it (and the scroll position) to the top of the list.
const neighbor = this.notifications[index + 1] || this.notifications[index - 1]
this.itemRefs?.[neighbor?.notificationId]?.focus()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.itemRefs?.[neighbor?.notificationId]?.focus()
this.itemRefs.get(neighbor?.notificationId)?.focus()

t,

/**
* Focus the dismiss button, used by the parent to keep focus in the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Public method should be declared:

	...
	emits: ['remove'],

	expose: ['focus'],

	data() {
	...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: 🏗️ At engineering

Development

Successfully merging this pull request may close these issues.

2 participants