refactor: remove tabindex @Prop #267
Conversation
Preserve arrow navigation functionality
pr3tori4n
requested review from
driskull and
jcfranco
and removed request for
driskull
pr3tori4n
added
enhancement
| @@ -86,6 +84,8 @@ export class CalciteTipManager { | |||
|
|
|||
| observer = new MutationObserver(() => this.setUpTips()); | |||
|
|
|||
| tabindex = "0"; | |||
There was a problem hiding this comment.
Since this is no longer a prop, can this be renamed to something a bit more expressive? initialTabIndexAttr?
There was a problem hiding this comment.
It stores both the initial value and then the overridden value if one is present. so initialTabIndex is a misnomer.
There another name you prefer that's more descriptive?
| @@ -242,7 +245,7 @@ export class CalciteTipManager { | |||
| return <Host />; | |||
| } | |||
| return ( | |||
| <Host onKeydown={this.tipManagerKeyDownHandler}> | |||
| <Host onKeydown={this.tipManagerKeyDownHandler} tabindex={this.tabindex}> | |||
There was a problem hiding this comment.
I do still prefer having a container inside the custom element to handle the tabindex that way we are not modifying the tabindex on the host node. We should probably be keeping that logic internal to the component.
It's also more consistent with how a native element works. For example, a button doesn't automatically get a tabIndex but it is focusable by nature due to its internals.
There was a problem hiding this comment.
You can override the default tabindex of button. By keeping the tabindex on the host, we allow that same override here. If we put the tabindex on a child container and ignore the tabindex on the host, then the user loses the ability to override the tabindex.
<button>normal button</button>
<button tabindex="-1">can't focus</button>
<calcite-tip-manager>normal tip manager</calcite-tip-manager>
<calcite-tip-manager tabindex="-1">can't focus</calcite-tip-manager>
There was a problem hiding this comment.
If we put the tabindex on a child container and ignore the tabindex on the host, then the user loses the ability to override the tabindex.
That's not true, if the user sets tabindex="-1" on the host it won't receive focus through tabbing, nor will its children.
This is exactly how the calcite-action works.
There was a problem hiding this comment.
https://codepen.io/pr3tori4n/pen/VwZEEGv
Tried it in this pen. tabindex="-1" in the outer div doesn't prevent focus on the inner div
There was a problem hiding this comment.
These work the same:
<style>
.testClass {
font-family: var(--calcite-app-font-family);
display: flex;
color: var(--calcite-app-foreground);
fill: var(--calcite-app-foreground);
background-color: var(--calcite-app-background);
border-radius: var(--calcite-app-border-radius);
padding: var(--calcite-app-cap-spacing-three-quarters) var(--calcite-app-side-spacing-three-quarters);
margin: 0;
justify-content: flex-start;
align-items: center;
border: none;
width: auto;
cursor: pointer;
font-size: var(--calcite-app-font-size--1);
line-height: normal;
transition: color 125ms ease-in-out, fill 125ms ease-in-out, background-color 125ms ease-in-out;
text-align: unset;
position: relative;
}
</style>
<button class="testClass" tabindex="-1">test</button>
<calcite-action tabindex="-1" text="hello world" text-enabled></calcite-action>There was a problem hiding this comment.
Apparently, shadowDom works differently. There's a whole issue here:
WICG/webcomponents#774
But basically this:
Setting tabindex=-1 on a shadow host as the effect of disabling the focus of the component. We have builtin elements like input and video elements, and if tabindex=-1 is set on either element, then its content including buttons and media controls aren't focusable.
There was a problem hiding this comment.
Seems there are many people suggesting this is unexpected behavior. This may end up changing.
| @@ -94,6 +94,9 @@ export class CalciteTipManager { | |||
|
|
|||
| connectedCallback() { | |||
| this.setUpTips(); | |||
| if (this.el.hasAttribute("tabindex")) { | |||
| this.tabindex = this.el.getAttribute("tabindex"); | |||
There was a problem hiding this comment.
I think we should be consistent with the calcite-action and not set a tabIndex on the host.
There was a problem hiding this comment.
This may also be related: ionic-team/stencil#1008
There was a problem hiding this comment.
and this. ionic-team/stencil#1623
| </header> | ||
| <div class={classnames(CSS.tipContainer, this.direction)} key={this.selectedIndex}> | ||
| <slot /> | ||
| <Host onKeydown={this.tipManagerKeyDownHandler}> |
There was a problem hiding this comment.
Minor: you could move the onKeyDown to the element that receives focus but it will bubble up in this case.
A user can add a |
Related Issue: #258
Summary