Header, navbar, and admin sidebar refactoring

[davide-negretti]

References

• Fixes Header and navbar refactoring #2656
• Fixes DOM contains duplicated HTML elements #2368
• Fixes [Accessibility] "All of DSpace" menu in header does not expose the presence of the submenu & its state via ARIA attributes #2699
• Fixes Accessibility of the administration menu (submenus) on the left hand side  #2178

Description

Purpose of this PR:

• change the structure of the header and the navbar in order to:
  • make customisation easier
  • improve responsivity
  • improve accessibility
• remove unnecessary styling from shared components in order to make customisation easier
• fix admin sidebar issues
  • sidebar width must be the same when vertical scrollbar is present
• improve overall accessibility of:
  • main navbar
  • menubar (login, language switch, ...)
  • admin sidebar
• decouple sidebar and header/navbar styling
• decouple custom and DSpace theme styling
• make color customisation easier

List of changes

• Menu items (*-menu-item components)
  • minor changes have been made in order to better handle their styling from their parent components
• Header and navbar
  • the content of header, navbar, and header-navbar-wrapper has been rearranged
    • the navbar component only contains the main menu
    • the <nav> tag now contains navigation links only, as required by semantic HTML specification
• Accessibility
  • ARIA attributes and roles have been improved according to specification
• Sidebar
  • the way collapsing work has been changed in order to solve issues with vertical scrolling
  • other changes were required in order to decouple sidebar and navbar styling
• Theming
  • SCSS variables have been reorganised and unnecessary variables have been removed
  • new SCSS variables have been added in order to make color customisation easier
• host-window service
  • some utility functions for responsivity have been added
• Other changes
  • some unecessary code has been removed

Accessibility

In order to test overall accessibility I used this Chrome extension that provides a visual representation of how screen readers render the page:
https://chromewebstore.google.com/detail/aria-devtools/dneemiigcbbgbdjlcdjjnianlikimpck

This is how the current layout is rendered:
admin sidebar
header

And this is how it is rendered with this PR:

I also checked for errors with this Chrome extensions and there are none:
https://chromewebstore.google.com/detail/wave-evaluation-tool/jbbplnpkjmmeebjpijfedlgcdilocofh

Note that menu and menuitem are now being used, and dropdown menus are better handled.

Instruction for reviewers

• Check that both dspace and custom theme work fine in both mobile and desktop mode
• Check that CSS variables such as theme colors work fine
• Check that header, navbar, and sidebar are fully accessible

Future improvements

These components still require some changes, for which I'm going to open a new issue:

• user-menu component
  • should only contain a list of menu items
  • toggler button is used in desktop mode only, and it should be moved to the parent component
  • the outer wrapper should be moved to the parent components as well
  • menu items should be
  • elements in order to improve the collapsible navbar
  • it should be made more accessible through ARIA attributes
  • Bootstrap classes should be removed as their properties are being overridden
• dropdown menus
  • they should be made more accessible through ARIA attributes
  • Bootstrap classes should be removed as their properties are being overridden

Checklist

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[github-actions]

Hi @davide-negretti,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

@davide-negretti : It looks like this PR now has merge conflicts, possibly because of the merger of #2683 (which also fixed some accessibility issues in the header). If you can get this updated, I'd love to give this PR a test to see if it fixes the duplicate tags in DOM & improves accessibility even further. Thanks!

[davide-negretti]

@tdonohue I just pushed my branch, which I already rebased onto main a couple of hours ago. Unfortunately I had to discharge most of the conflicting changes because they did not apply anymore. The only changes I could have kept were some <a> elements that have been replaced by <button> elements, however I already handled accessibility issues by using the role attribute. I'm currently fixing some broken tests, and I may need to do some minor layout fixes, however this is ready to be tested in the meanwhile

[tdonohue]

@davide-negretti : Even though this is still a "Draft", I gave this a very brief test just now in production mode (yarn build:prod; yarn serve:ssr), just to see how it's working. The basics seem to work.

However, when I login, I receive a number of server-side rendering (SSR) errors which imply that SSR isn't working anymore:

ERROR RuntimeError: NG03402
    at triggerTransitionsFailed (C:\dspace-angular\dist\server\main.js:7:4969532)
    at TransitionAnimationEngine.reportError (C:\dspace-angular\dist\server\main.js:7:5048893)
    at TransitionAnimationEngine._flushAnimations (C:\dspace-angular\dist\server\main.js:7:5053458)
    at TransitionAnimationEngine.flush (C:\dspace-angular\dist\server\main.js:7:5048266)
    at InjectableAnimationEngine.flush (C:\dspace-angular\dist\server\main.js:7:5069338)
    at C:\dspace-angular\dist\server\main.js:7:5605397
    at _ZoneDelegate.invoke (C:\dspace-angular\dist\server\main.js:7:2179478)
    at Zone2.run (C:\dspace-angular\dist\server\main.js:7:2172289)
    at NgZone2.runOutsideAngular (C:\dspace-angular\dist\server\main.js:7:3215596)
    at AnimationRendererFactory.end (C:\dspace-angular\dist\server\main.js:7:5605336) {
  code: 3402
}
Error in server-side rendering (SSR)
Error details :  RuntimeError: NG03402
    at triggerTransitionsFailed (C:\dspace-angular\dist\server\main.js:7:4969532)
    at TransitionAnimationEngine.reportError (C:\dspace-angular\dist\server\main.js:7:5048893)
    at TransitionAnimationEngine._flushAnimations (C:\dspace-angular\dist\server\main.js:7:5053458)
    at TransitionAnimationEngine.flush (C:\dspace-angular\dist\server\main.js:7:5048266)
    at InjectableAnimationEngine.flush (C:\dspace-angular\dist\server\main.js:7:5069338)
    at InjectableAnimationEngine.ngOnDestroy (C:\dspace-angular\dist\server\main.js:7:5609542)
    at R3Injector.destroy (C:\dspace-angular\dist\server\main.js:7:2954388)
    at NgModuleRef.destroy (C:\dspace-angular\dist\server\main.js:7:3139781)
    at C:\dspace-angular\dist\server\main.js:7:3229024
    at Array.forEach (<anonymous>) {
  code: 3402
}
Falling back to serving direct client-side rendering (CSR).

