Use getAttribute instead of accessing id directly

[lgoeckener]

If there is an input with name "id" in a form, element.id will target that input field instead of the id attribute of the form.

<form id="foo">
    <input type="hidden" name="id" value="1" />
</form>

Means, if in the current iteration of buildElementSelector, the element is the <form>-element,

element.id

will point to the input.

Therefore using element.getAttribute("id") will make sure, that the id-attribute of the current element is used.

[mshoho]

@lgoeckener thanks! Could you please add a test for the case?

[lgoeckener]

@mshoho I would love to! But I'm afraid I'm not quite sure in how to implement the test in the given environment.

I'm not familiar with the BroTest testing environment nor how I could create a scenario that would end up testing that particular branch.
Besides that I cannot create DOM elements since packages like JSDOM or react-testing-library are not installed.

If you give permission to install additional packages just for testing this branch (I would also need to export the buildElementSelector function), I could proceed doing so in a way that I'm familiar with.

[mshoho]

We intentionally don't do tests in a virtualized environment because for Tabster they make no sense (and don't quite work because of the JSDOM's limitations). We use real Chromium.

It would be enough to go to Deloser.test.tsx and add the following there:

    it("should restore focus in <form> with named inputs", async () => {
        await new BroTest.BroTest(
            (
                <form {...getTabsterAttribute({ root: {}, deloser: {} })}>
                    <button>Button1</button>
                    <input name="id" />
                    <button>Button2</button>
                </form>
            )
        )
            .pressTab()
            .pressTab()
            .activeElement((el) => {
                expect(el?.attributes.name).toEqual("id");
            })
            .removeElement()
            .wait(300)
            .activeElement((el) => {
                expect(el?.textContent).toEqual("Button1");
            });
    });

To run the tests you can just npm run test (or npm run test deloser to limit the scope).

And we're moving BroTest to a separate package (to reuse it in another repository), it is work-in-progress, but some documentation is expected to eventually appear here: https://github.com/microsoft/testbro. But for now it would be enough to just copy-paste the test above.

[lgoeckener]

@microsoft-github-policy-service agree