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.

Sign up for GitHub

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

test: improve tabs tests #6898

Merged
merged 14 commits into from
Jul 16, 2024
Merged

test: improve tabs tests #6898

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
73a0519
add changelog entry
bvandercar-vt Jun 25, 2024
9adf944
test: improve tabPanels tests
bvandercar-vt Jul 8, 2024
2c28bc4
style: rename var
bvandercar-vt Jul 8, 2024
82059b5
style: improve classes
bvandercar-vt Jul 8, 2024
bb1bd93
move checks
bvandercar-vt Jul 8, 2024
241696b
Merge branch 'develop' of https://github.com/bvandercar-vt/blueprint …
bvandercar-vt Jul 8, 2024
150687f
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 12, 2024
68d3042
Merge branch 'develop' into bvandercar/test/tabs
bvandercar-vt Jul 12, 2024
a6dfead
revert tabs selector
bvandercar-vt Jul 16, 2024
30dfe73
Delete packages/core/changelog/@unreleased/pr-6865.v2.yml
bvandercar-vt Jul 16, 2024
110923a
Merge branch 'develop' of https://github.com/palantir/blueprint into …
bvandercar-vt Jul 16, 2024
ad2559f
Merge branch 'develop' into bvandercar/test/tabs
bvandercar-vt Jul 16, 2024
f99a3a0
Merge branch 'bvandercar/test/tabs' of https://github.com/bvandercar-…
bvandercar-vt Jul 16, 2024
f27e6af
revert chagne
bvandercar-vt Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/core/changelog/@unreleased/pr-6865.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
fix:
description: '[core] a11y(Tooltip): wrap contents in "tooltip" aria role'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lemme know if you think the hostNodes change is important###

This PR is a precursor to #6896 , where this change is more beneficial

