Fixup :visited:focus specificity

[janbrasna]

One-line summary

Follows up from #15802

Significant changes and points to review

Anchor brand colors are not inverted in @mixin invert-colors for dark theme, and the blues and greens end up not readable on dark backgrounds. That is mostly overridden to blacks=whites, but here and there some of the original rules end up being higher specificity (or serving a newer color with css variable, but the older via hardcoded values); so this combination needs a tweak.

Only fixing this for css variables should not be an issue, as the dark theme inversion is also done via css variables — so UAs only using hex colors instead of vars won't use dark theme anyways.

Issue / Bugzilla link

Fixes #15800

Testing

http://localhost:8000/
http://localhost:8000/about/

(doesn't reproduce locally 🤷 so pasted the generated m24-base into www-dev to test there…)

https://www-demo1.allizom.org/
https://www-demo1.allizom.org/about/

[alexgibson]

I'm deploying this to a demo instance so we can be more confident this fix works before merging.

[alexgibson]

This seems to work now on demo for me, but can you also confirm @janbrasna? https://www-demo1.allizom.org/en-US/

[alexgibson]

One additional thought:

I wonder if adding more link state psudeo styles to .m24-c-cta might help give it more specificity in general? It has active, and focus-visible overrides currently, but falls back to the generic link styles for other states which could be brittle. Thoughts?

[janbrasna]

I'm actually writing that up to the issue ^^.

This was the last pseudo combo not having enough specificity to override the former protocol base (didn't do the math exactly as to why, it had to do something with how SASS→CSS compiled the nested rules, &-appending in what order etc.), I also checked those that get picked up from CTA styles (because otherwise the :active would also not apply) — so I'm actually thinking the opposite: Whether the base color overrides should not be self sufficient, and not rely on following selectors to match — i.e. whether it would be more resilient to also set the other :active or :focus-visible on the base elements (checking that as we speak…)

[janbrasna]

OK, basically it needs to wholesale redefine these, exactly: https://github.com/mozilla/protocol/blob/121dec9f988c8b8bef5962155ef6f6841244e0bc/assets/sass/protocol/base/elements/_links.scss#L33-L40

So I'm adding the :active there for the exact same reasons. (Whenever used outside of CTAs, for good measure.)

[alexgibson]

Hmm yeah, there are multiple layers of overrides here now with each written a little differently.

I actually wonder why we need to load protocol-mozilla-2024 on the redesigned pages at all. It exists for the nav / footer, but is that only to support older style pages?

[alexgibson]

I actually wonder why we need to load protocol-mozilla-2024 on the redesigned pages at all.

Hrm, digging into this file, there's a LOT of redundant CSS in here that the new site doesn't use. But I can't be confident other things wouldn't break if we just removed it. I think it's worth filing a followup issue to investigate more cleanly separating the new / old site.

[alexgibson]

Pushing your latest commit to demo ^

[janbrasna]

I'm afraid that's above my paygrade, it all comes from the point where rebrand, nav refresh, type refresh, and layout/pages refresh were planned to be launched separately, behind distinct switches, so there was a LOT of overlap once all the pieces started to come together — but yeah, it should all be in theory self-standing today(?), at least as per #15120…?

(However as e.g. #15790/files shows, there's a lot of little differences between the two… so yea, it's a bit wild;D)

[alexgibson]

Yes, this is a larger issue that our internal team needs a discussion on first, as it also likely depends on our longer term plan for Protocol 🙂

[janbrasna]

BTW there's also this bit #15569/files to complicate specificity at times, too;) So that's all the places one has to take into account, when touching similar rules.

[janbrasna]

@alexgibson Thanks for the deploy trigger, appreciate it. It does indeed look fixed to me, (although the Cloud Run might also have different bfcache behavior compared to other envs, I can positively reproduce on other demos but not on demo1, so that looks resolved).

I'll leave that to you whether you'd like Daniel to also confirm fixed on demo1 first, or push that to dev and have it confirmed there.

(I'm just quickly trying to infer the impact of the added :active change, but my feeling is there should be no adverse effects, quite the opposite — if there were any more gaps, this should help rectify that.)

[alexgibson]

Let's merge this to www-dev and Daniel can test from there. Agree this should be pretty safe, since the changes increase specificity 👍

[janbrasna]

👍 OK, hopefully the four-keys queue will start moving at some point, it seems stuck at a rev from yesterday.

[alexgibson]

It looks like our last deploy failed from yesterday, but this latest one seems to be running fine so far. Should be up soon

[janbrasna]

Confirmed. It slowly propagates but seems to start serving f5193cd in response. Thanks for the assistance Alex. 🚀

[janbrasna]

Notes:

1. Even though some of the more specific definitions for the same exist on the component classes, I did not remove them even when they may look superfluous now — as they were authored with some reasoning behind it, and the base pseudos added here only target ‹a› links, so there's still some utility in case the CTAs would include buttons and similar pointer event targets.

2. Quickly tested UAs with no css var support (IE10, 11, Safari 5) and no link color issues there to compromise legibility.