Support links in Combobox Pajamas component

added Pajamas UX featureaddition frontend typefeature labels

@slashmanov, Please add a group or category label to identify issue ownership.

You can refer to the Features by Group handbook page for guidance.

If you are unsure about the correct group, please do not leave the issue without a group label, and refer to GitLab's shared responsibility functionality guidelines for more information on how to triage this kind of issue.

This message was generated automatically. You're welcome to improve it.

Don't know which group that belongs to, so I am assigning it to groupcode review for now (we must be the only ones who completed the migration with this particular case).

@slashmanov if I understand this correctly, it poses an accessibility problem since the only child of role="listbox" can be role="option" (ref) and not links.

@jeldergl would it violate ARIA guidelines if we override the role here? <a role=listbox>...</a>

I think if you overrode the role on the ul perhaps, but then I'm not sure why a disclosure wouldn't work since it can contain links.

In Pajamas, the recommendation is to default to disclosure if not all items are options, which seems to be the case here. In the design spec we don't have the concept of a selected action (link or button), only a selected option.

Is there a specific dropdown that you could point me to for context? Thanks!

@jeldergl here's the one I had troubles with: Draft: Migrate compare versions dropdown to GlC... (!135277 - closed). Also, the same applies to every single legacy sorting dropdown (using links to apply the sorting), which has a list of links and an active\selected state. An example can be seen on the issues list page.

@slashmanov thanks for the reference. It's actually coded as a menu with button role="menuitem", but no links. This, I think, is much closer to the listbox with options.

@markrian are you familiar with any migrations of similar instances?

This a similar-ish migration, yes: !129722 (merged).

The intention of the dropdown was similar to this one (make it possible to open windows from these items), but its original implementation was different. In that MR, the items were regular buttons which were given special Ctrl-click behaviour.

There was a fairly big discussion about preserving that.

The rewrite ended up more or less preserving it, because it was regarded as an important user feature (despite not being discoverable). It's definitely a hack and a compromise, but in the interest of moving things forward, that's what was decided.

I still think that these should be implemented as GlDisclosureDropdowns, as fundamentally they're more like navs than select boxes.

Perhaps accessibility could be improved with aria-current="page" attribute on the current item. If this is a common pattern, we'd probably want to create a reusable implementation of this, perhaps as the inner part of the dropdown content.

I still think that these should be implemented as GlDisclosureDropdowns, as fundamentally they're more like navs than select boxes.

@markrian that means we won't have a selected state to represent a current selection since only options should have that.

Are these more like nav than selection because of the underlying behavior? A different URL vs. just a page reload with different presentation?

Because each of these states is deep-linkable, it seems natural to want to expose them as links. I think that's where the source of this difficulty is.

If these states never were deep-linkable, we wouldn't be in this position, because then this dropdowns would just be selects or GlCollapsibleListboxs.

that means we won't have a selected state to represent a current selection

But if we treat this as a nav, wouldn't that selected state essentially be the aria-current="page"?

Thanks for the helpful explanation @markrian!

But if we treat this as a nav, wouldn't that selected state essentially be the aria-current="page"?

Not necessarily, the current page is still Issues list, while this is a choice about the sort order of the content on that page.

In the design, we've intentionally not included a selected state for an action to avoid conflating the difference between actions and options.

If these states never were deep-linkable…

Naive question, but why do they have to be? Is this because the choice is appended to the URL?

But if we treat this as a nav, wouldn't that selected state essentially be the aria-current="page"?

Not necessarily, the current page is still Issues list, while this is a choice about the sort order of the content on that page.

Ah, I think we're talking about two different dropdowns I'm talking about the MR version chooser, whereas I think you're talking about the sort order dropdown in issues pages.

I suppose your point could still apply - the current page is still the merge request, while this is a choice about the version of the code shown on that page.

If these states never were deep-linkable…

Naive question, but why do they have to be? Is this because the choice is appended to the URL?

Nothing has to be deep linkable, it's just that:

that's the way it's currently implemented, and
making these linkable is a feature: a user may want to easily open two different versions of the same MR in different tabs.

Yeah, I was referring to the sorting, thanks for clarifying!

Would something like this be possible?

Use listbox, selecting an option updates the query string.
A user can copy the full URL for that specific view.

Is changing the current implementation a challenge or something that could be done in batches for all sorting instances?

Use listbox, selecting an option updates the query string.

@jeldergl, that's something I've done in this migration: Draft: Migrate compare versions dropdown to GlC... (!135277 - closed) (marking now as blocked by this issue). The downside is that the new dropdown is lacking an ability to work with list items as links (i.e. ctrl+click to open a new tab, copy link, etc.). From a user standpoint that's a breaking change. We should either support links in the listbox component or have a selected state in the disclosure one. So that for an end user nothing's changed. And by support I mean not only changing the implementation but also having this added to the Pajamas guidelines. I can work on that if we're aligned on one of these solutions.

We should either support links in the listbox component…

We shouldn't do this because it's not in line with role="option" being the only child of role="listbox" and will fail accessibility tests.

From a user standpoint that's a breaking change. … So that for an end user nothing's changed.

In regards to the UX of sorting, I've personally never expected to control-click to open a new page or to copy a URL. Is there a way to determine how frequently this is done? We're not just addressing the use case of one type of user here either since the changes in the new components were done so that the components are used in an accessible way that aligns with semantic use and expectations too. A screen reader user expects to select an option, but not that some other link-like behavior can be performed. Genuine question, is it okay to change something for the user if it results in better accessibility and more clear component use? It doesn't prevent a user from copying the resulting URL+query, or even opening a new tab and adjusting the query, it just changes how it might be done today with two different behaviors on one element.