links:
- https://github.com/palantir/blueprint/pull/6865
62 changes: 37 additions & 25 deletions packages/core/test/tabs/tabsTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ describe("<Tabs>", () => {
// default tabs content is generated from these Dsin each test
const TAB_IDS = ["first", "second", "third"];

// selectors using ARIA role
const TAB = "[role='tab']";
const TAB_LIST = "[role='tablist']";
const TAB_PANEL = "[role='tabpanel']";
const TAB_SELECTOR = `.${Classes.TAB}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No other test grabs by role-- use Classes like the rest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always heard it's best to select elements in tests as close as possible as to how your users would. Ideally by actual text content, followed by accessibility roles/element types (ex user searches for and clicks a <button>), and as a last resort something like targeting by an id or special test-id attribute.

So I don't think we should change these tests just because other tests are selecting by class name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK! Changed.

const TAB_LIST_SELECTOR = `.${Classes.TAB_LIST}`;
const TAB_PANEL_SELECTOR = `.${Classes.TAB_PANEL}`;

let testsContainerElement: HTMLElement;

Expand Down Expand Up @@ -67,30 +66,35 @@ describe("<Tabs>", () => {
{getTabsContents()}
</Tabs>,
);
assert.lengthOf(wrapper.find(TAB), 3);
assert.lengthOf(wrapper.find(TAB_SELECTOR), 3);
assert.strictEqual(wrapper.state("selectedTabId"), TAB_IDS[0]);
});

it("renders one TabTitle for each Tab", () => {
it("renders one TabTitle and one TabPanel for each Tab, aria roles are correct", () => {
const wrapper = mount(<Tabs id={ID}>{getTabsContents()}</Tabs>);
assert.lengthOf(wrapper.find(TAB), 3);
assert.lengthOf(wrapper.find(TAB_SELECTOR), 3);
assert.lengthOf(wrapper.find(TAB_LIST_SELECTOR), 1);
assert.lengthOf(wrapper.find(TAB_PANEL_SELECTOR), 3);
assert.lengthOf(wrapper.find("[role='tab']"), 3);
assert.lengthOf(wrapper.find("[role='tablist']"), 1);
assert.lengthOf(wrapper.find("[role='tabpanel']"), 3);
});

it("renders all Tab children, but active is not aria-hidden", () => {
it("renders all Tab children, active is not aria-hidden", () => {
const activeIndex = 1;
const wrapper = mount(<Tabs id={ID}>{getTabsContents()}</Tabs>);
wrapper.setState({ selectedTabId: TAB_IDS[activeIndex] });
const tabs = wrapper.find(TAB_PANEL);
assert.lengthOf(tabs, 3);
const tabPanels = wrapper.find(TAB_PANEL_SELECTOR);
assert.lengthOf(tabPanels, 3);
for (let i = 0; i < TAB_IDS.length; i++) {
// hidden unless it is active
assert.equal(tabs.at(i).prop("aria-hidden"), i !== activeIndex);
assert.equal(tabPanels.at(i).prop("aria-hidden"), i !== activeIndex);
}
});

it(`renders without ${Classes.LARGE} when by default`, () => {
const wrapper = mount(<Tabs id={ID}>{getTabsContents()}</Tabs>);
assert.lengthOf(wrapper.find(`.${Classes.TAB_LIST}.${Classes.LARGE}`), 0);
assert.lengthOf(wrapper.find(`${TAB_LIST_SELECTOR}.${Classes.LARGE}`), 0);
});

it(`renders using ${Classes.LARGE} when large={true}`, () => {
Expand All @@ -99,7 +103,7 @@ describe("<Tabs>", () => {
{getTabsContents()}
</Tabs>,
);
assert.lengthOf(wrapper.find(`.${Classes.TAB_LIST}.${Classes.LARGE}`), 1);
assert.lengthOf(wrapper.find(`${TAB_LIST_SELECTOR}.${Classes.LARGE}`), 1);
});

it("attaches className to both tab and panel container if set", () => {
Expand All @@ -113,7 +117,10 @@ describe("<Tabs>", () => {
<Tab id="third" title="Third" className={tabClassName} panel={<Panel title="third" />} />,
</Tabs>,
);
assert.lengthOf(wrapper.find(`.${tabClassName}`), 9);
const NUM_TABS = 3;
assert.lengthOf(wrapper.find(TAB_SELECTOR), NUM_TABS);
assert.lengthOf(wrapper.find(TAB_PANEL_SELECTOR), NUM_TABS);
assert.lengthOf(wrapper.find(`.${tabClassName}`).hostNodes(), NUM_TABS * 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not have length 9, should have length 6. Need to use hostNodes to remove the React nodes.

});

it("attaches panelClassName to panel container if set", () => {
Expand All @@ -126,6 +133,9 @@ describe("<Tabs>", () => {
<Tab id="third" title="Third" panel={<Panel title="third" />} />,
</Tabs>,
);
const NUM_TABS = 3;
assert.lengthOf(wrapper.find(TAB_SELECTOR), NUM_TABS);
assert.lengthOf(wrapper.find(TAB_PANEL_SELECTOR), NUM_TABS);
assert.lengthOf(wrapper.find(`.${panelClassName}`), 1);
});

Expand Down Expand Up @@ -158,12 +168,12 @@ describe("<Tabs>", () => {

it("sets aria-* attributes with matching IDs", () => {
const wrapper = mount(<Tabs id={ID}>{getTabsContents()}</Tabs>);
wrapper.find(TAB).forEach(title => {
wrapper.find(TAB_SELECTOR).forEach(title => {
// title "controls" tab element
const titleControls = title.prop("aria-controls");
const tab = wrapper.find(`#${titleControls}`);
// tab element "labelled by" title element
assert.isTrue(tab.is(TAB_PANEL), "aria-controls isn't TAB_PANEL");
assert.isTrue(tab.is(TAB_PANEL_SELECTOR), "aria-controls isn't TAB_PANEL");
assert.deepEqual(tab.prop("aria-labelledby"), title.prop("id"), "mismatched IDs");
});
});
Expand All @@ -173,7 +183,7 @@ describe("<Tabs>", () => {
<Tab id={id} key={id} panel={<Panel title={id} />} title={id} data-arbitrary-attr="foo" />
));
const wrapper = mount(<Tabs id={ID}>{tabs}</Tabs>);
wrapper.find(TAB).forEach(title => {
wrapper.find(TAB_SELECTOR).forEach(title => {
assert.strictEqual((title.getDOMNode() as HTMLElement).getAttribute("data-arbitrary-attr"), "foo");
});
});
Expand Down Expand Up @@ -204,7 +214,7 @@ describe("<Tabs>", () => {
);
assert.equal(wrapper.state("selectedTabId"), TAB_IDS[0]);
// last Tab is inside nested
wrapper.find(TAB).last().simulate("click");
wrapper.find(TAB_SELECTOR).last().simulate("click");
assert.equal(wrapper.state("selectedTabId"), TAB_IDS[0]);
assert.isTrue(changeSpy.notCalled, "onChange invoked");
});
Expand All @@ -219,8 +229,8 @@ describe("<Tabs>", () => {
{ attachTo: testsContainerElement },
);

const tabList = wrapper.find(TAB_LIST);
const tabElements = testsContainerElement.querySelectorAll<HTMLElement>(TAB);
const tabList = wrapper.find(TAB_LIST_SELECTOR);
const tabElements = testsContainerElement.querySelectorAll<HTMLElement>(TAB_SELECTOR);
tabElements[0].focus();

tabList.simulate("keydown", { key: "ArrowRight" });
Expand All @@ -241,8 +251,8 @@ describe("<Tabs>", () => {
</Tabs>,
{ attachTo: testsContainerElement },
);
const tabList = wrapper.find(TAB_LIST);
const tabElements = testsContainerElement.querySelectorAll<HTMLElement>(TAB);
const tabList = wrapper.find(TAB_LIST_SELECTOR);
const tabElements = testsContainerElement.querySelectorAll<HTMLElement>(TAB_SELECTOR);

// must target different elements each time as onChange is only called when id changes
tabList.simulate("keypress", { target: tabElements[1], key: "Enter" });
Expand All @@ -260,7 +270,7 @@ describe("<Tabs>", () => {
</Tabs>,
);
assert.isUndefined(wrapper.state().indicatorWrapperStyle);
assert.equal(wrapper.find("." + Classes.TAB_INDICATOR).length, 0);
assert.equal(wrapper.find(`.${Classes.TAB_INDICATOR}`).length, 0);
});

it("removes indicator element when selected tab is removed", () => {
Expand Down Expand Up @@ -408,14 +418,16 @@ describe("<Tabs>", () => {
function findTabById(wrapper: ReactWrapper<TabsProps>, id: string) {
// Need this to get the right overload signature
// eslint-disable-line @typescript-eslint/consistent-type-assertions
return wrapper.find(TAB).filter({ "data-tab-id": id } as React.HTMLAttributes<HTMLElement>);
return wrapper.find(TAB_SELECTOR).filter({ "data-tab-id": id } as React.HTMLAttributes<HTMLElement>);
}

function assertIndicatorPosition(wrapper: ReactWrapper<TabsProps, TabsState>, selectedTabId: string) {
const style = wrapper.state().indicatorWrapperStyle;
assert.isDefined(style, "Tabs should have a indicatorWrapperStyle prop set");
const node = wrapper.getDOMNode();
const expected = node.querySelector<HTMLLIElement>(`${TAB}[data-tab-id='${selectedTabId}']`)!.offsetLeft;
const expected = node.querySelector<HTMLLIElement>(
`${TAB_SELECTOR}[data-tab-id='${selectedTabId}']`,
)!.offsetLeft;
assert.isTrue(style?.transform?.indexOf(`${expected}px`) !== -1, "indicator has not moved correctly");
}

Expand Down