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

Consistent focus/blur eventing across components #5147

Open
3 of 7 tasks
jcfranco opened this issue Aug 13, 2022 · 6 comments
Open
3 of 7 tasks

Consistent focus/blur eventing across components #5147

jcfranco opened this issue Aug 13, 2022 · 6 comments
Assignees
Labels
1 - assigned Issues that are assigned to a sprint and a team member. blocked This issue is blocked by another issue. epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project.
Milestone

Comments

@jcfranco
Copy link
Member

jcfranco commented Aug 13, 2022

Overview

Currently, developers have to rely on focusin/focusout (composable + bubbling) from component internals or using, internal, blur/focus events for a few components.†
The goal for this effort is to provide a general solution for focus/blur events given that these events are commonly used and expected in many web development scenarios.

Also for consideration, setFocus has introduced a pattern that can lead to inconsistent focus behavior. Existing usage should be revisited to see if we can consolidate in favor of consistency (one focus pattern to rule them all).

† In general, this works but leads to boilerplate code as focusin/focusout fires when you move between elements inside a component and would need to check where focus lies when handling the event.

Tasks

  • determine if delegatesFocus is a viable solution

    • consult design regarding potential double focus outline

      • Design suggests hidding the focus ring on the host and only displaying it on the element it delegates focus to. a11y agrees with the recommendation
      • ** Note I haven't seen any double focus rings yet, it may have been a quirk that is worked out now**
    • check browsers focusing is consistent, if not weigh options

      • As usual, browsers behave differently when it comes to focus (example). However, since delegatesFocus is a web standard, I think we will have better consistency than we would if we try to jerry-rig up our own solution. Once we get it set up in the code we can do more browser testing, but all the browsers have adopted the pattern and are reletaively consistent, based on my research.*
    • check that focus/blur events will behave as expected for both light/shadow DOM content (if so, we use these and drop our internal focus/blur events)

    • confirm dev expectations light/shadow DOM content focus/blur (e.g., blurring slotted content not emitting blur on custom element) - https://codepen.io/jcfranco/pen/ZExMgwx?editors=1000

    • check if https://www.npmjs.com/package/delegates-focus-polyfill will fit the bill since Safari 14 doesn't support it

      • Safari 16 released so we don't need the polyfill
  • consult design, a11y concerns from having multiple focus targets from setFocus

    • if keeping single focusable target, we should consider promoting calciteComponent.focus/blur() for shadow-targeted focus/blur or slottedContent.focus/blur() for light-targeted focus/blur

Additional resources

cc @benelan @geospatialem

@jcfranco jcfranco added the epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project. label Aug 13, 2022
@benelan benelan self-assigned this Oct 22, 2022
@benelan benelan added the 2 - in development Issues that are actively being worked on. label Oct 24, 2022
@geospatialem geospatialem added this to the 2023 January Priorities milestone Nov 23, 2022
@benelan
Copy link
Member

benelan commented Dec 13, 2022

Findings

Disabled behavior

It turns out delegatesFocus does not take into account tabindex or pointer-events: none when determining the first focusable element on click and focus(). This is an issue for us because components emulated as disabled will still be delegated focus if they are the first element in the flat tree.

Using inert on the mock "disabled" element solves this issue , but there isn't enough browser support at the moment (supported by supported browsers).

Checked behavior

delegatesFocus also doesn't understand our custom checked property (e.g. in radio-group). Meaning it will always focus the first radio-group-item in the flat tree.

However, focus is delegated to elements with the native checked property (e.g. radio-button-group), even if the radio-button isn't the first element.

Focus method

Calling the focus() method on components that delegate focus only works correctly once the component is fully loaded. In order to get rid of our async setFocus method, users would need to ensure the component is ready before using focus().

Examples

This correctly delegates focus to the internal input:

customElements.whenDefined("calcite-filter").then(() => 
         document.querySelector("calcite-filter").setFocus());

This does as well, but not without the timeout:

customElements
    .whenDefined("calcite-filter")
    .then(() => setTimeout(() => document.querySelector("calcite-filter").focus(), 1000));

This works too:

(async () => {
  await customElements.whenDefined("calcite-date-picker");
  const el = await document
    .querySelector("calcite-date-picker")
    .componentOnReady();
  requestAnimationFrame(() => el.focus());
})();

From https://github.com/Esri/calcite-design-system/pull/9751/files#r1689436792

One more downside to using delegatesFocus is that it prevents blurring any focusable element that has focus by clicking on a non-focusable area within the delegatesFocus host (see example). This might not be desired in some scenarios.

Next steps

  • Continue enabling delegatesFocus for the components that don't have the possibility of the first focusuble element being mock "disabled".
  • Add a list of the components being skipped at the bottom of this comment so we can circle back later.
  • Check with @jcfranco about the skipped items when he returns.

Resources

Here is a codepen demonstration: https://codepen.io/benesri/pen/PoavMWM

Here is the WICG issue where they made that decision: WICG/webcomponents#830

Specifically this snippet from the issue body:

