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: revert useless a:any-link:hover change #268

Merged
merged 3 commits into from
Jul 13, 2022

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Jul 13, 2022

Revert half of #266

a:any-link:hover change is not necessary and was based on an artificial usage of <a className="xyz"> which we don't have in practice (we use <span> in such cases)


Former issue text before reverting

Fix issue where all links have unexpected underlines:

CleanShot 2022-07-13 at 12 20 00@2x

Docusaurus issue: facebook/docusaurus#7748

See comment https://github.com/facebookincubator/infima/pull/266/files#r917208453

Alternative PR: #267

Support for :where is good enough and we already use it anyway: https://caniuse.com/?search=where

It's a good new tool to keep CSS specificity low

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 13, 2022
@netlify
Copy link

netlify bot commented Jul 13, 2022

Deploy Preview for infima ready!

Name Link
🔨 Latest commit 0de9584
🔍 Latest deploy log https://app.netlify.com/sites/infima/deploys/62cea7b7d2b0ca00097fa9ee
😎 Deploy Preview https://deploy-preview-268--infima.netlify.app/demo
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Jul 13, 2022

Size Change: -3.34 kB (-1%)

Total Size: 558 kB

Filename Size Change
./packages/core/dist/css/default-dark/default-dark-rtl.css 80.5 kB -500 B (-1%)
./packages/core/dist/css/default-dark/default-dark-rtl.min.css 57.4 kB -334 B (-1%)
./packages/core/dist/css/default-dark/default-dark.css 80.5 kB -500 B (-1%)
./packages/core/dist/css/default-dark/default-dark.min.css 57.3 kB -334 B (-1%)
./packages/core/dist/css/default/default-rtl.css 78.4 kB -500 B (-1%)
./packages/core/dist/css/default/default-rtl.min.css 56.2 kB -334 B (-1%)
./packages/core/dist/css/default/default.css 78.4 kB -500 B (-1%)
./packages/core/dist/css/default/default.min.css 56.2 kB -334 B (-1%)
ℹ️ View Unchanged
Filename Size
./packages/core/dist/js/alerts.js 409 B
./packages/core/dist/js/alerts.min.js 409 B
./packages/core/dist/js/button-groups.js 267 B
./packages/core/dist/js/button-groups.min.js 267 B
./packages/core/dist/js/dropdowns.js 1.01 kB
./packages/core/dist/js/dropdowns.min.js 1.01 kB
./packages/core/dist/js/menu.js 2.4 kB
./packages/core/dist/js/menu.min.js 2.4 kB
./packages/core/dist/js/navbar.js 1.08 kB
./packages/core/dist/js/navbar.min.js 1.08 kB
./packages/core/dist/js/pills.js 270 B
./packages/core/dist/js/pills.min.js 270 B
./packages/core/dist/js/radio-behavior.js 705 B
./packages/core/dist/js/radio-behavior.min.js 705 B
./packages/core/dist/js/tabs.js 267 B
./packages/core/dist/js/tabs.min.js 267 B

compressed-size-action

@github-actions
Copy link

github-actions bot commented Jul 13, 2022

Dist CSS Diff

diff --git a/packages/core/dist/css/default/default.css b/packages/core/dist-branch/css/default/default.css
index f258e29..52e3ab6 100644
--- a/packages/core/dist/css/default/default.css
+++ b/packages/core/dist-branch/css/default/default.css
@@ -1233,25 +1233,7 @@ a {
   transition: color var(--ifm-transition-fast) var(--ifm-transition-timing-default);
 }
 
-a:link:hover, a:visited:hover {
-    color: var(--ifm-link-hover-color);
-    /* autoprefixer: ignore next */
-    text-decoration: var(--ifm-link-hover-decoration);
-  }
-
-a:-webkit-any-link:hover {
-    color: var(--ifm-link-hover-color);
-    /* autoprefixer: ignore next */
-    text-decoration: var(--ifm-link-hover-decoration);
-  }
-
-a:-moz-any-link:hover {
-    color: var(--ifm-link-hover-color);
-    /* autoprefixer: ignore next */
-    text-decoration: var(--ifm-link-hover-decoration);
-  }
-
-a:any-link:hover {
+a:hover {
     color: var(--ifm-link-hover-color);
     /* autoprefixer: ignore next */
     text-decoration: var(--ifm-link-hover-decoration);

@Josh-Cena
Copy link
Contributor

Josh-Cena commented Jul 13, 2022

Thought about it, but we may expect more complaints like facebook/docusaurus#7617 coming in 😅 I don't have objections to this, but I wonder what the purpose of changing the style for all a elements in #266 was?

Edit: I see this:

You can have <a> elements that don't represent links (like headings). It usually only makes sense to display hover styles if the <a> represents a link.

Not sure if a11y experts will accept the idea of "non-link <a> tags" at all. In Docusaurus we render it as a <span> if there's no href. What about just reverting this change?

@slorber
Copy link
Collaborator Author

slorber commented Jul 13, 2022

Thought about it, but we may expect more complaints like facebook/docusaurus#7617 coming in 😅

Looks like it may not be a big deal in this specific case if some users have an inappropriate :hover effect.

I don't have objections to this but I wonder what the purpose of changing the style for all a elements in #266 was?

Yes, I think it may not be useful in the first place 😅 thanks for noticing that

The created demo looks artificial and does not match what we have on the Docusaurus side.

I've run this query on many Docusaurus pages and couldn't find a single usage: document.querySelectorAll("a:not(:any-link)") (and ensured the query actually works with a manual test for sure)

@srmagura I'm going to revert this change and adapt the "artificial" example ensure that breadcrumbs without links are not using <a> without href

@slorber slorber changed the title fix: use :where(:any-link) to avoid increasing CSS specificity in a:any-link:hover fix: revert useless a:any-link:hover change Jul 13, 2022
@slorber
Copy link
Collaborator Author

slorber commented Jul 13, 2022

If the a element has no href attribute, then the element represents a placeholder for where a link might otherwise have been placed, if it had been relevant.

https://www.w3.org/TR/2011/WD-html5-author-20110809/the-a-element.html

I guess we can assume we'll never have this case in the future, but may revisit later the introduction of :where() if needed, as it still makes sense technically 🤷‍♂️

Wonder if the breadcrumb items without links can be interpreted as "placeholder" 🤪

Note we have Infima demo items using <a> without links so I'll also clean these up to match Docusaurus HTML structure

@Josh-Cena
Copy link
Contributor

Maybe a11y gurus like @BenDMyers has more insight on this 😄

We do have the jsx-a11y rule that forbids this case:

The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but still need the element to resemble a link, use a button and change it with appropriate styles. Learn more: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/HEAD/docs/rules/anchor-is-valid.md

@slorber
Copy link
Collaborator Author

slorber commented Jul 13, 2022

Will merge this for now, looks good enough but let's continue the discussion

Note that we have some cases of <a href="#"> in Docusaurus too, wonder if those are legit usages

document.querySelectorAll("a[href='#']")

CleanShot 2022-07-13 at 13 23 27@2x

@slorber slorber merged commit 818ee9b into main Jul 13, 2022
@slorber slorber deleted the slorber/fix-any-link-specificity-issue branch July 13, 2022 11:25
@srmagura
Copy link
Contributor

Thanks @slorber !!! Seems like I made this change based on the Infima demo site having an example which does not actually occur in practice. Sorry for the regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants