fix(radio-button-group): no longer focus first radio button on label click and adds `setFocus` method. by anveshmekala · Pull Request #7050 · Esri/calcite-design-system

anveshmekala · 2023-05-26T23:02:48Z

Related Issue: #6698

Summary

This PR will remove the focus flash from the first calcite-radio-button in group on label click

Additional enhancements:

Adds setFocus( ) method in calcite-radio-button-group
Users will be able to select label text in chrome. Unable to select label text in radio group #6357

Note:
With this PR, delegateFocus is removed from calcite-radio-button-group which is causing #6698. Because the tabIndex is set to 0 for the first focusable element on page load to enable keyboard navigation into the group it makes delegateFocus to apply focus styles on the first focusable element when the user clicks on the label (invokes .focus( )) resulting in the behavior mentioned in the issue 6698. To resolve this, delegateFocus is removed and added setFocus( ) method.

…lect-issue

driskull

Looks good 👍

driskull · 2023-06-01T20:21:07Z

    }
  };

+  private getFocusableRadioButton(): HTMLCalciteRadioButtonElement | null {


Could you use the focusFirstTabbable helper in the dom utils?

This is what the modal does...

@Method() async setFocus(): Promise<void> { await componentLoaded(this); focusFirstTabbable(this.el); }

Would we want only 1 radio button within a group tabbable at a time? Seems like that might make for a good enhancement in the future.

tried implementing focusFirstTabbable helper here, it works well if none if the radio-button's in the group are disabled. The reason for this is the way we assign tabIndex at the radio-button level. The tabIndex=0 only if the radio-button is checked or if its the first element in the group. I think this is a bug. ~~Wonder if the assignment of tabIndex should stay at the group level not the radio-button level.~~

Would we want only 1 radio button within a group tabbable at a time? Seems like that might make for a good enhancement in the future.

The above is following w3 radio group pattern keyboard specs i believe. One button is tabbable and the user can use Arrow Keys to navigate between the items.

…lect-issue

driskull

comments! :)

driskull · 2023-06-05T20:36:29Z

    }
  };

+  private getFocusableRadioButton(): HTMLCalciteRadioButtonElement | null {


Can you use array.find instead of the while loop?

return this.radioButtons.find((radioButton) => !radioButton.disabled);

The other option would to be to use the tabbable dependency to just get the first tabbable element and focus it.

driskull

👍

driskull · 2023-06-06T17:09:22Z


+  private getFocusableRadioButton(): HTMLCalciteRadioButtonElement | null {
+    const index = this.radioButtons.findIndex((radiobutton) => !radiobutton.disabled);
+    return index >= 0 ? this.radioButtons[index] : null;


@anveshmekala if you're returning the button, why not just use .find() instead of .findIndex?

return this.radioButtons.find((radioButton) => !radiobutton.disabled) ?? null;

my bad, i checked in the wrong commit. .find( ) is the optimal choice here.

driskull · 2023-06-06T17:25:22Z

      return undefined;
    }
-    return this.checked || this.isDefaultSelectable() ? 0 : -1;
+    return this.checked || this.isFocusable() ? 0 : -1;


If a radio-button is added as the first radio-button after all the other radio buttons have already been added to the DOM my guess is that there will be two focusable radio buttons.

We may want to consider creating a registry of radio buttons that are keyed by the name property so that we can ensure only one is ever focusable. @anveshmekala what do you think?

While testing the above edge case found out an issue with the focusable logic. tabIndex=0 is being assigned to all the tabbable radio-buttons if the first radio button is disabled due to async execution of isFocusable method in each radio-button component.

Regarding the addition of new radio-button after the initial component load, once the above is fixed it wouldn't let two radio-buttons having tabIndex=0 but the first focusable radio-button wont be updated. Using registry may not help since it is specific to each radio-button and any updates (Ex: tabIndex) to the siblings wont trigger updates to the registry.

Since radio button's don't always belong to a group we can't rely on a mutation observer of a parent group so we likely need some kind of map/registry to maintain the active focusable radio button for a specific name property. This logic would make sure only one radio button with the same name value can be focusable. It should be set/updated anytime a radio-button connectedCallback/disconnectedCallback occurs.

Probably something like...

type RootNode = Document | ShadowRoot; type ActiveRadioButtonLookup = WeakMap<RootNode, Map<string, HTMLCalciteRadioButtonElement>>;

There is a method queryButtons in radio-button component which queries all the radio-buttons with a similar name for a given radio-button. This will ensure any mutations to the tabIndex attribute of radio-button will be in the same group.

I tried addressing edge cases with the existing setup but it is breaking other features. Will probably split this PR tomorrow in to individual PR's since the issue we have is related to#7113 and prototype with WeakMap.

…lect-issue

driskull

👍 Looks good!

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 1.4.3</summary> ## [1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components@1.4.2...@esri/calcite-components@1.4.3) (2023-06-26) ### Bug Fixes * **accordion-item:** support items working across shadowDOM ([#7035](#7035)) ([6378e35](6378e35)), closes [#6167](#6167) * **alert:** Sets autoCloseDuration to "medium" by default ([#7157](#7157)) ([1b9a8ed](1b9a8ed)) * **alert:** Update alert queue when an alert is removed from the DOM ([#7189](#7189)) ([edd59eb](edd59eb)) * **combobox, dropdown, input-date-picker, input-time-picker, popover, tooltip:** Prevent repositioning from affecting other floating components ([#7178](#7178)) ([1b02dae](1b02dae)) * Ensure mouse events are blocked for disabled components in Firefox ([#7107](#7107)) ([271d985](271d985)) * **input-date-picker:** Fix showing the placeholder when resetting the value ([#7156](#7156)) ([8d60ffd](8d60ffd)) * **input, input-number:** Allows numeric characters. ([#7213](#7213)) ([739f0af](739f0af)) * **input,input-number:** Allow typing decimal separator in firefox for arabic locale ([#7173](#7173)) ([595e6f2](595e6f2)) * **list:** No longer has incorrect border width ([a810943](a810943)) * **list:** Update selectedItems property on all item selection changes ([#7204](#7204)) ([da048f6](da048f6)) * **menu-item:** Ensure correct order of rendered icons ([#7098](#7098)) ([fd344e9](fd344e9)), closes [#7097](#7097) * **navigation:** Label is no longer a required property ([#7084](#7084)) ([ba2bd4d](ba2bd4d)) * **radio-button-group:** No longer focus first radio button on label click and adds `setFocus` method. ([#7050](#7050)) ([4267b8c](4267b8c)) * **radio-button, radio-button-group:** Prevent emitting events when selecting a checked radio button ([#7102](#7102)) ([77fcc81](77fcc81)) * **radio-button:** Focuses first focusable radio-button element in group. ([#7152](#7152)) ([dd7ec60](dd7ec60)) * **scrim:** Responsively set the scale of the loading spinner ([#7182](#7182)) ([72c5943](72c5943)) * **tooltip:** Improve component timing ([#7172](#7172)) ([106f5d2](106f5d2)) * **tree-item:** Ensure expanded tree-item is displayed when expanded and made visible ([#7216](#7216)) ([3c0fbf5](3c0fbf5)) <details><summary>@esri/calcite-components-react: 1.4.3</summary> ## [1.4.3](https://github.com/Esri/calcite-components/compare/@esri/calcite-components-react@1.4.2...@esri/calcite-components-react@1.4.3) (2023-06-26) ### Miscellaneous Chores * **@esri/calcite-components-react:** Synchronize undefined versions ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from 1.4.3-next.7 to 1.4.3 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben Elan <no-reply@benelan.dev>

fix(radio-group): user can select text in radio group

3585474

github-actions Bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label May 26, 2023

github-actions Bot assigned anveshmekala May 26, 2023

anveshmekala added 3 commits May 31, 2023 10:47

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

0d52082

…lect-issue

add setFocus for radio button group

f70b1a1

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

61fdbea

…lect-issue

anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jun 1, 2023

simplify logic

b15ff52

anveshmekala changed the title ~~fix(radio-group): user can select text in radio group~~ fix(radio-button-group): user can select text in radio group Jun 1, 2023

anveshmekala changed the title ~~fix(radio-button-group): user can select text in radio group~~ fix(radio-button-group): user can select text. Jun 1, 2023

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 1, 2023

remove white space

2753186

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 1, 2023

add more e2e tests

4b5ca37

anveshmekala marked this pull request as ready for review June 1, 2023 20:10

anveshmekala requested a review from a team as a code owner June 1, 2023 20:10

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 1, 2023

anveshmekala changed the title ~~fix(radio-button-group): user can select text.~~ fix(radio-button-group): user can select label text and no longer focus first radio button on click. Jun 1, 2023

driskull reviewed Jun 1, 2023

View reviewed changes

anveshmekala added 4 commits June 1, 2023 17:26

clean up

082cb9c

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

1740178

…lect-issue

focus first focusable item on tab

c389f18

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

1357821

…lect-issue

anveshmekala marked this pull request as draft June 3, 2023 01:17

anveshmekala added 3 commits June 5, 2023 11:14

update focus algorithm

cffff6d

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

88cd7ce

…lect-issue

fix test error

f7f25d7

anveshmekala marked this pull request as ready for review June 5, 2023 19:17

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 5, 2023

anveshmekala changed the title ~~fix(radio-button-group): user can select label text and no longer focus first radio button on click.~~ fix(radio-button-group): no longer focus first radio button on label click and adds setFocus method. Jun 5, 2023

anveshmekala requested a review from driskull June 5, 2023 19:50

driskull reviewed Jun 5, 2023

View reviewed changes

Comment thread packages/calcite-components/src/components/radio-button-group/radio-button-group.tsx Outdated

cleanup

55c2018

anveshmekala requested a review from driskull June 6, 2023 17:04

driskull approved these changes Jun 6, 2023

View reviewed changes

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

955b751

…lect-issue

anveshmekala marked this pull request as draft June 6, 2023 19:06

anveshmekala added 3 commits June 6, 2023 18:17

refactor

0510a88

revert changes to radio-button

2a0cc42

Merge branch 'master' into anveshmekala/6357-fix-radio-button-text-se…

184e2a9

…lect-issue

anveshmekala marked this pull request as ready for review June 7, 2023 16:05

driskull approved these changes Jun 7, 2023

View reviewed changes

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 7, 2023

anveshmekala merged commit 4267b8c into master Jun 7, 2023

anveshmekala deleted the anveshmekala/6357-fix-radio-button-text-select-issue branch June 7, 2023 17:09

github-actions Bot added this to the 2023 June patch priorities milestone Jun 7, 2023

calcite-admin mentioned this pull request Jun 7, 2023

chore: release latest #7144

Merged

Conversation

anveshmekala commented May 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anveshmekala Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

driskull left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

anveshmekala commented May 26, 2023 •

edited

Loading

anveshmekala Jun 1, 2023 •

edited

Loading