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.

Sign up for GitHub

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: move focus ring to button host to allow padding #3792

Closed
wants to merge 1 commit into from
Closed

fix: move focus ring to button host to allow padding #3792

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 15 additions & 16 deletions packages/web-components/fast-components/src/styles/patterns/button.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const BaseButtonStyles = css`
min-width: calc(${heightNumber} * 1px);
background-color: ${neutralFillRestBehavior.var};
color: ${neutralForegroundRestBehavior.var};
border: calc(var(--outline-width) * 1px) solid transparent;
border-radius: calc(var(--corner-radius) * 1px);
fill: currentColor;
cursor: pointer;
Expand All @@ -56,8 +57,8 @@ export const BaseButtonStyles = css`
white-space: nowrap;
outline: none;
text-decoration: none;
border: calc(var(--outline-width) * 1px) solid transparent;
color: inherit;
border: none;
border-radius: inherit;
fill: inherit;
cursor: inherit;
Expand All @@ -71,11 +72,10 @@ export const BaseButtonStyles = css`
background-color: ${neutralFillActiveBehavior.var};
}

.control:${focusVisible} {
Copy link
Contributor

@nicholasrice nicholasrice Aug 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Doh - I just realized this is going to break :focus-visible when that rolls in. :focus-visible is not out from behind a flag yet so I think this is okay for now but we should get other folk's opinion on that. @EisenbergEffect @chrisdholt

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Assuming we had focus visible, what would be the alternative fix? If we can plan for forward compat, that's definitely preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Propose :focus-visible-within be added to the spec? 🙃 I can't think of a pure CSS way to keep :focus-visible. Does fast have a mechanism for forwarding styles to child elements?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been discussed!

I can't either... You can forward styles to shadowed element has a part attribute, which button does.

Copy link
Member

Choose a reason for hiding this comment

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

@nicholasrice @CuddleBunny :focus-visible is shipping in Chrome 86 which is currently in Beta: https://twitter.com/ChromiumDev/status/1301262894617640960

This will cover both Chrome and I'm checking on if it will also be in "Edgium" (h/t @CuddleBunny) for 86...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this should just be closed as a non-issue and documented that button padding can be controlled by targeting the control part? At this point it is down to developer experience vs accessibility. The latter is probably more important.

Copy link
Contributor

Choose a reason for hiding this comment

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

@CuddleBunny Unfortunately I think that is the best course for now. Thanks for looking at this though!

:host(:focus-within) {
border: calc(var(--outline-width) * 1px) solid ${neutralFocusBehavior.var};
box-shadow: 0 0 0 calc((var(--focus-outline-width) - var(--outline-width)) * 1px) ${
neutralFocusBehavior.var
};
box-shadow: 0 0 0 calc((var(--focus-outline-width) - var(--outline-width)) * 1px)
${neutralFocusBehavior.var};
}

.control::-moz-focus-inner {
Expand All @@ -91,10 +91,8 @@ export const BaseButtonStyles = css`
.start,
.end,
::slotted(svg) {
${
/* Glyph size and margin-left is temporary -
replace when adaptive typography is figured out */ ""
} width: 16px;
${/* Glyph size and margin-left is temporary -
replace when adaptive typography is figured out */ ""} width: 16px;
height: 16px;
}

Expand Down Expand Up @@ -129,8 +127,9 @@ export const AccentButtonStyles = css`
background: ${accentFillActiveBehavior.var};
}

:host(.accent) .control:${focusVisible} {
box-shadow: 0 0 0 calc(var(--focus-outline-width) * 1px) inset ${neutralFocusInnerAccentBehavior.var};
:host(.accent:focus-within) {
box-shadow: 0 0 0 calc(var(--focus-outline-width) * 1px) inset
${neutralFocusInnerAccentBehavior.var};
}

:host(.accent.disabled) {
Expand All @@ -154,13 +153,13 @@ export const HypertextStyles = css`
height: auto;
min-width: 0;
background: transparent;
border: none;
box-shadow: none;
}

:host(.hypertext) .control {
display: inline;
padding: 0;
border: none;
box-shadow: none;
border-radius: 0;
line-height: 1;
}
Expand Down Expand Up @@ -197,13 +196,13 @@ export const LightweightButtonStyles = css`
:host(.lightweight) {
background: transparent;
color: ${accentForegroundRestBehavior.var};
border: none;
box-shadow: none;
}

:host(.lightweight) .control {
padding: 0;
height: initial;
border: none;
box-shadow: none;
border-radius: 0;
}

Expand Down Expand Up @@ -280,7 +279,7 @@ export const OutlineButtonStyles = css`
border-color: inherit;
}

:host(.outline) .control:${focusVisible} {
:host(.outline:focus-within) {
border: calc(var(--outline-width) * 1px) solid ${neutralFocusBehavior.var});
box-shadow: 0 0 0 calc((var(--focus-outline-width) - var(--outline-width)) * 1px) ${neutralFocusBehavior.var};
}
Expand Down