Since we want to change the behavior of focus delegation to not be related to sequential focus navigation, we should probably remove the tabindex priority thing as well in this case. So we should always delegate focus to the first focusable area in DOM/composed-tree order

and this comment near the bottom:

Sounds like there was a consensus to move forward with what's being proposed thus far:

  • Programmatic focus would focus the first element in the flat tree that's programmatically focusable.
  • Mouse focus would focus the first element in the flat tree that's mouse focusable.
  • Keyboard focus would focus the first element in tab index order that's keyboard focusable.

Skipped components

expand list

Selected value

Skipped due to the inconsistency between radio-group and radio-button-group delegating focus to the first element in the flat tree versus the checked element.

  • radio-group

Disabled issue

If a "mock disabled" element is first in the flat tree, it will be delegated focus. A lot of our components can have slotted content as the first focusable element.

  • alert
  • accordion
  • accordion-item
  • block
  • block-section
  • card
  • combobox
  • combobox-item-group
  • date-picker-month
  • flow
  • flow-item
  • list
  • list-item
  • list-item-group
  • modal
  • notice
  • option-group
  • panel
  • pick-list
  • pick-list-group
  • pick-list-item
  • popover
  • shell
  • shell-center-row
  • shell-panel
  • stepper
  • stepper-item
  • select
  • tab
  • tabs
  • tab-nav
  • tile
  • tile-select
  • tile-select-group
  • tree
  • tip
  • tip-group
  • tree-item
  • value-list
  • value-list-item
  • label ?
  • rating ?

Nothing to delegate focus to

The components don't need to delegate focus to another element.

  • action
  • avatar
  • button
  • chip
  • checkbox
  • date-picker-day
  • date-picker-month-header
  • dropdown-item
  • combobox-item
  • fab
  • graph
  • handle
  • icon
  • link
  • progress
  • input-message
  • input-text
  • input
  • input-number
  • option
  • scrim
  • switch
  • tab-title
  • textarea
  • tooltip
  • radio-button
  • radio-group-item

@benelan benelan added 1 - assigned Issues that are assigned to a sprint and a team member. blocked This issue is blocked by another issue. and removed 2 - in development Issues that are actively being worked on. labels Dec 15, 2022
@benelan
Copy link
Member

benelan commented Dec 16, 2022

@geospatialem I moved this to Stalled because a lot of the components need Franco's feedback. But can you please verify the components that have been completed so far when you have time? You can use the setFocus method to ensure the first focusable element is focused. Here is an example for filter:
https://codepen.io/benesri/pen/XWYvoGa?editors=1010

  • dropdown (focus)
  • dropdown-group (setFocus)
  • action-bar (focus)
  • action-group (focus)
  • action-pad (setFocus)
  • date-picker (focus)
  • filter (setFocus)
  • inline-editable (setFocus)
  • input-date-picker (setFocus)
  • input-time-picker (setFocus)
  • time-picker (setFocus)
  • split-button (focus)
  • pagination (focus)
  • radio-button-group (setFocus)
  • slider (setFocus)

@geospatialem
Copy link
Member

@benelan Thanks for the thorough list to work through! I wasn't able to verify the setFocus method for the components listed below.

Is there an additional step needed to ensure setFocus is selecting the first focusable element?

@benelan
Copy link
Member

benelan commented Dec 16, 2022

Taking another look at inline-editable. The rest don't have setFocus() methods so you can use a timeout and focus()

  customElements
    .whenDefined("calcite-date-picker")
    .then(() => setTimeout(() => document.querySelector("calcite-date-picker").focus(), 1000));

@benelan
Copy link
Member

benelan commented Dec 16, 2022

This works for inline-editable

(async () => {
  await customElements.whenDefined("calcite-inline-editable");
  const el = await document
    .querySelector("calcite-inline-editable")
    .componentOnReady();
  requestAnimationFrame(() => el.setFocus());
})();

Edit

The above code also works with the native focus() method so you don't need a timeout.

@geospatialem
Copy link
Member

Awesome, thanks for the insights! ✨ The above components are verified, will await for discussion on the skipped components upon Franco's return.

benelan added a commit that referenced this issue Feb 3, 2023
…add setFocus method (#6405)

**Related Issue:** #5147

## Summary

Adds the public `setFocus` method to the `delegatesFocus` components
that didn't have it. See
#5147 (comment)
jcfranco pushed a commit that referenced this issue Feb 4, 2023
…add setFocus method (#6405)

**Related Issue:** #5147

## Summary

Adds the public `setFocus` method to the `delegatesFocus` components
that didn't have it. See
#5147 (comment)
benelan added a commit that referenced this issue Mar 28, 2023
…add setFocus method (#6438)

**Related Issue:** #5147

## Summary

Adds the public `setFocus` method to the `delegatesFocus` components
that don't have it. 

Ref: #5147 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 - assigned Issues that are assigned to a sprint and a team member. blocked This issue is blocked by another issue. epic Large scale issues to be broken up into sub-issues and tracked via sprints and/or project.
Projects
None yet
Development

No branches or pull requests

4 participants