department-of-veterans-affairs / vets-design-system-documentation

Repository for design.va.gov website
https://design.va.gov
36 stars 55 forks source link

Breadcrumb's current page styling should not remove pointer events #1458

Open joshkimux opened 1 year ago

joshkimux commented 1 year ago

Bug Report

What happened

The current page in the breadcrumb responds to keyboard commands

Screen Shot 2023-01-23 at 7 24 08 AM

If a user were to hit enter, it will refresh the page.

https://user-images.githubusercontent.com/14154792/214039052-101ebfdb-b3a8-4160-9405-85ac8332d5ac.mov

But it lacks a hand cursor on hover and does not respond on click

https://user-images.githubusercontent.com/14154792/214045045-9c5b4cb3-5dd6-459d-bc7a-677d77fc7a83.mov

The hand cursor (which is native to links) appears on hover. However, the aria current styling removes pointer events and the hand cursor.

What I expected to happen

Following W3C's example, we shouldn't hide the cursor or block pointer events as it creates an inconsistency between the keyboard and hover experience.

https://user-images.githubusercontent.com/14154792/214045410-fb856179-7aa1-4a4a-a8bc-2b9f91073fb8.mov

This can be accomplished fairly easily by simply removing:

Some nuance

That being said, there remains to be some argument in the a11y community over whether or not the current page should (or shouldn't) be a link. That's definitely a conversation for another time, but for now, I think reaching a state of consistency between various navigation mechanisms (mouse vs. keyboard) is a solid step forward.

Steps to reproduce:

  1. View breadcrumbs on storybooks
  2. Tab to current page, confirm it takes focus and responds to enter
  3. Hover over current page, confirm it does not show a hand cursor and does not respond on click

Urgency

How urgent is this request? Please select the approriate option below and/or provide details

humancompanion-usds commented 1 year ago

That being said, there remains to be some argument in the a11y community over whether or not the current page should (or shouldn't) be a link.

Can you explain the rationale for why it should be a link? It's not a link in the USWDS Breadcrumb and I'm inclined to follow suit.

joshkimux commented 1 year ago

@RLHecht tagging you here for awareness! This question is a bit more complex than it seems and will need some discovery work. Could we create a ticket for this?

coforma-terry commented 1 year ago

Josh - created a ticket - #53331

rsmithadhoc commented 9 months ago

Hi @joshkimux, @humancompanion-usds, @laflannery,

I just started on the DST team and I was looking into a pagination issue, which led me to stumble upon this breadcrumb issue. I think the patterns are similar and should behave consistently, and I'd also like to bring the conversations together.

Recap of issues:

I'm leaning towards having the current page for both of these components be clickable (with the hand cursor) and focusable. These are navigational elements like a navigation bar and we don't typically hide or disable a particular device from clicking on/focusing on an item there just because they are already on that page.

Can anyone think of a reason these should not be links that are clickable/focusable, with the correct indicator that they are the current page?

humancompanion-usds commented 9 months ago

@rsmithadhoc - I cannot think of a reason. Ping the others on Slack if you don't get a response here. We have an opportunity to tell the USWDS team about the breadcrumb issue directly in our monthly calls.

laflannery commented 9 months ago

@rsmithadhoc and @humancompanion-usds I think my only concern with doing this is the possible difference in styling, especially the breadcrumbs. Right now, the current page breadcrumb is not styled as a link and therefore it's a bit confusing when it receives focus and acts like a link - because it's unexpected. If we move forward with this, could we consider the styling of the current page breadcrumb (and page) so that it's a bit more aligned with standard link styling and not as surprising that it is in fact a link?

rsmithadhoc commented 9 months ago

Thanks @laflannery, I agree, it should look like a link if we go that route. I'll check with the team on that styling change.

In the meantime, I'll work on fixing the issue with the link being clickable.

rsmithadhoc commented 9 months ago
Screenshot showing current breadcrumb and the proposed change
humancompanion-usds commented 9 months ago

Hmm. I see the discovery ticket (#53331) that was raised the last time this came up doesn't have much traction. I have some observations:

  1. Potential for information entered to be lost. For sighted users, having the current page represented as a link was historically thought to be confusing because it may raise the question of whether or not one is at the final level (i.e. am I at this level/page or is there another level/page?) I do wonder if making it a link would cause users to click on it thereby potentially reloading the page. IF we make the breadcrumbs in forms flows simply a back link then I'm not concerned (and we are working towards that with a test of that slated for early next month). We could also make it an anchor link to the h1 of the page. But currently that is not the case and thus clicking the current link would discard any data entered into a form, which is a cardinal sin worse than, for example, the focus state being odd.

  2. Lack of consensus. There was a suggestion in the thread that Josh captured in #53331 that the current page should just not be included at all. This makes me think there may not be consensus yet and that an A/B test, at minimum, is in order.

Changing a USWDS component requires some research with users to back it up. Alternatively, you could attempt to convince their a11y folks that this is just a bug and inconsistent with the pagination guidance and thus a change we should make now. As I mentioned, we can bring it up with. At the moment I'm more inclined to let the team that raised this finish their discovery and conduct a test before we create a PR to USWDS. We have opinions but no research with users to conclude that this change is superior to the current design.

rsmithadhoc commented 9 months ago

Thanks @humancompanion-usds, fair enough, let's wait for discovery from the team that raised this to give us a solid path forward and avoid rework. I'll hold off on changes here.

caw310 commented 9 months ago

Moving to backlog to be worked on in the future.

humancompanion-usds commented 8 months ago

@caw310 - When we moved this to the backlog we failed to move 2068, 2117, and 2143 so now we've already gone and done this. I'm going to go ahead and update the guidance to match what we've done rather than have us undo this. We may in fact have resolved to do this over Slack, but that's not captured here. Regardless, in future we need to be more careful when our issues are split into many issues and we pause on something that we pause across the board. We probably need to anoint an issue as the main issue and then have others tied to it.