Fix dropdown selection if multiple entries have identical labels#4180
Fix dropdown selection if multiple entries have identical labels#4180hemu wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates MenuList.svelte to determine the active menu entry by comparing its value instead of its label against the highlighted entry's value. The reviewer pointed out that this fix is incomplete because other parts of the file, such as keyboard navigation and highlight tracking, still rely on the label property, which can cause bugs when entries have duplicate labels.
| <LayoutRow | ||
| class="row" | ||
| classes={{ open: openChildValue === entry.value, active: entry.label === highlighted?.label, disabled: Boolean(entry.disabled) }} | ||
| classes={{ open: openChildValue === entry.value, active: entry.value === highlighted?.value, disabled: Boolean(entry.disabled) }} |
There was a problem hiding this comment.
While changing the active class check to use entry.value instead of entry.label is correct, this fix is incomplete. There are other places in this file that still identify the highlighted entry by its label instead of its value, which will cause bugs (such as getting stuck during keyboard navigation or highlighting the wrong item) when multiple entries have identical labels:
- Keyboard Navigation (line 338):
const index = highlighted ? flatEntries.map((entry) => entry.value).indexOf(highlighted.value) : 0;This should be updated to use entry.value and highlighted.value so that pressing ArrowUp/ArrowDown doesn't always jump to the first entry with a duplicate label.
- Highlight Tracking (line 100):
const highlightedFound = highlighted?.value && flattened.map((entry) => entry.value).includes(highlighted.value);This should also use value instead of label to correctly verify if the highlighted entry exists in the filtered list.
There was a problem hiding this comment.
i updated the other lines called out here
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
I originally started looking into this because of this issue: #3878 But i don't think both artboards were being exported if they identical names in that case like that issue mentions. I think it is just a visual bug? If you have multiple artboards and they overlap, the data from both does end up in the export which seems surprising but a separate issue. E.g. if I try to export the outer Artboard_RED here with the red shape, the green shape from Artboard_GREEN will be included. Setup
Export Dialog
Exported image
|
| if ((menuOpen || interactive) && (e.key === "ArrowUp" || e.key === "ArrowDown")) { | ||
| let newIndex = e.key === "ArrowUp" ? flatEntries.length - 1 : 0; | ||
| if (highlighted) { | ||
| const index = highlighted ? flatEntries.map((entry) => entry.label).indexOf(highlighted.label) : 0; |
There was a problem hiding this comment.
highlighted ? is redundant b/c we're already in if (highlighted) clause
1 participant
If a MenuList contains multiple entries with identical labels, currently both entries will appear selected.
This PR ensures only one entry appears selected.
This is relevant in the Export Dialog, which renders a dropdown with user specified Artboard names which can be identical.
I think this is related to this issue: #3878