Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue #650

muditchoudhary · 2024-06-04T11:40:35Z

Description

This PR, creates a new KRadioButtonGroup component that fixes radio button movement with arrow keys issues in Firefox.
It also has some Unit tests for KRadioButtonGroup.
It creates docs for KRadioButtonGroup

Issue addressed

Fixes: 10491

Addresses #PR# HERE

Before/after screenshots

Before

Before.mp4

After

After.mp4

Changelog

[#PR no]
- Description: Summary of change(s)
- Products impact: Choose from - none (for internal updates) / bugfix / new API / updated API / removed API. If it's 'none', use "-" for all items below to indicate they are not relevant.
- Addresses: Link(s) to GH issue(s) addressed. Include KDS links as well as links to related issues in a consumer product repository too.
- Components: Affected public KDS component. Do not include internal sub-components or documentation components.
- Breaking: Will this change break something in a consumer? Choose from: yes / no
- Impacts a11y: Does this change improve a11y or adds new features that can be used to improve it? Choose from: yes / no
- Guidance: Why and how to introduce this update to a consumer? Required for breaking changes, appreciated for changes with a11y impact, and welcomed for non-breaking changes when relevant.

[#PR no]: PR link

Steps to test

Yarn link the Kolibri with KDS. How? given: here
Import KRadioButtonGroup component and wrap KRadioButton components inside KRadioButtonGroup component, in these files:
2.1 kolibri/plugins/device/assets/src/views/DeviceSettingsPage/index.vue
2.2 kolibri/core/assets/src/views/language-switcher/LanguageSwitcherModal.vue
Run Server and test in Firefox by visiting these pages:
3.1 http://127.0.0.1:8000/en/device/#/settings
3.2 Choose Language

Second way

Paste the given playground.vue code. here
Run the KDS server and visit playground and test.

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Contributor has fully tested the PR manually
If there are any front-end changes, before/after screenshots are included
Critical and brittle code paths are covered by unit tests
The change is described in the changelog section above

Reviewer guidance

Is the code clean and well-commented?
Are there tests for this change?
Are all UI components LTR and RTL compliant (if applicable)?
Add other things to check for here

After review

The changelog item has been pasted to the CHANGELOG.md

Comments

For the change language model KGrid is used, which creates two KRadioButtonGroup components as the array is split.
Although the arrow keys are working fine but when we shift tab it puts focus on the last focus radio button on the first list.
I want to know your thoughts. Is that acceptable?

…-v1.5.x Release 1.5.x into develop

- This component enables focus movenment on radio buttons with arrow keys on firefox

- Add KRadioButton method description - Add KRadioButtonGroup slot and props description

muditchoudhary · 2024-06-04T12:00:26Z

Hi @MisRob Could you please review the PR?

MisRob · 2024-06-05T09:31:51Z

Hi @muditchoudhary, thanks a lot! I think as first step, we will QA in Kolibri. Would you be able to open a Kolibri PR for testing that would reference the latest commit of this KDS PR and use KRadioButton at least at one Kolibri place? Our QA team doesn't work with code directly. If you couldn't do that, I can prepare the PR as well.

muditchoudhary · 2024-06-05T09:56:11Z

Sure @MisRob I had tested on Kolibri and I have the changes in stash, so I will have no problem in making a PR. I will send a PR soon.

- This component enables focus movenment on radio buttons with arrow keys on firefox

- Add KRadioButton method description - Add KRadioButtonGroup slot and props description

…o KRadioButtonGroup-Component

- Used ._componentTag instead of ._name - Changed onkeydown with onkeyup listener

MisRob

Thanks so much @muditchoudhary, firstly for researching the issue and approaches to solve it, and then in this PR for finalizing it. Also thanks for taking care of docs and tests. Overall it's great work and feels solid.

I'm leaving a couple of comments. The most important is the one related to many event listeners and it's the only change I request before proceeding to merging. I'd also like to clarify the enable logic.

I'd appreciate if we could have the improvement regarding the dependency to KGridItem, however it's also something that I could open a follow-up issue for. Same applies to the remaining comments that are rather minor. It really depends on your bandwith - let me know what works for you.

MisRob · 2024-07-14T17:52:58Z

lib/__tests__/KRadioButtonGroup.spec.js

+      await wrapper.vm.$nextTick();
+      const radioButtons = wrapper.findAllComponents(KRadioButton);
+      await radioButtons.at(2).trigger('click');
+      expect(wrapper.vm.focusedRadioIdx).toBe(2);


It'd be great if we could test that this radio button is focused without dependency on its internal implementation, that is looking at vm.focusedRadioIdx. Imagine a situation when we'd rename/remove focusedRadioIdx, but in a way that focus would still work properly for users. In such a situation, this test would give negative result, even though there would be no problem for users. So for maintenance and clarity benefits, whenever possible, we to implement tests in a way that's similar to how the user interact with elements.

I think this technique should do https://stackoverflow.com/a/53042010 (but with attachTo as attachToDocument is deprecated)

MisRob · 2024-07-14T17:53:19Z

lib/__tests__/KRadioButtonGroup.spec.js

+      await radioButtons.at(2).trigger('click');
+      expect(wrapper.vm.focusedRadioIdx).toBe(2);
+      expect(radioButtons.at(2).vm.tabIndex).toBe(0);
+      expect(radioButtons.at(1).vm.tabIndex).toBe(-1);


For the same reason as mentioned above, here I'd recommend looking into attributes of the DOM node rather than into vm's private data. I assume it could look something like expect(radioButtons.at(1).attributes('tab-index')).toBe(0).

MisRob · 2024-07-14T18:04:46Z

lib/KRadioButtonGroup.vue

+       * Specifies whether the radio button group is enabled or disabled.
+       * Used to make the first radio button active when the radio group becomes enabled again after being disabled.
+       */
+      enable: {


I don't quite understand yet in what case we'd need to use enable functionality, would you explain on an example?

MisRob · 2024-07-14T18:21:07Z

lib/KRadioButtonGroup.vue

+    mounted() {
+      this.$nextTick(() => {
+        if (this.isFirefox) {
+          if (this.$children[0].$options._componentTag === 'KGridItem') {


Instead of checking specifically for KGridItem, could we recursively search through $children and their $children etc. until we find the level of radio buttons?

This would make KRadioButtonGroup work with any other component that wraps its KRadioButtons, such as KFixedGridItem or even HTML elements such as div.

The alternative would be to properly document that only this single use-case is supported, however it seems to me that this suggestion shouldn't hopefully be complicated and I believe it would also help with clarity and deduplicated some of the code.

MisRob · 2024-07-14T18:23:47Z

lib/KRadioButtonGroup.vue

+              gridItem.$children.forEach(fixedGridItem => {
+                fixedGridItem.$children.forEach(radioBtn => {
+                  this.radioButtons.push(radioBtn);
+                  radioBtn.$el.addEventListener('keyup', this.onKeyUp);


In larger groups of radio buttons, such as the language selection, adding an event listener to each one of them could potentially have negative performance impact, particularly in combination with looping through many elements. A general recommendation we follow in our codebases is to delegate listeners to a container element higher in the structure, so that we have as less listeners as possible.

In this case, KRadioButtonGroup element when mounted (and only in Firefox) would attach a single keyup event listener. This event's handler function would then determine whether event.target was a radio button, and if yes, it would run the logic needed.

I would also recommend doing the very same thing for the input event (line 71)

So you'd have something like

mounted() { this.$el.addEventListener('keyup', onKeyUp); this.$el.addEventListener('input', onInput); ...

and these two would be the only event listeners in KRadioButtonGroup.

Related logic would need to be adjusted to work in this updated way.

MisRob · 2024-07-14T18:24:36Z

lib/KRadioButtonGroup.vue

+    },
+    mounted() {
+      this.$nextTick(() => {
+        if (this.isFirefox) {


Just a minor suggestion that could help with code readability. This would avoid one level of nesting:

mounted() { if (!this.isFirefox) { return; } this.$nextTick(() => { ... ... ...

MisRob · 2024-07-14T18:33:45Z

lib/KRadioButtonGroup.vue

+    },
+    watch: {
+      // To focus on the first radio button when radio group enables again
+      enable(newVal, oldVal) {


I believe that before running the code below, we'd also need isFirefox check here

MisRob · 2024-07-14T18:40:48Z

lib/KRadioButtonGroup.vue

+      },
+      focusRadio(radioIdx, event) {
+        if (radioIdx !== undefined && radioIdx !== null) {
+          this.radioButtons[radioIdx].toggleCheck(event);


Since we're calling KRadioButton's (currently private) toggleCheck method, could you please add @public JSDoc comment to toggleCheck, similarly to how you did for setTabIndex? It will ensure that if refactoring KRadioButton, we will know that the method is being used from another component and don't remove/rename it accidentally.

MisRob · 2024-07-15T18:22:31Z

lib/KRadioButton.vue

@@ -220,6 +222,13 @@
         */
        this.$emit('blur');
      },
+      /**
+       * @public
+       * Set the tabIndex value


Thank you for making sure its documented. Just a little improvement for clarity:

Suggested change

* Set the tabIndex value

* Sets `tab-index` on input

muditchoudhary · 2024-07-17T01:43:49Z

Thanks a lot, @MisRob for giving the detailed review and suggesting changes. I will send the changes soon.

MisRob and others added 8 commits September 28, 2023 20:29

Merge pull request learningequality#457 from learningequality/release…

fbdf0c0

…-v1.5.x Release 1.5.x into develop

Merge remote-tracking branch 'upstream/develop' into develop

2b44dbf

Merge remote-tracking branch 'upstream/develop' into develop

7a2c1ad

Add new KRadioButtonGroup Component

dd6a483

- This component enables focus movenment on radio buttons with arrow keys on firefox

Add setTabIndex method to set tab index on each radio component

ea5a9f1

Add test for KRadioButtonGroup component

6100a15

Add docs for KRadioButtonGroup

eb9c801

- Add KRadioButton method description - Add KRadioButtonGroup slot and props description

Update tableOfContents

fddf31e

MisRob self-assigned this Jun 7, 2024

MisRob self-requested a review June 7, 2024 08:31

muditchoudhary added 9 commits June 19, 2024 11:55

Merge remote-tracking branch 'upstream/develop' into develop

afcd18f

Add new KRadioButtonGroup Component

4b0688a

- This component enables focus movenment on radio buttons with arrow keys on firefox

Add setTabIndex method to set tab index on each radio component

87a3928

Add test for KRadioButtonGroup component

ebaf9cb

Add docs for KRadioButtonGroup

a8c1951

- Add KRadioButton method description - Add KRadioButtonGroup slot and props description

Update tableOfContents

3f2d8ea

Fix: fix tab issue for change language model

b1d09dc

Merge remote-tracking branch 'origin/KRadioButtonGroup-Component' int…

220eacf

…o KRadioButtonGroup-Component

Update KRadioButtonGroup component

29f339a

- Used ._componentTag instead of ._name - Changed onkeydown with onkeyup listener

muditchoudhary mentioned this pull request Jun 20, 2024

DO_NOT_MERGE_ONLY_FOR_QA KRadioButtonGroup testing learningequality/kolibri#12325

Draft

9 tasks

muditchoudhary added 3 commits July 10, 2024 16:23

fix lint

73e8e76

Change keydown event with keyup event

df58010

fix lint

0e9be90

MisRob reviewed Jul 15, 2024

View reviewed changes

MisRob mentioned this pull request Jul 15, 2024

issue-#10491 Allow moving with arrow keys over radio buttons in firefox #554

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue #650

Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue #650

muditchoudhary commented Jun 4, 2024

muditchoudhary commented Jun 4, 2024 •

edited

Loading

MisRob commented Jun 5, 2024

muditchoudhary commented Jun 5, 2024

MisRob left a comment •

edited

Loading

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 14, 2024

MisRob Jul 15, 2024

muditchoudhary commented Jul 17, 2024

Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue #650

Are you sure you want to change the base?

Made a new KRadioButtonGroup component to fix Firefox radio button movement with arrow keys issue #650

Conversation

muditchoudhary commented Jun 4, 2024

Description

Issue addressed

Before/after screenshots

Before

After

Changelog

Steps to test

Second way

(optional) Implementation notes

At a high level, how did you implement this?

Does this introduce any tech-debt items?

Testing checklist

Reviewer guidance

After review

Comments

muditchoudhary commented Jun 4, 2024 • edited Loading

MisRob commented Jun 5, 2024

muditchoudhary commented Jun 5, 2024

MisRob left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muditchoudhary commented Jul 17, 2024

muditchoudhary commented Jun 4, 2024 •

edited

Loading

MisRob left a comment •

edited

Loading