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(CSS): outline doesn't handle some keyword values #299

Merged

Conversation

lesha1201
Copy link
Contributor

@lesha1201 lesha1201 commented Nov 1, 2023

Now outline works well with CSS-wide keywords and all outline-style keywords.

Fixes #296

List of CSS-wide keywords:

outline spec:

outline-style spec:

Now `outline` works well with CSS-wide keywords and all `outline-style` keywords.
Copy link

vercel bot commented Nov 1, 2023

@lesha1201 is attempting to deploy a commit to the Aoyue Team on Vercel.

A member of the Team first needs to authorize it.

Comment on lines +1671 to +1672
const outlineGlobalValuesRegex =
/^(?:inherit|initial|revert|revert-layer|unset)$/i
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These values are applicable for all CSS properties but decided to keep it here while it's used only in this place.

@1aron
Copy link
Member

1aron commented Nov 2, 2023

@lesha1201 You move fast, and I was just about to fix that issue 😂

@1aron 1aron self-requested a review November 2, 2023 08:18
@1aron 1aron added bug Something isn't working css labels Nov 2, 2023
@lesha1201
Copy link
Contributor Author

@lesha1201 You move fast, and I was just about to fix that issue 😂

@1aron Sorry, didn't notice an assignee. Feel free to edit or close this PR as you want.

@1aron
Copy link
Member

1aron commented Nov 2, 2023

@lesha1201 No, you did a good job, and I'll merge it as soon as possible and make some tweaks 💯

@1aron 1aron changed the base branch from beta to dev/beta November 2, 2023 09:51
@1aron 1aron merged commit 3c1f6a4 into master-co:dev/beta Nov 2, 2023
19 of 45 checks passed
@1aron
Copy link
Member

1aron commented Nov 3, 2023

@lesha1201 The final solution may be to preset the outline and border styles in @master/normal.css and remove the auto-fill behavior for related rules.

*,
::before,
::after {
    border: 0 solid;
    outline: 0 solid;
}

If you want to keep accessibility:

:focus {
  outline: 2px solid blue;
  outline-offset: 2px;
}

Applying that approach will result in less CSS generated.

@lesha1201
Copy link
Contributor Author

@1aron It's ok to have it for border but I'm really unsure about outline. Disabling it by default migth decrease accessibility significantly because many devs won't be paying attention to it and just will keep the default behavior without realizing that accessibility in this case is worse.

By the way, modern browsers uses :focus-visible by default. They also use a special double color that works with any background color (dark/light).

@1aron
Copy link
Member

1aron commented Nov 4, 2023

@lesha1201 Yep, the default outline styles for most elements:

image

If we could make sure that the above is the default behavior of all elements, then just reset focus outline again in @master/normal.css:

*,
::before,
::after {
    border: 0 solid;
    outline: 0 solid;
}

:focus-visible {
    outline: -webkit-focus-ring-color auto 1px;
}

@1aron 1aron added the wontfix This will not be worked on label Nov 7, 2023
1aron added a commit that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 outline:none is broken
2 participants