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.

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

fix: Changing select open property errors #5586

Closed
rajsite opened this issue Feb 9, 2022 · 1 comment · Fixed by #5590
Closed

fix: Changing select open property errors #5586

rajsite opened this issue Feb 9, 2022 · 1 comment · Fixed by #5590
Assignees
Labels
bug A bug community:request Issues specifically reported by a member of the community. status:needs-investigation Needs additional investigation

Comments

@rajsite
Copy link

rajsite commented Feb 9, 2022

🐛 Bug Report

Changing the value of the open property results in an error in the select control.

When creating a control programmatically and setting the open property, the following codepath is triggered:

this.ariaControls = this.listbox.id;

Resulting in error:

Uncaught TypeError: Cannot read properties of undefined (reading 'id')

💻 Repro or Code Sample

const select = document.createElement('fast-select');
select.innerHTML = `
  <fast-option value="one">One</fast-option>
  <fast-option value="two">Two</fast-option>
  <fast-option value="three">Three</fast-option>
`;
select.open = true; // throw exception

Stackblitz: https://stackblitz.com/edit/typescript-q93mmy

🤔 Expected Behavior

Expect to be able to manipulate property values before an element has been connected to the DOM.

😯 Current Behavior

Manipulating the open property on Select results in an error.

💁 Possible Solution

Curious what approaches there are to protect this kind of situation. Seems like tests for setting properties programmatically may be useful.

🔦 Context

Tried to update to latest FAST. We have some tests where we copied the fixture.ts and the following fixture started to fail:

async function setup(
    position?: string,
    open?: boolean
): Promise<Fixture<Select>> {
    const viewTemplate = html`
        <nimble-select
            ${position !== undefined ? `position="${position}"` : ''}
            ${open ? 'open' : ''}
        >
            <nimble-listbox-option value="one">One</nimble-listbox-option>
            <nimble-listbox-option value="two">Two</nimble-listbox-option>
            <nimble-listbox-option value="three">Three</nimble-listbox-option>
        </nimble-select>
    `;
    return fixture<Select>(viewTemplate);
}

where setup is called as setup('above', true);

🌍 Your Environment

  • OS & Device: Win10 PC
  • Browser Chrome 98
  • Version fast-components 2.21.2
@rajsite rajsite added the status:triage New Issue - needs triage label Feb 9, 2022
@rajsite rajsite changed the title fix: Changing select open property errors fix: Changing select open property errors Feb 9, 2022
@EisenbergEffect EisenbergEffect added area:fast-components bug A bug community:request Issues specifically reported by a member of the community. status:needs-investigation Needs additional investigation and removed status:triage New Issue - needs triage labels Feb 10, 2022
@chrisdholt
Copy link
Member

@radium-v can you investigate when able?

rajsite added a commit to ni/nimble that referenced this issue Feb 11, 2022
# Pull Request

## 🤨 Rationale

Updates FAST dependencies to the latest version we can. Unfortunately [hit an issue with select controls](microsoft/fast#5586) that manifests in the [storybook matrix tests](https://60e89457a987cf003efc0a5b-uxxekwhpfi.chromatic.com/?path=/story/tests-select--select-opened-theme-matrix) in an example branch (see console).

Fixes #339 

## 👩‍💻 Implementation

- Updated FAST packages to latest except fast-foundation which is updated to the latest version without the select issue
- Modified fast-foundation range specifier to avoid package with error

## 🧪 Testing

Rely on existing tests and CI.

## ✅ Checklist

- [x] I have updated the project documentation to reflect my changes or determined no changes are needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug community:request Issues specifically reported by a member of the community. status:needs-investigation Needs additional investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants