Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#56928 closed defect (bug) (fixed)

Twenty Twenty-Three: visited state of button links use the incorrect text color

Reported by: mikachan's profile mikachan Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.1
Component: Themes Keywords: has-patch commit fixed-major
Focuses: css Cc:

Description

When both the visited and hover states are active on a button, the visited text color overrides the hover text color, which means the two colors are not contrasting in the default style variation.

https://cldup.com/i8uPleQS_3.png
The second image has both a :visited and :hover state.

Video example: https://user-images.githubusercontent.com/2207451/198293193-958030d2-5817-4313-bb2d-be65bfdb8f36.mov

We may be able to fix this by adding an !important tag to the hover text color property, although this isn't ideal.

Attachments (1)

56928.patch (613 bytes) - added by sabernhardt 2 years ago.
moves :visited to the beginning of the array

Download all attachments as: .zip

Change History (19)

#1 @sabernhardt
2 years ago

  • Focuses css added

Moving :visited before the :hover and :focus styles in global styles should be simpler.

Last edited 2 years ago by sabernhardt (previous) (diff)

#2 @davidbaumwald
2 years ago

  • Milestone changed from Awaiting Review to 6.1.1

This would be good to address in 6.1.1.

#3 @mikachan
2 years ago

Moving :visited before the :hover and :focus styles in global styles should be simpler.

Yes, I thought so too, but this doesn't seem to work in my testing... Perhaps this ordering should be addressed in Gutenberg.

This would be good to address in 6.1.1.

Ok!

@sabernhardt
2 years ago

moves :visited to the beginning of the array

#4 @sabernhardt
2 years ago

  • Keywords has-patch added

GB34448 seems to be the same issue.

For Twenty Twenty-Three, I was able to fix it by changing the order of pseudo-classes in the VALID_ELEMENT_PSEUDO_SELECTORS array.

#5 @mikachan
2 years ago

For Twenty Twenty-Three, I was able to fix it by changing the order of pseudo-classes in the VALID_ELEMENT_PSEUDO_SELECTORS array.

Nice, thank you! This patch looks good and works well for me.

Sorry, in your earlier comment I thought you meant the ordering in the theme's theme.json file (which is what I had tried).

Last edited 2 years ago by mikachan (previous) (diff)

#6 @sabernhardt
2 years ago

I had said theme.json in that comment and then discovered my mistake :)

#7 @mukesh27
2 years ago

  • Keywords commit added

Thanks @mikachan for the ticket and @sabernhardt for the patch.

56928.patch LGTM and ready for commit.

#8 @mikachan
2 years ago

Great! Would this need an upstream PR in the Gutenberg GitHub repository as well, to reflect the fix in this patch?

#9 @sabernhardt
2 years ago

  • Component changed from Bundled Theme to Themes

@mikachan I think the plugin should be fixed, too, if you would like to create a PR.

I'm changing the component since the proposed patch could affect more themes. Note: GB34448 is related but not exactly the same (so "Fixes: 34448" might not be appropriate).

#10 @mikachan
2 years ago

Thanks for confirming, I've opened a PR here: https://github.com/WordPress/gutenberg/pull/45559

This ticket was mentioned in PR #3582 on WordPress/wordpress-develop by @Mamaduka.


2 years ago
#11

Backports changes from https://github.com/WordPress/gutenberg/pull/45559

The order of the selectors should be: visited, hover, focus, active. This is to ensure that 'visited' has the lowest specificity and the other selectors can always overwrite it.

Props: @mikachan, @sabernhardt, @getdave (get_dave).

Trac ticket: https://core.trac.wordpress.org/ticket/56928

@desrosj commented on PR #3582:


2 years ago
#12

Thanks @Mamaduka! I changed the ticket description to point to Trac-56928 since that ticket is specific to this issue/change.

@Mamaduka commented on PR #3582:


2 years ago
#13

Thank you, @desrosj. The Trac ticket link makes sense to me, I will port changes back to the plugin for consistency 👍

#14 @peterwilsoncc
2 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 54774:

Themes: Re-order valid link pseudo classes.

Re-order the link pseudo classes to follow the long term LoVe (F)HA rule when set via theme.json.

In order that the CSS cascade behaves in a predictable manner, it's recommended that the selectors follow the order :visited, :focus/:hover, :active. As order affects the specificity, this ensures the interaction states override the visited states. CSS specificity is really quite beautiful, although complex.

Props mikachan, sabernhardt, davidbaumwald, mukesh27, Mamaduka, desrosj.
Fixes #56928.

#16 @peterwilsoncc
2 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging to the 6.1 branch.

@Mamaduka commented on PR #3582:


2 years ago
#17

Thank you, @peterwilsoncc.

Now the only thing left is to cherry-pick the commit for the 6.1 branch.

#18 @desrosj
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54778:

Themes: Re-order valid link pseudo classes.

Re-order the link pseudo classes to follow the long term LoVe (F)HA rule when set via theme.json.

In order that the CSS cascade behaves in a predictable manner, it's recommended that the selectors follow the order :visited, :focus/:hover, :active. As order affects the specificity, this ensures the interaction states override the visited states. CSS specificity is really quite beautiful, although complex.

Props mikachan, sabernhardt, davidbaumwald, mukesh27, Mamaduka, desrosj, peterwilsoncc.
Fixes #56928.

Note: See TracTickets for help on using tickets.