Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject #3891

ShGKme · 2023-03-15T14:00:56Z

Currently, NcAppSidebarTabs gets content passed slot, goes throw all nodes, and detects NcAppSidebarTab instances. But it happened once on mounted and does not support tabs update.

In this solution:

NcAppSidebarTabs provides:
- registerTab method
- unregisterTab method
- activeTab property (replacement for $parent), could be reverted
All NcAppSidebarTab register and unregister themself on created and beforeDestroy
After registration NcAppSidebarTabs has the tab's data directly from the NcAppSidebarTabs

Sorting works the same way it was before. But there is no warnings if a user passes not-tabs as content.

This solution is fully compatible with Vue 3 as it is and Composition API.

raimund-schluessler

I like the approach and I think that we can waive the warning when tabs and non-tabs are mixed. The keyboard navigation needs fixing, but that's an easy fix.

src/components/NcAppSidebar/NcAppSidebar.vue

src/components/NcAppSidebar/NcAppSidebarTabs.vue

raimund-schluessler · 2023-03-15T19:48:03Z

src/components/NcAppSidebarTab/NcAppSidebarTab.vue

 		isActive() {
-			return this.$parent.activeTab === this.id
+			return this.activeTab === this.id


I just realized that this doesn't work in the Tasks app. I have no idea why, though.

@ShGKme I think it would be a good idea to revert this change for now. We can still use it when we figure out why it's broken currently. For vue3 it works fine, though.

It could be a problem with getter is old Vue is used. Updated with classic getter instead of computed.

src/components/NcAppSidebar/NcAppSidebarTabs.vue

raimund-schluessler · 2023-03-15T20:03:52Z

It currently does not work in the Tasks app, and I didn't figure out yet why.

raimund-schluessler · 2023-03-15T20:21:33Z

It currently does not work in the Tasks app, and I didn't figure out yet why.

For some reason, the injected activeTab value is not reactive when I test it in the Tasks app. Reverting to return this.$parent.activeTab === this.id fixes this. But I have no idea why though, especially since it works in the netlify docs. 🤷

raimund-schluessler · 2023-03-16T16:24:09Z

@ShGKme I pushed a few commits to fix some issues that I found, because I wanted to test the changes with my vue3 branch. If you don't like the changes, feel free to discard them.

raimund-schluessler · 2023-03-16T16:43:27Z

I checked that the approach also works for vue3. A few adjustments are of course necessary, but it works fine for vue3 as well, see #3692.

raimund-schluessler · 2023-03-26T17:00:49Z

@ShGKme Ping 🙂

ShGKme · 2023-03-27T11:55:53Z

@ShGKme Ping 🙂

Pong 👀. I'll work on this today.

ShGKme · 2023-03-28T01:29:07Z

Rebased onto master and rewrote WIP commit

ShGKme · 2023-03-28T01:57:37Z

Finished fix

Removed mixing tabs and not tabs content test
Replace computed with classic getter in case that was the problem with the Tasks app
Fix activating tabs after registration/unregistration (active prop in prior)
NcAppSidebarTab's icon slot rendering method moved to NcAppSidebarTab as a public Tab's method
NcAppSidebarTab's icon slot default content rendering also moved to NcAppSidebarTab component (otherwise slot and default slot content were defined in a different components).
All exposed NcAppSidebarTab instance properties defined in expose

Fix a11y

Set active tab's tabindex to 0 instead of undefined. It allows to navigate to tabs with keyboard (Tab, SHIFT+Tab). Previously you could move from tab to tab's content but couldn't move back...
Handle Home and End keypress to navigate to the first/last tab according to w3

See: https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html

@raimund-schluessler Don't you know if previous behavior was intended? It wasn't possible to focus tabs navigation with Tab of Shift+Tab.

Updated docs with JSDocs for events and default slots and examples

ShGKme · 2023-03-28T02:10:53Z

Tested in Talk

skjnldsv

Awesome work!

raimund-schluessler

Works nicely now with Tasks as well!

raimund-schluessler · 2023-03-28T07:38:41Z

Shall we squash the commits or merge as is?

fix: #3461 Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com> Co-authored-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>

ShGKme · 2023-03-28T13:35:36Z

Shall we squash the commits or merge as is?

I think, we may squash commits between the first WIP and finished fixing issue in fix dynamic tabs registration. But the last two other commits are less related to the original problem of the PR: a11y and general docs, I'd keep them separated.

I squashed with Co-authored-by, hope GH understands this :D

ShGKme mentioned this pull request Mar 15, 2023

Use render function in NcAppSidebarTabs #3874

Closed

raimund-schluessler requested changes Mar 15, 2023

View reviewed changes

raimund-schluessler added bug Something isn't working enhancement New feature or request 3. to review Waiting for reviews feature: app-sidebar Related to the app-sidebar component labels Mar 15, 2023

raimund-schluessler added this to the 7.8.1 milestone Mar 15, 2023

julien-nc modified the milestones: 7.8.1, 7.8.2 Mar 16, 2023

raimund-schluessler requested review from susnux, Pytal, marcoambrosini and skjnldsv March 16, 2023 16:47

raimund-schluessler mentioned this pull request Mar 16, 2023

Vue 3 #3692

Closed

2 tasks

mejo- modified the milestones: 7.8.2, 7.8.3 Mar 22, 2023

nickvergessen modified the milestones: 7.8.3, 7.8.4, 7.8.5 Mar 23, 2023

ShGKme force-pushed the fix/3461/sidebar-tabs-provide-inject branch from 98ee351 to 92851bb Compare March 28, 2023 01:26

ShGKme changed the title ~~DRAFT: Use provide/inject to register NcAppSidebarTab in NcAppSidebarTabs~~ Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject Mar 28, 2023

ShGKme marked this pull request as ready for review March 28, 2023 02:11

ShGKme requested a review from raimund-schluessler March 28, 2023 02:11

skjnldsv approved these changes Mar 28, 2023

View reviewed changes

raimund-schluessler approved these changes Mar 28, 2023

View reviewed changes

ShGKme and others added 3 commits March 28, 2023 15:26

fix(NcAppSIdebarTabs): fix dynamic tabs registration

32beb5d

fix: #3461 Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com> Co-authored-by: Raimund Schlüßler <raimund.schluessler@mailbox.org>

fix(NcAppSIdebarTabs): fix a11y, focusable navigation, Home End hotkeys

f9bc507

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>

docs(NcAppSIdebarTabs): add JSDocs and examples

f1acfcc

Signed-off-by: Grigorii Shartsev <grigorii.shartsev@nextcloud.com>

ShGKme force-pushed the fix/3461/sidebar-tabs-provide-inject branch from 871a53d to f1acfcc Compare March 28, 2023 13:35

ShGKme enabled auto-merge March 28, 2023 13:37

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 28, 2023

ShGKme merged commit 490c4a1 into master Mar 28, 2023
15 checks passed

ShGKme deleted the fix/3461/sidebar-tabs-provide-inject branch March 28, 2023 13:40

nickvergessen mentioned this pull request May 13, 2023

Right sidebar has no tab headers anymore on the files app when activity app is enabled nextcloud/server#38229

Closed

ShGKme mentioned this pull request May 13, 2023

NcAppSidebar: fix tabs with css icon #4111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject #3891

Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject #3891

ShGKme commented Mar 15, 2023

raimund-schluessler left a comment

raimund-schluessler Mar 15, 2023

raimund-schluessler Mar 15, 2023

raimund-schluessler Mar 16, 2023

ShGKme Mar 28, 2023

raimund-schluessler commented Mar 15, 2023

raimund-schluessler commented Mar 15, 2023

raimund-schluessler commented Mar 16, 2023

raimund-schluessler commented Mar 16, 2023

raimund-schluessler commented Mar 26, 2023

ShGKme commented Mar 27, 2023

ShGKme commented Mar 28, 2023

ShGKme commented Mar 28, 2023

ShGKme commented Mar 28, 2023

skjnldsv left a comment

raimund-schluessler left a comment

raimund-schluessler commented Mar 28, 2023

ShGKme commented Mar 28, 2023

Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject #3891

Fix dynamic tabs registration and a11y in NcAppSidebarTabs with provide/inject #3891

Conversation

ShGKme commented Mar 15, 2023

raimund-schluessler left a comment

Choose a reason for hiding this comment

raimund-schluessler Mar 15, 2023

Choose a reason for hiding this comment

raimund-schluessler Mar 15, 2023

Choose a reason for hiding this comment

raimund-schluessler Mar 16, 2023

Choose a reason for hiding this comment

ShGKme Mar 28, 2023

Choose a reason for hiding this comment

raimund-schluessler commented Mar 15, 2023

raimund-schluessler commented Mar 15, 2023

raimund-schluessler commented Mar 16, 2023

raimund-schluessler commented Mar 16, 2023

raimund-schluessler commented Mar 26, 2023

ShGKme commented Mar 27, 2023

ShGKme commented Mar 28, 2023

ShGKme commented Mar 28, 2023

Finished fix

Fix a11y

Updated docs with JSDocs for events and default slots and examples

ShGKme commented Mar 28, 2023

skjnldsv left a comment

Choose a reason for hiding this comment

raimund-schluessler left a comment

Choose a reason for hiding this comment

raimund-schluessler commented Mar 28, 2023

ShGKme commented Mar 28, 2023