scottyaslan commented on code in PR #11561:
URL: https://github.com/apache/nifi/pull/11561#discussion_r3846132633
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/searchable-select/searchable-select.component.html:
##########
Review Comment:
**Issue**
Non-virtual grouped rendering now highlights by reference identity
(`activeOptionRef() === option`), which is the right contract. The virtual
template still highlights by value
(`isOptionActiveByValue(getItemValue(item))`, template ~163-168), which leaves
grouped plus virtual with three distinct defects rather than one cosmetic one.
1. Duplicate DOM ids. `activeOptionId` is a single fixed id
(`getUniqueId('option-active')`), so value-based assignment hands that same id
to every row sharing the active value. Two rows can carry it at once, which is
invalid DOM and makes `aria-activedescendant` and `document.getElementById`
ambiguous.
2. Arrow order does not match render order, independent of duplicates.
`updateActiveTemplateIndex()` derives the active option from
`getVisibleOptions()[_activeOptionIndex]`, and in virtual mode
`getVisibleOptions()` returns `this._options` in source order. The rendered
list comes from `getVirtualVisibleItems()` via `groupedOptions()`, which emits
ungrouped first and then groups sorted with `localeCompare`. For interleaved
input the nth arrow press does not land on the nth rendered row, and
`getVirtualIndexForOptionIndex()` scrolls to the wrong row. Non-virtual grouped
avoids this only because its `getVisibleOptions()` branch returns
`groups.flatMap((g) => g.options)`, the same order the template renders.
3. Enter is still value-resolved in virtual mode. `toggleOption()` takes the
`enableVirtualScrolling()` branch and updates `_virtualSelectedValues` by
value, never consulting `activeOptionId` or `activeOptionRef` the way the
non-virtual branch does. With duplicate values Enter cannot express which row
was highlighted, even if the highlight is corrected.
**Why the current spec does not protect this**
`documents that virtual + groups highlight is value-based (duplicate values
unsupported)` asserts the defective behavior: `isOptionActiveByValue('dup') ===
true` and `visible.every((o) => isOptionActiveByValue(o.value)) === true`. That
spec fails only if someone fixes the component. It cannot fail when a caller
configures grouped plus virtual plus duplicate values and reproduces the
ambiguity in the UI. The comment on this input and the template comment are the
only remaining guardrails, and comments do not fail a build.
**Fix**
Please bring grouped plus virtual to the same identity contract as grouped
non-virtual in this PR.
1. Highlight by identity in the virtual template: replace
`isOptionActiveByValue(getItemValue(item))` with a reference comparison against
the item's option object for both `[id]` and `[class.mat-mdc-option-active]`.
This needs no restructuring today, because `groupedOptions()` pushes the
original `opt` references (`groupMap.get(opt.group)!.push(opt)`) and
`getVirtualVisibleItems()` pushes those same objects
(`group.options.forEach((opt) => items.push(opt))`), so `activeOptionRef() ===
asOptionItem(item)` is a valid comparison.
2. Make the navigation list match the rendered list: in the
`enableVirtualScrolling()` branch of `getVisibleOptions()`, return grouped
order when `hasGroupedOptions()` is true, using the same `groups.flatMap((g) =>
g.options)` as the non-virtual branch, so `_activeOptionIndex`,
`activeOptionRef`, and `getVirtualIndexForOptionIndex()` all agree with what
`cdkVirtualFor` renders.
3. Give `trackByVirtualItem` a group-scoped identity for option rows. It
currently returns `item.value`, so two rows sharing a value across groups
collide during `cdkVirtualFor` diffing. Group id plus value, or the option
reference, both work.
4. Resolve Enter from the highlighted row in the virtual branch of
`toggleOption()` rather than by value.
5. Replace the documenting spec with a regression asserting exactly one
active row and exactly one element carrying `activeOptionId`, plus coverage for
arrow order matching rendered grouped order, Enter selecting the highlighted
duplicate, a second Enter toggling the same row in multi-select, and an active
row scrolled outside the rendered buffer, since CDK recycles nodes and the
active element is not guaranteed to be mounted.
**Preferred outcome**
Grouped plus virtual behaves like grouped non-virtual: at most one active
row, one `activeOptionId` in the DOM, arrow order equal to render order, and
Enter acting on the highlighted row. The `documents that ... is value-based`
spec is gone, replaced by regressions that fail if the ambiguity returns.
If that is more than you want in this change, the acceptable alternative is
a follow-up Jira referenced from this input comment together with a guard that
makes the unsupported combination impossible to configure silently, for example
a development-mode warning when `enableVirtualScrolling()` is set on a grouped
list. Landing only comments plus a spec that locks the defect leaves nothing
that fails when a caller trips it.
##########
nifi-frontend/src/main/frontend/libs/shared/src/components/searchable-select/searchable-select.component.spec.ts:
##########
@@ -1835,6 +2427,29 @@ describe('SearchableSelect', () => {
expect(component.trackByVirtualItem(1, option)).toBe('aws-1');
});
+
+ it('documents that virtual + groups highlight is value-based
(duplicate values unsupported)', async () => {
Review Comment:
**Issue**
This spec locks the defect rather than the contract.
`expect(component.isOptionActiveByValue('dup')).toBe(true)` and
`expect(visible.every((o) =>
component.isOptionActiveByValue(o.value))).toBe(true)` pass precisely because
two rows sharing a value are both active. It fails only when someone corrects
the component, and never when a caller configures grouped plus virtual plus
duplicate values and reproduces the ambiguity.
**Fix**
Replace it with a regression matching the grouped non-virtual contract:
exactly one active row, exactly one element carrying `activeOptionId`, and
Enter acting on the highlighted row. The component changes this depends on are
described in the comment on `enableVirtualScrolling`.
**Preferred outcome**
No spec in this file asserts that duplicate rows are simultaneously active.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
