fix(core:styles): cleanup focus css prop #6396
Conversation
| @@ -120,7 +119,7 @@ export class CdsButton extends CdsBaseButton { | |||
| </div>`; | |||
| } | |||
|
|
|||
| static styles = [baseStyles, baseButtonStyles, styles]; | |||
| static styles = [baseStyles, styles]; | |||
There was a problem hiding this comment.
I combined base button styles and the buttons styles int a single style sheet. Icon button inherits button styles from the button element.
| static styles = [baseStyles, baseButtonStyles, styles]; | ||
| static get styles() { | ||
| return [super.styles, styles]; | ||
| } |
There was a problem hiding this comment.
Rather than generate the base button css twice just inherit from the button
| width: var(--cds-alias-object-interaction-touch-target); | ||
| height: var(--cds-alias-object-interaction-touch-target); | ||
| cursor: var(--cursor); | ||
| } |
There was a problem hiding this comment.
This ensures the inline controls (checkbox, radio, switch) get the appropriate standard focus outline and minimum touch target size
| @@ -59,7 +59,8 @@ export class CdsInternalControlInline extends CdsControl { | |||
| ? 'order:reverse' | |||
| : ''}" | |||
| > | |||
| <div class="input" @click=${this.selectInput}></div> | |||
| <div role="presentation" class="input" @click=${this.selectInput}></div> | |||
| <div role="presentation" focusable @click=${this.selectInput}></div> | |||
There was a problem hiding this comment.
This fixes an a11y bug where the SR can get confused on treating the click handler as an interactive group rather than focusing to the native slotted checkbox or radio input.
coryrylan
force-pushed
the
topic/core-focus-cleanup
branch
from
d0c0857 to
d9f5f27
Compare
| position: relative; | ||
| --width: #{$cds-global-space-7}; | ||
| --height: #{$cds-global-space-7}; | ||
| --cds-alias-object-interaction-touch-target: #{$cds-global-space-8}; |
There was a problem hiding this comment.
Sets a minimum touch target for inline elements like radio/checkbox. Also useful for datagrid to align target/focus to cell easily.
| --outline: var(--cds-alias-object-interaction-outline); | ||
| --outline-offset: var(--cds-alias-object-interaction-outline-offset); | ||
| --cds-alias-object-interaction-outline: none; | ||
| --cds-alias-object-interaction-outline-offset: 0; |
There was a problem hiding this comment.
Input overrides the default outline since it uses the underline focus effect. However I still use the variable so its consistent with theming and can be adjusted like any other component.
Signed-off-by: Cory Rylan <splintercode.cb@gmail.com>
coryrylan
force-pushed
the
topic/core-focus-cleanup
branch
from
d9f5f27 to
d4a1e2b
Compare
| @@ -1,244 +0,0 @@ | |||
| // Copyright (c) 2016-2021 VMware, Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
Why are we deleting base button?
There was a problem hiding this comment.
There was a problem hiding this comment.
Got it. I was confused. I thought we were deleting base-button.element.ts...
| @@ -96,6 +96,7 @@ export class CdsAccordionPanel extends LitElement implements Animatable { | |||
| ?disabled="${this.disabled}" | |||
| aria-disabled="${this.disabled}" | |||
| aria-expanded="${this.expanded}" | |||
| focusable | |||
There was a problem hiding this comment.
Would this focusable attribute work in React when users need to assign boolean values?
There was a problem hiding this comment.
This in particular is internal in the shadow dom so no consumer would access/use it. It is for making it easy to apply the focus element styles internal to our templates.
| outline: 0 !important; | ||
| } | ||
|
|
||
| [focusable] { |
There was a problem hiding this comment.
Related question. I remember for boolean attributes, I needed to specify [focusable]=true/false in React. I wonder if that applies here too.
There was a problem hiding this comment.
Same as the other example, this is internal to the template. It doesnt use::slotted so its not applying as part of the public API. If its a property of the component React can set it like any other property however if its a plain html attribute (@property was not used in the web component) then users will run into this problem facebook/react#9230
There was a problem hiding this comment.
Makes sense. Thanks for the explanation.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the new behavior?
This is some work I am pulling out of the datagrid branch. This adds the native focus outline as a token value to help make the outline usage consistent within components. This also adds a focus outline state demo for the interaction examples in storybook.
Does this PR introduce a breaking change?
Other information