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

Append card title to action links inside of a Summary Card #3961

Merged
merged 1 commit into from
Jul 27, 2023

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Jul 14, 2023

It was raised in the recent DAC audit that pages with multiple Summary Cards had the potential to fail the Level A criterion Link Purpose (In Context) and the Level AAA criterion Link Purpose (Link Only), due to the repetition of links with identical labels—unless the implementor went out of their way to manually provide the required distinctions.

This PR modifies the Nunjucks template for Summary Lists so that, if a card title has been provided, the title is automatically appended to any action links within the card. This is applied to both the card-level and individual row's action links.

This PR resolves #3673 add #3674.

Changes

  • Updated the component's internal _actionLink macro now takes a second, optional cardTitle parameter.
  • Updated instances of _actionLink to pass through the params.card.title object, if it exists.

Thoughts

  • This is currently hardcoding a content order (and parentheses) that might not be preferable in all localisations. I'm hoping that, as two distinct pieces of information that are intended as separate phrases, this might be okay, but I'm not sure.
  • Piping params.card.title.html into a visually-hidden span feels a bit weird and like it may have adverse side effects depending on what HTML is provided, however we have no guarantee that params.card.title.text is set in all situations.

@querkmachine querkmachine self-assigned this Jul 14, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 14, 2023 12:37 Inactive
@querkmachine querkmachine force-pushed the summary-card-title-in-action-links branch from 43fbd17 to 90b03a3 Compare July 14, 2023 13:06
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 14, 2023 13:06 Inactive
@querkmachine querkmachine force-pushed the summary-card-title-in-action-links branch from 90b03a3 to 09cd0d7 Compare July 14, 2023 14:08
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 14, 2023 14:08 Inactive
@querkmachine querkmachine requested a review from a team July 14, 2023 15:08
@querkmachine querkmachine marked this pull request as ready for review July 14, 2023 15:08
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Looks brill from a code point of view

Do we know if this is best approach?

Versus using the <h2> govuk-summary-card__title with aria-describedby?

@querkmachine
Copy link
Member Author

querkmachine commented Jul 20, 2023

That certainly seems like a possibility that might work in practice (assuming the AT consistently announces aria-describedby), though I'm not sure if doing that is compliant with the listed WCAG criteria, as the link text itself would not be unique and would appear identical in contexts like VoiceOver's links view.

CC @davidc-gds in case you have a view on this.

@dav-idc
Copy link

dav-idc commented Jul 21, 2023

Hi @querkmachine from a information hierarchy standpoint, would it make sense to put the unique element (the action) first, and then the card title second?

We can take the 'Summary list as a summary card with actions plus summary list actions' PR preview example as a case study.

Right now it looks to be read out as:

"Undergraduate teaching assistant: Delete job history"
"Undergraduate teaching assistant: Withdraw job history"

I'm suggesting that since the unique element is the action name, we should reverse the order of the items, like so:

"Delete job history: Undergraduate teaching assistant"
"Withdraw job history: Undergraduate teaching assistant"

Thoughts , possibly from @claireashworth?

@dav-idc
Copy link

dav-idc commented Jul 21, 2023

That certainly seems like a possibility that might work in practice (assuming the AT consistently announces aria-describedby), though I'm not sure if doing that is compliant with the listed WCAG criteria, as the link text itself would not be unique and would appear identical in contexts like VoiceOver's links view.

CC @davidc-gds in case you have a view on this.

I tested the aria-describedby option in a few screen readers, and generally it looked like it worked for some and didn't for others. I think to be robust we can go ahead with the govuk-visually-hidden currently in use here.

I tested an aria-describedby version with the VoiceOver 'rotor' list of links, and it didn't show the describedby piece in the link label. I couldn't test the 'list of links' features in NVDA or JAWS quick enough, mainly because my computer doesn't have an insert key, and I couldn't troubleshoot how to open the list in the time I had.

So in conclusion:

  • this current govuk-visually-hidden method works in all 3 screen readers I tested
  • the aria-describedby was hit and miss, but we'd be fine using it as its totally valid use of that ARIA attribute

The only consistent downside for govuk-visually-hidden is that it always makes copy/pasting the content a real mess:

Copy/paste using govuk-visually-hidden:

Undergraduate teaching assistant
Undergraduate teaching assistant:Delete job history
Undergraduate teaching assistant:Withdraw job history
Name Firstname Lastname
Undergraduate teaching assistant:EditnameUndergraduate teaching assistant:Deletename
Date of birth 13/08/1980 Undergraduate teaching assistant:Changedate of birth

Without using govuk-visually-hidden:

Undergraduate teaching assistant
Delete job history
Withdraw job history
Name Firstname Lastname
Edit Delete
Date of birth 13/08/1980 Change

@querkmachine
Copy link
Member Author

@davidc-gds Swapping the order makes sense to me.

The text-selection problem makes me wonder whether we want to set user-select: none on visually hidden text, but that's probably a topic for wider discussion.

@claireashworth
Copy link
Contributor

Having the action first makes sense to me as well.

@querkmachine querkmachine force-pushed the summary-card-title-in-action-links branch from 09cd0d7 to 2d8f726 Compare July 24, 2023 10:48
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 24, 2023 10:48 Inactive
@querkmachine
Copy link
Member Author

I've pushed changes to place the card title after the action name.

I've also added user-select: none to visually-hidden elements for the time being, so that it can be AT tested in situ.

@querkmachine querkmachine force-pushed the summary-card-title-in-action-links branch from 2d8f726 to c086b81 Compare July 24, 2023 11:03
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 24, 2023 11:03 Inactive
@dav-idc
Copy link

dav-idc commented Jul 24, 2023

@querkmachine I may be doing it wrong, but I can still copy/paste the hidden text, even with user-select: none applied to govuk-visually-hidden on the PR deployment.

I'm using cmd + c and cmd+ v on a Mac, on Chrome.

@querkmachine
Copy link
Member Author

querkmachine commented Jul 24, 2023

^ We found that David's macOS is configured is to paste with formatting by default. This doesn't appear to be the default behaviour in macOS, needing to be configured in Settings → Keyboard → Keyboard Shortcuts... → App Shortcuts.

This does not happen in Firefox or Safari, so it seems to be a case of Chromium browsers purposefully ignoring the CSS rule. As this is a configuration of the user's environment, we probably can't practically do anything about it.

Pasting without formatting omits the visually-hidden text as expected.


Having tested in VoiceOver/Safari, NVDA/Firefox and JAWS/Chrome, it doesn't appear that making hidden text unselectable has had any side-effects to how screen readers announce or interact with it.

@querkmachine querkmachine changed the title Prepend card title to action links inside of a Summary Card Append card title to action links inside of a Summary Card Jul 24, 2023
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for checking we picked the best solution 😊

Some comments on HTML output but see what you think

@querkmachine querkmachine force-pushed the summary-card-title-in-action-links branch from c086b81 to b09b24d Compare July 27, 2023 12:22
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3961 July 27, 2023 12:22 Inactive
@querkmachine querkmachine merged commit fdff8c5 into main Jul 27, 2023
@querkmachine querkmachine deleted the summary-card-title-in-action-links branch July 27, 2023 13:10
colinrotherham pushed a commit that referenced this pull request Jul 27, 2023
…inks

Append card title to action links inside of a Summary Card
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
github-merge-queue bot pushed a commit to x-govuk/govuk-components that referenced this pull request Dec 21, 2023
This change appends the card title to action links when rendered within
a summary card. It follows [the upstream
change](alphagov/govuk-frontend#3961) introduced
in govuk-frontend version 5.

In the simplest case it works like the reference implementation:

```erb
<%= govuk_summary_list(card: { title: "Shiny new widget" }) do |sl| %>
  <%= sl.with_row do |r| %>
    <%= r.with_key(text: "Price") %>
    <%= r.with_value(text: "£19.99") %>
    <%= r.with_action(href: "/widgets/1/edit", text: "Modify", visually_hidden_text: "this widget's price") %>
  <% end %>
<% end %>
```

Here, the rendered HTML will now include the card title in the visually
hidden text on the action:

```html
<dl class="govuk-summary-list">
  <div class="govuk-summary-list__row">
    <dt class="govuk-summary-list__key">Price</dt>
    <dd class="govuk-summary-list__value">£19.99</dd>
    <dd class="govuk-summary-list__actions">
      <a class="govuk-link" href="/a">Modify<span class="govuk-visually-hidden"> this widget's price (Shiny new widget)</span></a>
    </dd>
  </div>
</dl>
```

Additionally the suffix text can be set manually. This allows it to be
set without passing in `card: { ... }` when initialising the summary
list. It might be useful when rendering the summary list within an outer
card. It is set using the `action_suffix` keyword.

```erb
<%= govuk_summary_card(title: "Shiny new widget") do %>
  <%= govuk_summary_list(action_suffix: "Shiny new widget") do |sl| %>
    <%= sl.with_row do |r| %>
      <%= r.with_key(text: "Price") %>
      <%= r.with_value(text: "£19.99") %>
      <%= r.with_action(href: "/widgets/1/edit", text: "Modify", visually_hidden_text: "this widget's price") %>
    <% end %>
  <% end %>
<% end %>
```

This will output the same HTML as the above example.

When both the `card` and `action_suffix` arguments are provided the
`action_suffix` takes precedence, which offers a little more flexibility
as there might be some cases other text would make more sense than the
card's title.

Fixes #479, #480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants