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

Fix accordion underline hover state being removed when hovering plus/minus symbol #1778

Merged
merged 2 commits into from
Mar 31, 2020

Conversation

36degrees
Copy link
Contributor

@36degrees 36degrees commented Mar 31, 2020

At present, when you hover over a section the section title gets the affordance of an underline to show which element is being interacted with – except if you hover over the plus/minus 'control', in which case the underline disappears again.

This happens because the icon is appended to the heading, rather than the button, and so the button and icon are siblings, and the icon being absolutely positioned overlays the button.

(The only reason that you can click the control to toggle the section is that the click handler is bound to the heading, rather than the button. If the click handler was bound to the button, clicking the icon would not work.)

Append the icon to the button rather than the heading, which makes it part of the button, and thus hovering the icon will correctly trigger the button hover styles.

Browser testing

  • IE8 (Windows) – does not get the hover state, but this matches the existing behaviour
  • IE9 (Windows)
  • IE10 (Windows)
  • IE11 (Windows)
  • Edge (Windows)
  • Chrome (Windows)
  • Firefox (Windows)
  • Safari (macOS)
  • Chrome (macOS)
  • Firefox (macOS)
  • Safari (iOS)
  • Chrome (iOS)
  • Chrome (Android)
  • Samsung Internet (Android)

Fixes #1769

At present, when you hover over a section the section title gets the affordance of an underline to show which element is being interacted with – except if you hover over the plus/minus 'control', in which case the underline disappears again.

This happens because the icon is appended to the heading, rather than the button, and so the button and icon are siblings, and the icon being absolutely positioned overlays the button.

(The only reason that you can click the control to toggle the section is that the click handler is bound to the heading, rather than the button. If the click handler was bound to the button, clicking the icon would not work.)

Append the icon to the button rather than the heading, which makes it part of the button, and thus hovering the icon will correctly trigger the button hover styles.
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1778 March 31, 2020 15:22 Inactive
@36degrees 36degrees changed the title Append icon to accordion button, not heading Fix accordion underline hover state being removed when hovering plus/minus symbol Mar 31, 2020
Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Since the current behaviour relies on the parent having relative positioning then moving it inside another child logically doesnt change the behaviour, only risk I could think of is that the accessible name for the button is changed but that doesnt seem likely given that the icon has aria-hidden=true applied.

So I'm happy to approve given that you are doing cross browser testing.

@36degrees
Copy link
Contributor Author

36degrees commented Mar 31, 2020

This does change the behaviour in Firefox if using a custom colour scheme – the controls end up black (no matter what foreground colour is chosen, it seems) like the rest of the button (which is already an issue – see alphagov/reported-bugs#30):

Before

After

@NickColley
Copy link
Contributor

@36degrees that's unfortunate but I would argue if the darker text was a usability issue in general then that text is still the most important clickable part of the component. So given that we're improving the usability elsewhere feels like a decent compromise to me...

@36degrees 36degrees merged commit da18710 into master Mar 31, 2020
@36degrees 36degrees deleted the accordion-icon-hover-fix branch March 31, 2020 16:00
@36degrees 36degrees mentioned this pull request Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accordion underline on hover is removed when over plus/minus symbol
3 participants