nhsuk-frontend icon indicating copy to clipboard operation
nhsuk-frontend copied to clipboard

Rework clickable cards using CSS invisible overlay rather than JS event handler

Open andymantell opened this issue 2 years ago • 12 comments

Description

Rework clickable cards using CSS invisible overlay rather than JS event handler

This avoids problems with using Ctrl-click, middle click, right click to open new tabs since the link just behaves as it would natively

Fixes #758 Fixes #749

Checklist

Issues observed during browser testing

  • ~IE10 - image is not clickable in "card with an image" example. Ignoring, because IE10 is not supported and the experience is ok.~ Fixed. Don't know whether we want IE10 fixes lingering around, but it's relatively inoffensive, so might not hurt to have it for a while? Easily removed if we don't want it...

andymantell avatar Aug 03 '21 13:08 andymantell

@Fenwick17 as you did the original work with the clickable Card, are you OK to take a look at this? I'll also give it a review

chrimesdev avatar Aug 05 '21 09:08 chrimesdev

Spotted one issue that occurs with MacOS Hover Text. The large-text view does not work for the content of the card due to the overlay, hovering the heading will display the large-text view, but not the body content.

Everything else appears to be working as you would expect.

Fenwick17 avatar Aug 05 '21 10:08 Fenwick17

Thanks @Fenwick17 - do you mean these ones?

image

andymantell avatar Aug 05 '21 10:08 andymantell

Sorry, I see what you mean now. Using https://support.apple.com/en-gb/guide/mac-help/mchlb203bc78/mac

Let me have a think about this 🤔

andymantell avatar Aug 05 '21 10:08 andymantell

Added a gif of the issue, if it can't be resolved we will have to make a decision on whether it is acceptable as it is. I would image any Windows software that does this would have a similar behaviour.

I believe NVDA with mouseover will also not read the body content due to the overlay, but I haven't tested that yet.

Hover text not display on hover

Fenwick17 avatar Aug 05 '21 11:08 Fenwick17

yeah makes sense. I'm on windows so I'll see if there's anything similar here. And will have a think if there's anything sensible can be done

andymantell avatar Aug 05 '21 11:08 andymantell

@chrimesdev @Fenwick17 I'm not sure what to do about this. Surely anything that alters what HoverText "sees" would also affect what a screenreader sees. So if we start changing things for HoverText, we would negatively affect other tools.

I don't know if a perfect solution exists for this then. I think we're stuck choosing between two slightly imperfect solutions:

  1. Go back to the JS way of doing this, and try and enhance it for more ways to open new tabs (Ctrl-Click, middle click etc). This will never catch all the ways though (right click / long press on mobile to open new tab from the context menu for example)

  2. Go with the invisible overlay, and accept the hovertext deficiency.

Any thoughts? Have I missed any other options?

andymantell avatar Aug 23 '21 09:08 andymantell

I just performed some Browserstack testing and didn't spot any issues with the CSS only solution. So the only issues we seem to have are not being able to copy text and no hover text on the paragraph.

For both of these I personally don't think they are show stoppers.
The text is meant to just be a little bit of extra context so there shouldn't really be a need to copy it.
The link itself ideally should be clear enough for a user to know what it means without needing the additional text and standard zoom behaviour will still work as a fallback.

The only thing that comes to mind at the moment is if we add visually hidden content to the link itself, so that it will show up with Hover Text.

<a class="nhsuk-card__link" href="./pages/examples.html">
  Examples<span class="nhsuk-u-visually-hidden">: Page layouts, text pages and components</span>
</a>

Then you would have to use aria-hidden on the <p>. This is probably wildly overkill but I thought it was a cool workaround.

What do you think? Overall, I think we can proceed with what Andy has done.

Fenwick17 avatar Sep 22 '21 14:09 Fenwick17

@Fenwick17 as you have done more work on this than I have, I am happy to go with what you think is best.

chrimesdev avatar Oct 21 '21 08:10 chrimesdev

@andymantell do you think this needs a little more information in the CHANGELOG around removing the JavaScript for the Card component? If anyone is importing and initialising the Card using ES6 and we have now removed it, would it be a breaking change rather than a fix?

chrimesdev avatar Oct 21 '21 12:10 chrimesdev

@chrimesdev Agreed. Done - moved it to 6.0.0 and added a note about removing JS

andymantell avatar Oct 21 '21 12:10 andymantell

Thanks Andy, as this is a breaking change we will hold off merging this PR and merge/release with #740

chrimesdev avatar Oct 21 '21 12:10 chrimesdev