Skip to content

Commit

Permalink
fix(ui5-radiobutton): fix tab order within group (#2783)
Browse files Browse the repository at this point in the history 
We used to set tabindex="0" to the selected item and tabindex="-1" to the rest, which works fine in all cases, besides the one reported in the issue - when the application wants to define a group without preselected item. In this case, all the radio buttons in the group have tabindex="-1" and as a result the group is not accessible via the keyboard.
Now, we calculate the tabindex for each item on every important action, such as: selection of item within the group, creating a new group, adding an item to the group or removal an item from the group. The logic considers the use-case from the issue as well - If the group does not have a selected item, the first (not disabled) item would have tabindex="0".
In openui5, this is handled by the sap.m.RadioButtonGroup control, here it is implemented within the RadioButtonGroup util class.

FIXES: #2774
  • Loading branch information
@ilhan007
ilhan007 committed Feb 8, 2021
1 parent f21cbc9 commit 6ca5a97
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 22 deletions.
16 changes: 14 additions & 2 deletions packages/main/src/RadioButton.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ const metadata = {
wrap: {
type: Boolean,
},

_tabIndex: {
type: String,
defaultValue: "-1",
noAttribute: true,
},
},
events: /** @lends sap.ui.webcomponents.main.RadioButton.prototype */ {

Expand Down Expand Up @@ -240,13 +246,14 @@ class RadioButton extends UI5Element {

onBeforeRendering() {
this.syncGroup();

this._enableFormSupport();
}

syncGroup() {
const oldGroup = this._name;
const currentGroup = this.name;
const oldSelected = this._selected;
const currentSelected = this.selected;

if (currentGroup !== oldGroup) {
if (oldGroup) {
Expand All @@ -262,7 +269,12 @@ class RadioButton extends UI5Element {
RadioButtonGroup.enforceSingleSelection(this, currentGroup);
}

if (this.name && currentSelected !== oldSelected) {
RadioButtonGroup.updateTabOrder(this.name);
}

this._name = this.name;
this._selected = this.selected;
}

_enableFormSupport() {
Expand Down Expand Up @@ -395,7 +407,7 @@ class RadioButton extends UI5Element {
}

if (this.name) {
return this.selected ? "0" : "-1";
return this._tabIndex;
}

return tabindex || "0";
Expand Down
24 changes: 24 additions & 0 deletions packages/main/src/RadioButtonGroup.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class RadioButtonGroup {
} else {
this.createGroup(radioBtn, groupName);
}

this.updateTabOrder(groupName);
}

static removeFromGroup(radioBtn, groupName) {
Expand All @@ -48,6 +50,8 @@ class RadioButtonGroup {
if (!group.length) {
this.removeGroup(groupName);
}

this.updateTabOrder(groupName);
}

static createGroup(radioBtn, groupName) {
Expand All @@ -72,6 +76,23 @@ class RadioButtonGroup {
this.updateSelectionInGroup(nextItemToSelect, groupName);
}

static updateTabOrder(groupName) {
if (!this.hasGroup(groupName)) {
return;
}

const group = this.getGroup(groupName);
const hasSelectedRadio = group.some(radioBtn => radioBtn.selected);

group.filter(radioBtn => !radioBtn.disabled).forEach((radioBtn, idx) => {
if (hasSelectedRadio) {
radioBtn._tabIndex = radioBtn.selected ? "0" : "-1";
} else {
radioBtn._tabIndex = idx === 0 ? "0" : "-1";
}
});
}

static selectPreviousItem(item, groupName) {
const group = this.getGroup(groupName),
groupLength = group.length,
Expand All @@ -88,6 +109,7 @@ class RadioButtonGroup {

static selectItem(item, groupName) {
this.updateSelectionInGroup(item, groupName);
this.updateTabOrder(groupName);
}

static updateSelectionInGroup(radioBtnToSelect, groupName) {
Expand Down Expand Up @@ -161,6 +183,8 @@ class RadioButtonGroup {
} else if (radioBtn === selectedRadio) {
this.selectedRadios.set(groupName, null);
}

this.updateTabOrder(groupName);
}

static get groups() {
Expand Down
57 changes: 38 additions & 19 deletions packages/main/test/pages/RadioButton.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,50 +17,57 @@
.radio-section {
display: flex;
flex-direction: column;
margin-bottom: 2rem;
}
</style>
</head>

<body style="background-color: var(--sapBackgroundColor);">
<ui5-title level="H1">ui5-radiobutton</ui5-title>
<ui5-radiobutton id="rb1"></ui5-radiobutton>
<ui5-radiobutton id="rb2" text="Option B"></ui5-radiobutton>
<ui5-radiobutton id="rb3" wrap text="Option C"></ui5-radiobutton>
<ui5-radiobutton id="rb4" disabled text="Option D"></ui5-radiobutton>
<ui5-radiobutton id="truncatingRb" text="Long long long text that should truncate at some point">></ui5-radiobutton>
<br/>
<ui5-radiobutton id="wrappingRb" wrap text="Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s." style="width: 300px"></ui5-radiobutton>

<ui5-input id="field"></ui5-input>
<ui5-title level="H1">Radios within a group</ui5-title><br>
<div class="radio-section">
<ui5-title>Group1 - None pre-selection</ui5-title>
<ui5-radiobutton id="testRbtn" text="N/A" disabled name="test"></ui5-radiobutton>
<ui5-radiobutton id="testRbtn1" text="Male" name="test"></ui5-radiobutton>
<ui5-radiobutton id="testRbtn2"text="Female" name="test"></ui5-radiobutton>
</div>

<section class="radio-section">
<ui5-title>Group 2</ui5-title>
<ui5-radiobutton id="testRbtn11" name="test2" selected text="Selected A"></ui5-radiobutton>
<ui5-radiobutton id="testRbtn12" name="test2" disabled text="Disabled B"></ui5-radiobutton>
<ui5-radiobutton id="testRbtn13" name="test2" text="Standard C"></ui5-radiobutton>
</section>

<section class="radio-section">
<ui5-title>ui5-radiobutton in group</ui5-title>
<ui5-title>Group 3</ui5-title>
<ui5-input id="tabField"></ui5-input>
<ui5-radiobutton id="groupRb1" name="a" selected text="Option A long long should shrink long long text text text text text text text text"></ui5-radiobutton>
<ui5-radiobutton id="groupRb2" name="a" disabled text="Option C"></ui5-radiobutton>
<ui5-radiobutton id="groupRbReadOnly" name="a" readonly text="Option E"></ui5-radiobutton>
<ui5-radiobutton id="groupRb3" name="a" text="Option D"></ui5-radiobutton>
<ui5-radiobutton id="groupRbReadOnly" name="a" readonly text="Option E"></ui5-radiobutton>
</section>

<section class="radio-section">
<ui5-title>ui5-radiobutton in group</ui5-title>
<ui5-title>Group 4</ui5-title>
<ui5-radiobutton id="groupRb4" name="b" selected text="Option A long long should shrink long long text text text text text text text text"></ui5-radiobutton>
<ui5-radiobutton id="groupRb5" name="b" disabled text="Option C"></ui5-radiobutton>
<ui5-radiobutton id="groupRb6" name="b" text="Option D"></ui5-radiobutton>
</section>

<div id="radioGroup" class="radio-section">
<ui5-title>Group of states</ui5-title>
<ui5-label id="lblRadioGroup"></ui5-label>
<ui5-label id="lblEventCounter"></ui5-label>
<section id="radioGroup" class="radio-section">
<ui5-title>Group 4 - Value states</ui5-title>
<ui5-label id="lblRadioGroup">N/A</ui5-label>
<ui5-label id="lblEventCounter">0</ui5-label>
<ui5-radiobutton id="groupRb7" text="None selected" value-state="None" name="GroupB"></ui5-radiobutton>
<ui5-radiobutton id="groupRb8" text="Warning" value-state="Warning" selected name="GroupB"></ui5-radiobutton>
<ui5-radiobutton id="groupRb9"text="Error" value-state="Error" selected name="GroupB"></ui5-radiobutton>
<ui5-radiobutton id="groupRb10" text="Default selected" value-state="None" selected name="GroupB"></ui5-radiobutton>
<ui5-radiobutton id="groupRb11" text="Other group selected" value-state="None" selected name="GroupBB"></ui5-radiobutton>
</div>
<ui5-radiobutton id="groupRb11" text="Radio From Another Group" value-state="None" selected name="GroupBB"></ui5-radiobutton>
</section>

<ui5-title level="H1">Standalone Radios Not Grouped</ui5-title>
<section>
<ui5-title>ui5-radiobutton states</ui5-title></p>
<ui5-radiobutton id="myRb6" selected text="Default"></ui5-radiobutton>
<ui5-radiobutton id="myRb7" readonly text="read only"></ui5-radiobutton>
<ui5-radiobutton id="myRb8" disabled text="disabled"></ui5-radiobutton>
Expand All @@ -72,6 +79,18 @@
<ui5-radiobutton id="myRb14" value-state="Error" selected text="error"></ui5-radiobutton>
</section>


<section class="radio-section">
<ui5-radiobutton id="rb1"></ui5-radiobutton>
<ui5-radiobutton id="rb2" text="Option B"></ui5-radiobutton>
<ui5-radiobutton id="rb3" wrap text="Option C"></ui5-radiobutton>
<ui5-radiobutton id="rb4" disabled text="Option D"></ui5-radiobutton>
<ui5-radiobutton id="truncatingRb" text="Long long long text that should truncate at some point">></ui5-radiobutton>
<br/>
<ui5-radiobutton id="wrappingRb" wrap text="Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s." style="width: 300px"></ui5-radiobutton>
<ui5-input id="field"></ui5-input>
</section>

<p>*Params</p>
<p>
<ui5-label>- for compact add 'ui5-content-density-compact' class to any dom element</ui5-label>
Expand Down
26 changes: 25 additions & 1 deletion packages/main/test/specs/RadioButton.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe("RadioButton general interaction", () => {
});

it("tests radio buttons selection within group with ARROW-RIGHT key", () => {
const field = browser.$("#field");
const field = browser.$("#tabField");
const radioButtonPreviouslySelected = browser.$("#groupRb1");
const radioButtonToBeSelected = browser.$("#groupRb3");

Expand All @@ -81,14 +81,38 @@ describe("RadioButton general interaction", () => {
assert.ok(radioButtonToBeSelected.getProperty("selected"), "Pressing ArrowLeft selects the next (not disabled) radio in the group.");
});

it("tests tabindex within group with selected item", () => {
const selectedRadio = browser.$("#testRbtn11").shadow$(".ui5-radio-root");
const disabledRadio = browser.$("#testRbtn12").shadow$(".ui5-radio-root");
const radio = browser.$("#testRbtn13").shadow$(".ui5-radio-root");

assert.strictEqual(selectedRadio.getAttribute("tabindex"), "0", "The selected radio has tabindex = 0");
assert.strictEqual(disabledRadio.getAttribute("tabindex"), "-1", "The disabled radio has tabindex = -1");
assert.strictEqual(radio.getAttribute("tabindex"), "-1", "None selected item has tabindex = -1");
});

it("tests tabindex within group with no selected item", () => {
const radio1 = browser.$("#testRbtn1").shadow$(".ui5-radio-root");
const radio2 = browser.$("#testRbtn2").shadow$(".ui5-radio-root");

assert.strictEqual(radio1.getAttribute("tabindex"), "0", "The first radio has tabindex = 0");
assert.strictEqual(radio2.getAttribute("tabindex"), "-1", "The other radio has tabindex = -1");
});

it("tests radio buttons selection within group by clicking", () => {
const radioButtonPreviouslySelected = browser.$("#groupRb6");
const radioButtonPreviouslySelectedRoot = browser.$("#groupRb6").shadow$(".ui5-radio-root");

const radioButtonToBeSelected = browser.$("#groupRb4");
const radioButtonToBeSelectedRoot = browser.$("#groupRb4").shadow$(".ui5-radio-root");

radioButtonToBeSelected.click();

assert.ok(!radioButtonPreviouslySelected.getProperty("selected"), "Previously selected item has been de-selected.");
assert.strictEqual(radioButtonPreviouslySelectedRoot.getAttribute("tabindex"), "-1", "The previously selected radio has tabindex = -1");

assert.ok(radioButtonToBeSelected.getProperty("selected"), "Pressing ArrowRight selects the next (not disabled) radio in the group.");
assert.strictEqual(radioButtonToBeSelectedRoot.getAttribute("tabindex"), "0", "The newly selected radio has tabindex = 0");
});

it("tests single selection within group, even if multiple radios are set as selected", () => {
Expand Down

0 comments on commit 6ca5a97

Please sign in to comment.