Skip to content

fix: make top nav bar right elements within a nav - #63239

Merged
susnux merged 1 commit into
masterfrom
fix/top-navbar-right-elements-nav
Sep 22, 2026
Merged

susnux merged 1 commit into
masterfrom
fix/top-navbar-right-elements-nav

Conversation

@kristian-zendato

@kristian-zendato kristian-zendato commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Move the top navbar right elements within a nav element.

Checklist

AI (if applicable)

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

@kristian-zendato
kristian-zendato requested review from a team as code owners August 14, 2026 06:42
@kristian-zendato
kristian-zendato requested review from icewind1991, leftybournes, provokateurin, skjnldsv, sorbaugh and susnux and removed request for a team August 14, 2026 06:42
@susnux
susnux requested a review from pringelmann August 14, 2026 13:30
Comment thread core/templates/layout.user.php Outdated
Comment thread core/css/mobile.scss Outdated
Comment thread core/src/components/UnifiedSearch/UnifiedSearchLocalSearchBar.vue Outdated
@kristian-zendato
kristian-zendato force-pushed the fix/top-navbar-right-elements-nav branch from 384a1bd to 4551f67 Compare August 20, 2026 17:51

@susnux susnux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This part looks good, likely needs adjustments in the vue library for NcHeadMenu to set role = menuitem

@kristian-zendato

Copy link
Copy Markdown
Contributor Author

@susnux Regarding updating role of NcHeaderMenu to menuitem, I don't think adding role="menuitem" in NcHeaderMenu is a good solution because we have many places using it.
To solve this problem, I am going to add is-menu-item prop to it.
What do you think about my idea?

@susnux

susnux commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

@kristian-zendato which other place is using it? It must only be used within that header bar thats its full purpose.

@kristian-zendato

Copy link
Copy Markdown
Contributor Author

@susnux I checked that again and yes, they are only used in header.
Then it means, we can update role in NcHeaderMenu without any further update?

@susnux

susnux commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Then it means, we can update role in NcHeaderMenu without any further update?

yes

@ShGKme

ShGKme commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

ul needs role = menubar

@susnux Why? menubar has different semantics and requirements.

menubar represents a menu, just with always visible items.

Semantically, this is not about any set of buttons but a complete actions set. Like this one:
image

I can see how it can be used as an application menu, but even in this case, it is usually about the classic application menus (File | Edit | View | Help).

Technically, a menubar must have menu keyboard navigation, for example:

  • Entire menu is a single focusable element, entered by Tab and exited by the next Tab
  • Navigation inside the manu is performed by Arrows

So if we keep it a menubar, we must implement it as a menu bar.

What about toolbar instead?

@susnux

susnux commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What about toolbar instead?

You are right! The proper role is likely toolbar after reading the role descriptions again.
Thank you!

@kristian-zendato
kristian-zendato force-pushed the fix/top-navbar-right-elements-nav branch from 4551f67 to ff48cf3 Compare August 28, 2026 08:04
@kristian-zendato

Copy link
Copy Markdown
Contributor Author

@susnux , @ShGKme Thank you for your review.
I update the role to toolbar instead of menubar and remove the PR from nextcloud-vue as no update is needed from there.

I'd appreciate it if you review it again.

Comment thread core/templates/layout.user.php Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@solracsf solracsf added this to the Nextcloud 36 milestone Sep 12, 2026
Comment thread core/templates/layout.user.php Outdated
Comment on lines +78 to +88
<nav class="header-end" aria-label="<?php p($l->t('Header menu')); ?>">
<ul class="header-end__items" role="toolbar">
<li class="header-end__item" role="none">
<div id="notifications"></div>
</li>
<li class="header-end__item" role="none">
<div id="contactsmenu"></div>
</li>
<li class="header-end__item" role="none">
<div id="user-menu"></div>
</li>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the plan would be now:

  • remove role none from li
  • remove role toolbar
  • = keep this as a simple list
  • change label as "header menu" is too unspecific

Signed-off-by: kristian-zendato <kristian.zendato@nextcloud.com>
@kristian-zendato
kristian-zendato force-pushed the fix/top-navbar-right-elements-nav branch from 09c5907 to 4ae9e88 Compare September 15, 2026 11:36
<div id="contactsmenu"></div>
<div id="user-menu"></div>
</div>
<nav class="header-end" aria-label="<?php p($l->t('Notifications, contacts and settings')); ?>">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@nimishavijay or @kra-mo do you have a better idea how to label the section of the navigation here? (the part including notifications, assistent, contacts etc)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They are all rather distinct apps there, and if anyone using a screenreader heard something generic for this category like "More apps" or something like that, they may skip over it entirely.

If there is any way we can take them out of this section and have them be directly accessible that would be the best from a UX persepctive. If that's not possible then I'd suggest "Nextcloud Assistant, notifications and settings" to indicate the most commonly used apps.

In the future we could also think about moving the assistant to the center section as it's closer in function to searching, then the top right group could become "Notifications and settings" which makes more sense in my head.
@kra-mo any thoughts here since you worked on the new layout?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The original proposal did have Assistant next to search, but I wonder about the balance if we have suggested apps on the left in the future.

As for the label, considering it also includes the people menu for example, I really am not sure what to call it. I do think something generic may be the best, but without using the word "app". For regular users, I would reserve that word for apps in the grid. Maybe "Actions and options"?

@susnux
susnux merged commit bd33dd4 into master Sep 22, 2026
154 checks passed
@susnux
susnux deleted the fix/top-navbar-right-elements-nav branch September 22, 2026 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants