Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve breadcrumb bar accessibility #6912

Merged
merged 34 commits into from
Oct 22, 2022

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jul 23, 2022

What's new

  • The active breadcrumb is now a text element rather than a clickable link (better for accessibility)
  • Cleanup to the code to bring it inline with modern Jenkins BEM practices
  • ARIA labels have been added

Behaviour changes

  • The active breadcrumb (due to it being text and not a link) no longer has a dropdown menu
    • I'd argue the menu wasn't necessary as it just duplicates what's on the page
    • For the Configure System page, the section-to-sidebar-items solution would be better suited
  • The chevron after the active breadcrumb has been removed if the active crumb is not a ModelObjectWithChildren

Proposed changelog entries

  • N/A

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@janfaracik janfaracik marked this pull request as ready for review July 24, 2022 11:48
@timja
Copy link
Member

timja commented Jul 24, 2022

/reviewer @jenkinsci/sig-ux

@comment-ops-bot comment-ops-bot bot requested a review from a team July 24, 2022 14:47
@timja
Copy link
Member

timja commented Jul 24, 2022

/label web-ui,enhancement

@comment-ops-bot comment-ops-bot bot added the web-ui The PR includes WebUI changes which may need special expertise label Jul 24, 2022
@comment-ops-bot
Copy link

I wasn't able to add the following labels: enhancement

Check that the label exists and is spelt right then try again.

@timja
Copy link
Member

timja commented Jul 24, 2022

/label rfe

@comment-ops-bot comment-ops-bot bot added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jul 24, 2022
@uhafner
Copy link
Member

uhafner commented Jul 24, 2022

Behaviour changes

  • The active breadcrumb (due to it being text and not a link) no longer has a dropdown menu

    • I'd argue the menu wasn't necessary as it just duplicates what's on the page
    • For the Configure System page, the section-to-sidebar-items solution would be better suited
  • The chevron after the active breadcrumb has been removed

    • It suggests there is more after the active breadcrumb, but that's never the case

Would be good to hear thoughts on the above ^

I think that both improvements make sense and clean up the UI!

@NotMyFault
Copy link
Member

Behaviour changes

  • The active breadcrumb (due to it being text and not a link) no longer has a dropdown menu
    • I'd argue the menu wasn't necessary as it just duplicates what's on the page
    • For the Configure System page, the section-to-sidebar-items solution would be better suited

Makes sense to remove it, it displays content you already have on the page available.

  • The chevron after the active breadcrumb has been removed
    • It suggests there is more after the active breadcrumb, but that's never the case

+1

@uhafner
Copy link
Member

uhafner commented Nov 29, 2022

getDisplayName of ModelObject

That'll just duplicate the previous breadcrumb that always exists. Please read the cases I describe above again. For example, in https://weekly.ci.jenkins.io/view/all/builds, you're asking to have another breadcrumb labeled All.

I'm not sure if I understand your argumentation. The view that renders the URL builds should be a ModelObject with the name Build History of Jenkins. Or is this a view without a model object?

@daniel-beck
Copy link
Member

daniel-beck commented Nov 29, 2022

Or is this a view without a model object?

The ModelObject for builds is the View. It has index and builds views, among others.

There would never be a use case for a full (non-ajax) non-index view based on your argumentation.

@uhafner
Copy link
Member

uhafner commented Nov 29, 2022

I see. So the same ModelObject renders different URLs? This is something that we should avoid. Has this approach a historical background or some use cases that could not be done by providing a standalone model object for each link? In my plugins every view is a model object with URL and display name. I must admit that I don't see the benefit for not following a strict architectural pattern that every view with a URL should be a model object. Would it be feasible to change that or at least create a warning if someone does not follow the default path?

@daniel-beck
Copy link
Member

This is something that we should avoid.

It has always worked like this and any attempt to change this will require changing practically every plugin that defines custom views. Just to make sure this is clear: Every view not named index that is an actual view (~has <l:layout>) is affected by this. I wouldn't be surprised if it's thousands across the plugin ecosystem. Often it doesn't make sense to update breadcrumbs. For example, PluginManager is the ModelObject for the index (updates, really), available, installed, and advanced views. Having each view governed by its own ModelObject would be madness.

@uhafner
Copy link
Member

uhafner commented Nov 29, 2022

This is something that we should avoid.

It has always worked like this and any attempt to change this will require changing practically every plugin that defines custom views. Just to make sure this is clear: Every view not named index that is an actual view (~has <l:layout>) is affected by this. I wouldn't be surprised if it's thousands across the plugin ecosystem. Often it doesn't make sense to update breadcrumbs. For example, PluginManager is the ModelObject for the index (updates, really), available, installed, and advanced views. Having each view governed by its own ModelObject would be madness.

Tanks for clarifying. In my opinion having a ModelObject for each view would be a clean architecture and not madness. From #7457 we see that we now need to change a lot of views, this anti-pattern clearly violates the open closed principle. But anyway, it's too late for an architectural change for the existing views. I can only recommend to implement it differently for new views.

@Wadeck
Copy link
Contributor

Wadeck commented Nov 29, 2022

⚠️ Due to the attempt to easily fix the issues in core in #7457, I am wondering if we should revert this PR and provide a more organized migration path. I already have seen multiple feedbacks about users being surprised by the new behavior, which is not necessarily a good signal.

@janfaracik
Copy link
Contributor Author

Hey folks,

Thought I'd comment on a few points just to share my thoughts:

  1. Giving every page a breadcrumb item

I'd be opposed to giving every page their own breadcrumb item, IMO pages inside of the two column navigation sidebar (like the build page, plugin manager, design library) should only have the one breadcrumb (that being the parent page). I see the sidebar for switching part of a pages content, much like a tab bar, hence not having an additional breadcrumb for its children pages.

To go back from the console output, clicking 'Status' should work just fine (unless there's a plugin/situation where that's not possible?)


  1. Usage of the breadcrumb button as a way to jump to the top of the page

In a similar fashion to the use of the breadcrumb bar item to refresh the page (sorry again!) the component wasn't designed to jump to the top of the page, although I can definitely see how that'd be helpful in both cases. In the current prototype design for Jenkins there's a button to do so in the sidebar which might be of interest to you.

image

As for a possible reversion - if this is wanted (and I have no quarrel with that if that's preferred) I'd rather myself revert the change to the last breadcrumb (from a link to text) rather than revert the entire PR as there's still benefits to the component outside of the mentioned change.

@benipeled
Copy link

In the current prototype design for Jenkins there's a button to do so in the sidebar which might be of interest to you.

That might be an nice option,

Another option is to stick the sidebar on scrolling down (position: fixed), allowing easy navigation between the different components (console, config etc.)

@daniel-beck
Copy link
Member

daniel-beck commented Nov 30, 2022

From #7457 we see that we now need to change a lot of views

No need. We can just revert part of this PR.

In my opinion having a ModelObject for each view would be a clean architecture

… necessitating even more changes, and not just in views. This is even worse than just adding breadcrumbs everywhere.

@jtnord
Copy link
Member

jtnord commented Nov 30, 2022

Shouldn't all views get a breadcrumb link by default?

How would you infer the label?

if the URL is not the URL of the last breadcrumb - use the page title as a fallback if not specified?

Will be a little "verbose" in many cases "Manage Jenkins -> Plugin Manager -> Plugin Manage Updates" (or something like that)

(possibly will break somewhere though, and there is the question of if trying to infer a breadcrumb is even a good idea or not)

@jtnord
Copy link
Member

jtnord commented Nov 30, 2022

(OT: to go to the top of the page with the mouse as would happen with the old behaviour there is a handy "reload" button in almost every browser just above the breadcrumb bar) for some accessibility there is keyboard for this too (ctrl+r, and F5 on windows (option something on mac?)) for assistive tech, not sure but I would expect a "reload" option to control the browser too.
To do without reloading there is ctrl+home (on windows - something with option on mac) but no standard mouse option to do it in one.

@daniel-beck
Copy link
Member

use the page title as a fallback if not specified

In case of https://weekly.ci.jenkins.io/view/all/builds, that would still result in a duplicate "All" link. Now what?

without reloading there is ctrl+home (on windows - something with option on mac)

Just Home, which is Fn+Left on small keyboards. Also for iOS it's tapping near the top of the screen.

Given the myriad options this feels like less of a necessity to me, although I guess some pages with dynamically loaded content still benefit from easy reloading. But that seems to me that the pages themselves should be looked at critically.

no standard mouse option to do it in one.

Firefox has reload in the context menu.

@benipeled
Copy link

Another option is to stick the sidebar on scrolling down (position: fixed), allowing easy navigation between the different components (console, config etc.)

@janfaracik multiple options on the sidebar should be taken into account but in general this is the idea
WDYT?

fixed_sidebar.webm

@uhafner
Copy link
Member

uhafner commented Nov 30, 2022

Another option is to stick the sidebar on scrolling down (position: fixed), allowing easy navigation between the different components (console, config etc.)

@janfaracik multiple options on the sidebar should be taken into account but in general this is the idea WDYT?

Yes, this is the concept we should use for the sidebar.

@janfaracik
Copy link
Contributor Author

Another option is to stick the sidebar on scrolling down (position: fixed)

I'm definitely in favour of the sidebar being stuck to the side of the screen 👍 The project config screen already does this, would be good to get it adopted by other pages consistently (only issue is pages with sidebars that are too long).

@timja
Copy link
Member

timja commented Dec 10, 2022

see https://groups.google.com/g/jenkinsci-dev/c/2Z1OuI6XC90/m/Se_zVkfwGwAJ for a mailing list request of a revert or partial revert, @janfaracik are you able to take a look at this please?

basil added a commit to basil/jenkins that referenced this pull request Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues squash-merge-me Unclean or useless commit history, should be merged only with squash-merge web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.