The downside is that the new dropdown is lacking an ability to work with list items as links (i.e. ctrl+click to open a new tab, copy link, etc.)

I don't think this should be a capability for options. It's adding a second function/capability to something that shouldn't have it. In other words, selecting an option and following a link are two different things that shouldn't be combined. I don't think there's even a normative way to offer both behaviors for a keyboard user.

… or have a selected state in the disclosure one.

From a UX standpoint, this starts to confuse two components with different use cases and makes future migrations potentially more challenging because it's tougher to determine when to use each when the overlap is greater. If this were treated as navigation, something like aria-current="page" would be confusing for the selected state of a "Created date" sorting option in the issues list because the page isn't "created date," it's just the order of the issues.

I would ask, is the intent for a user to navigate or to choose a different view/parameter for the page they're already on? I think it should be one or the other, but not both at the same time and we should choose the component that matches the primary intent.

I've personally never expected to control-click to open a new page or to copy a URL. Is there a way to determine how frequently this is done?

This is a very common pattern when working with links. You can use either a Ctrl keyboard key or a middle mouse button. I personally use only middle mouse button for that and it's super convenient when compared to this flow: duplicate current page, get to the same state as the original page, click the necessary button. We can track middle clicks on these links but that'd be only tracking gitlab.com users, excluding standalone installations.

For this particular use-case in MR it speeds up things significantly. If you need to have specific commit compared in another tab you'll have to wait for the MR to load twice: first to get to the original state, then to get to the desired one which causes full page reload. I can totally see how this can be frustrating if you wanted to have multiple of these commit pages opened.

We're not just addressing the use case of one type of user here either since the changes in the new components were done so that the components are used in an accessible way that aligns with semantic use and expectations too. A screen reader user expects to select an option, but not that some other link-like behavior can be performed.

Thanks for that explanation! I think that means we are not supposed to use listbox component in the MR version select. It's not a form control that you config and then submit a form for example. It's just a list of links the lead you to a different state of the page (we can consider them different pages). So this probably should be handled but adding a selected state support to the disclosure component or something else.

I would ask, is the intent for a user to navigate or to choose a different view/parameter for the page they're already on? I think it should be one or the other, but not both at the same time and we should choose the component that matches the primary intent.

At the moment the intent is to navigate to a different page, we can't change this state without reloading the page completely as of now.

From a UX standpoint, this starts to confuse two components with different use cases and makes future migrations potentially more challenging because it's tougher to determine when to use each when the overlap is greater. If this were treated as navigation, something like aria-current="page" would be confusing for the selected state of a "Created date" sorting option in the issues list because the page isn't "created date," it's just the order of the issues.

This component is very close to what we have in the new navigation sidebar:

I see that they had to implement their own custom components for that, so it might be that we need to do the same here? Unfortunately I don't see any aria roles attached to these.

Thanks @jeldergl!

This is a very common pattern when working with links.

Oh yes! To be sure, I was just referring to this type of behavior for role="option". I use it all of the time for links too.

So this probably should be handled but adding a selected state support to the disclosure component or something else.

At the moment the intent is to navigate to a different page

Got it, I've probably been too focused on the sorting and am starting to have a better understanding of what's needed here, thanks for your patience. So for the MR version select, I can see now why you'd need a selected state. Along those lines, perhaps the ✓ isn't the right indicator, but something closer to nav might be. @danmh, @seggenberger, or @aregnery do any other nav-like elements come to mind that we've applied a selected state to?

Unfortunately I don't see any aria roles attached to these.

I defer to @thutterer for the nav in general, but I do know that aria is used for the current page. What roles are you looking for?

What roles are you looking for?

Any. Right now this control only has an implicit list role, which might be sufficient for this case.

If it's in a nav element then it's good.

Yeah, the navigation sidebar is in a nav, and the active link gets an aria-current="page" attribute.

@jeldergl should we proceed with adding an active\selected state to the disclosure dropdown?

@slashmanov so far this is the only instance of this I've come across, so hesitant to add it as a default. I understand the need to extend the component for this though. @markrian what do you feel would make the most sense here?

I think the GlDisclosureDropdownItem component already has everything it needs for this to be implemented.

I see it being implemented something like this (pseudocode):

<gl-disclosure-dropdown ...>
  <nav>
    <gl-disclosure-dropdown-item v-for="item in items" ...>
      <template #list-item>
        <gl-icon :class="item.classes" name="check" />
        {{ item.text }}
      </template>
    </gl-disclosure-dropdown-item>
  </nav>
</gl-disclosure-dropdown>

where each item would have classes, like { 'gl-visibility-hidden': !isCurrentPage(item.href) }. If we wanted to add aria-current="page" as well, that could be done by setting item.extraAttrs.

I wouldn't want to bring this into GitLab UI unless we were sure it were a reasonably semantic and accessible solution. Being a one-off, I think it's okay for it not to be.

Thoughts?

added groupcode review label

added devopscreate sectiondev labels

mentioned in issue #346804 (closed)

marked this issue as blocking #413047

mentioned in issue #413047

mentioned in merge request !135277 (closed)

mentioned in issue #417778 (closed)

@jeldergl

Along those lines, perhaps the ✓ isn't the right indicator, but something closer to nav might be

FWIW I like the distinction of ✓ (or some styling) vs having a nav style indicator because the "page" you are on is dependent on a number of different selected states of the group/project/object, the nav item, the various filter/sort options, selected tab, and others. If we begin to indicate "states" consistent with Nav items it makes it feel like those other items should be marked as selected and I think that gets out of hand quickly.

Support links in Combobox Pajamas component

Designs

Child items ...

Activity

Support links in Combobox Pajamas component

Blocks

Activity