[davide-negretti]

@tdonohue this is now complete and can be tested. This PR is going to conflict with #2704, however I already handled all ARIA attributes

[davide-negretti]

@artlowel

• for the navbar issue I can use an attribute selector [dsNavbarSection] so that the <ul> will have only direct <li dsNavbarSection> descendants
• I will make the header height variable on mobile screens
• I will fix merge conflicts and the login dropdown issue

[davide-negretti]

@artlowel

• I replaced all <ul> and <li> with <div> in navbar and sidebar
• I made the header height variable on mobile screens. I did not fix the issue with sr-only, however this only occurs when the screen width is below 270px
• I restored the behaviour of links in the user dropdown menu

[artlowel]

Thanks @davide-negretti!

One more minor comment. Perhaps due to the switch from li's to divs, the spacing between options in the mobile menu seems to have decreased. It is especially noticeable, because the user menu above still uses li's

[alexandrevryghem]

@davide-negretti: I think it would be best to remove the changes you made regarding to the <ul>/<li> structure replacement to nested <div>s, because this is not good for accessibility. Instead you could just replace this ng-container with an li and replace this <li> & this <li> with a <div>.

[davide-negretti]

@alexandrevryghem I already added to every menu the menubar, menu and menuitem ARIA roles, which are more appropriate - from the accessibility point of view - than unordered lists, which use the list and listitem roles under the hood.

I carefully tested the header with a screen reader, which previously recognised the menus as generic lists with N items, and now correctly recognises them as navigation bars.

I also inspected the page with ARIA DevTools chrome extension. This is the current situation on demo.dspace.org:

And this is with the correct roles:

[davide-negretti]

@artlowel I restored the spacing between items

[artlowel]

Thanks @davide-negretti!

[github-actions]

Hi @davide-negretti,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

[tdonohue]

👍 Thanks @davide-negretti for your hard work on this! I retested today after merging the last 8.0 features that touch the header/navbar. This is still working perfectly and accessibility issues mentioned all appear fixed. Merging this immediately. (Please note: I am going to attempt an automated port to 7.x...though based on the size of this PR, it may not succeed.)

[dspace-bot]

Backport failed for dspace-7_x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dspace-7_x
git worktree add -d .worktree/backport-2676-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-2676-to-dspace-7_x
git switch --create backport-2676-to-dspace-7_x
git cherry-pick -x 640612033c7a296d56a3e3d7497fa99492947011 e84773afb25b20a9689a614f20361f7e8b086fc9 d59f33278dd5c1d76cadce614d3e125e9b5e0568 216558125bfdd12fda3a6104315b07ef2f1cd75b b8ec74a04624e1c351235ffa2f40bfbc8c2fe55c 8f73b8ff9d6ec426f51cb8a5a6694b9afb265781 f25ec6210b33752dfa039a8dc1bece43f864cbea 80cc4c5d9a521e3dbe98bd508a08d395ea836537 87d3383bbaff352cd976d661d56f90d94f8f76a4 46fe7f63e316c7e14ff41e05dfcbbbf39567faf1 523850bc45ed640d3331e07932d9eae8785d33c6 abba806d405af9de44fea3734c542fa8ab7fef1b bff93a08471240e107704cd7c525ac747b0c93b6 8e35c0f713f37c76e0711ac7f862d02ed4c29d83 d1dc8e6038673de22946fddc8d41b22862bcb5a6 fdbe7a6005e59783075395aa3ba26c2b56f7afe5 a88282d70b8677712d5515ca697c46276840e774

[tdonohue]

@davide-negretti : As I worried, this was unable to be ported automatically to 7.x. If you feel this work could be ported to 7.x, then I'd appreciate it if you could find time to create a version of this PR against the dspace-7_x branch. If you feel this work is not easily possible to port to 7.x, then we can keep this as an 8.0-only accessibility fix.

Please let me know what approach you feel is best.

[davide-negretti]

@tdonohue I'm going to try to port it manually, I'll let youknow if the merge conflicts can be easily handled

[davide-negretti]

@tdonohue

• I opened the PR for the porting: 7.x - Header, navbar, and admin sidebar refactoring #2858
• I made a minor fix for this PR (already included in the porting PR): Header, navbar, and admin sidebar refactoring (minor fixes) #2859