Support editing properties of type selector and selectorAll

[vincentfretin]

Add support for editing a property of type selector and selectorAll types.
Also refactor the previous fix 2162f9e that added support for type array without oneOf.

That backports changes from

• part of c-frame/aframe-editor@5af818c to support resetting a property by the entityupdate command, I didn't backport the ui for it though.
• Fix editing entity id, trigger the update only on blur and enter c-frame/aframe-editor#52 except we can't edit the id in 3dstreet
• Support editing properties of type array without oneOf, selector and selectorAll c-frame/aframe-editor#69

[claude]

Code Review: Support editing properties of type selector and selectorAll

Thanks for this PR, @vincentfretin! The backport from c-frame/aframe-editor is well-scoped and the commit messages are excellent. Overall the approach is sound — here are my observations:

---

What this PR does

• Adds selector / selectorAll property editing in the inspector via InputWidget using onBlur (not onChange) to avoid querying the DOM on every keystroke.
• Generalises the old array-only value.split(',') in InputWidget to schema.parse(), also gaining whitespace trimming.
• Passes null through EntityUpdateCommand as newValue to support resetting properties (instead of calling stringify(null) and potentially throwing).
• Guards stringifyComponentValue in entity.js against undefined property values.

---

Strengths

• onBlur rationale is correct. Partial selectors are never valid mid-keystroke, and querying the DOM on every character is wasteful. The comment explains this clearly.
• stringifyValue is a real bug fix. The old this.props.value || '' would coerce false or 0 to ''; the new helper handles null/undefined explicitly while preserving falsy-but-valid values.
• schema.parse() generalisation is cleaner than the special-case split(',') and the whitespace-trimming side-effect is a genuine improvement.
• entity.js fix is well-targeted — undefined values reaching stringify after a property reset is a real edge case.
• updateProperty extraction removes the duplicated AFRAME.INSPECTOR.execute block cleanly.
• payload.entity → entity cleanup in EntityUpdateCommand reduces noise and is internally consistent.

---

Issues / Suggestions

1. Deprecated keyCode — use e.key instead (InputWidget.js)

// current
if (e.keyCode === 13) {

// preferred
if (e.key === 'Enter') {

keyCode has been deprecated in the DOM spec for years and removed from TypeScript strict-mode typings. e.key === 'Enter' is the correct modern form.

---

2. stopPropagation() fires for all keydown events, not just Enter (InputWidget.js)

onKeyDown = (e) => {
  e.stopPropagation();   // ← runs unconditionally

  if (e.keyCode === 13) {
    this.input.current.blur();
    return;
  }
};

This is new global behaviour for every InputWidget. It means that while a text field is focused, no keydown event (Delete, Ctrl+Z, Escape, arrow keys, etc.) can reach the inspector's document-level listeners. This may be intentional — preventing Delete from removing the selected entity while typing is arguably correct. But it's a side-effect worth explicitly acknowledging in the commit message / PR description, since it touches all string fields, not just selector ones.

If intentional: a short comment (// prevent A-Frame keyboard shortcuts from firing while typing) would help future readers. If unintentional: move stopPropagation inside the if block.

---

3. No Escape-to-cancel on selector fields

When onBlur is the commit trigger, pressing Escape or clicking away both commit the partial selector. A typical UX pattern is to restore the pre-edit value on Escape. Not a blocker, but worth a follow-up issue since onBlur exits without a cancel path.

---

4. componentDidUpdate — consider skipping update while focused (InputWidget.js)

componentDidUpdate(prevProps) {
  if (this.props.value !== prevProps.value) {
    this.setState({ value: this.stringifyValue(this.props.value) });
  }
}

For the new onBlur-only path the user types freely, and an external prop update (e.g., from undo) could overwrite their in-progress text mid-typing. Consider checking document.activeElement !== this.input.current before setState, so external updates don't interrupt typing. The existing onChange path has the same issue in principle, but it was less visible because it committed on every keystroke.

---

5. parseInput swallows schema errors silently

parseInput = (value) => {
  if (this.props.schema) {
    return this.props.schema.parse(value);
  }
  return value;
};

schema.parse can throw for malformed input (e.g., a bad vec3). If that happens the onChange call chain will propagate the exception up uncaught. Wrapping in a try/catch and returning the raw string as a fallback would be more resilient, though admittedly this was also true of the old split(',') path.

---

6. Test coverage

No tests are included. Given the complexity of the null-value path, the onBlur commit flow, and the stringifyValue edge cases (false, 0, null, undefined), a few unit tests for InputWidget and EntityUpdateCommand would help prevent regressions — especially since InputWidget is used by virtually every non-boolean, non-select property in the editor.

---

Minor nits

• widgetProps spreading (...(isSelectorType ? { onBlur } : { onChange })) is clear and idiomatic — nice pattern.
• The value: this.state.value in render (removing the redundant || '') is safe since stringifyValue guarantees a string.

---

Summary

The core approach is correct and well-reasoned. The two items I'd prioritise before merging are:

1. e.key === 'Enter' (trivial fix, removes deprecated API)
2. Clarify / constrain the stopPropagation scope (affects all InputWidget instances, worth an explicit decision)

Everything else is either a follow-up improvement or a minor observation.

[vincentfretin]

@claude about the e.stopPropagation() in onKeyDown, I did it similar to NumberWidget without thinking too much, you're right that it prevents the event to emit on the document and that's on purpose, but actually we also have the shouldCaptureKeyEvent in shortcuts.js that return false if we're in an input, so even without the e.stopPropagation(), the event will be discarded and won't trigger the delete confirmation of the selected entity if we press Delete inside the input. I'd say we keep it, that's fine, and ctrl+z and arrows actually works correctly with it. Esc doesn't work as you describe with or without it.

[claude]

Claude finished @vincentfretin's task in 1m 22s —— View job

---

Code Review: Support editing properties of type selector and selectorAll

Thanks for this PR, @vincentfretin! The backport from c-frame/aframe-editor is well-scoped and the commit messages are excellent. Overall the approach is sound — here are my observations:

---

What this PR does

• Adds selector / selectorAll property editing in the inspector via InputWidget using onBlur (not onChange) to avoid querying the DOM on every keystroke.
• Generalises the old array-only value.split(',') in InputWidget to schema.parse(), also gaining whitespace trimming.
• Passes null through EntityUpdateCommand as newValue to support resetting properties (instead of calling stringify(null) and potentially throwing).
• Guards stringifyComponentValue in entity.js against undefined property values.

---

Strengths

• onBlur rationale is correct. Partial selectors are never valid mid-keystroke, and querying the DOM on every character is wasteful. The comment explains this clearly.
• stringifyValue is a real bug fix. The old this.props.value || '' would coerce false or 0 to ''; the new helper handles null/undefined explicitly while preserving falsy-but-valid values.
• schema.parse() generalisation is cleaner than the special-case split(',') and the whitespace-trimming side-effect is a genuine improvement.
• entity.js fix is well-targeted — undefined values reaching stringify after a property reset is a real edge case.
• updateProperty extraction removes the duplicated AFRAME.INSPECTOR.execute block cleanly.
• payload.entity → entity cleanup in EntityUpdateCommand reduces noise and is internally consistent.

---

Issues / Suggestions

1. Deprecated keyCode — use e.key instead (InputWidget.js)

// current
if (e.keyCode === 13) {

// preferred
if (e.key === 'Enter') {

keyCode has been deprecated in the DOM spec for years and removed from TypeScript strict-mode typings. e.key === 'Enter' is the correct modern form.

---

2. stopPropagation() scope — resolved ✓

@vincentfretin clarified: shouldCaptureKeyEvent in shortcuts.js already returns false for INPUT/TEXTAREA targets (line 15), so keyboard shortcuts (Delete, etc.) are guarded by the event target check independently of stopPropagation(). The behaviour is consistent with NumberWidget which does the same thing. Ctrl+Z, arrows, and Esc all behave correctly. No action needed.

---

3. No Escape-to-cancel on selector fields

When onBlur is the commit trigger, pressing Escape or clicking away both commit the partial selector. A typical UX pattern is to restore the pre-edit value on Escape. Not a blocker, but worth a follow-up issue since onBlur exits without a cancel path.

---

4. componentDidUpdate — consider skipping update while focused (InputWidget.js)

componentDidUpdate(prevProps) {
  if (this.props.value !== prevProps.value) {
    this.setState({ value: this.stringifyValue(this.props.value) });
  }
}

For the new onBlur-only path the user types freely, and an external prop update (e.g., from undo) could overwrite their in-progress text mid-typing. Consider checking document.activeElement !== this.input.current before setState, so external updates don't interrupt typing. The existing onChange path has the same issue in principle, but it was less visible because it committed on every keystroke.

---

5. parseInput swallows schema errors silently

parseInput = (value) => {
  if (this.props.schema) {
    return this.props.schema.parse(value);
  }
  return value;
};

schema.parse can throw for malformed input (e.g., a bad vec3). If that happens the onChange call chain will propagate the exception up uncaught. Wrapping in a try/catch and returning the raw string as a fallback would be more resilient, though admittedly this was also true of the old split(',') path.

---

6. Test coverage

No tests are included. Given the complexity of the null-value path, the onBlur commit flow, and the stringifyValue edge cases (false, 0, null, undefined), a few unit tests for InputWidget and EntityUpdateCommand would help prevent regressions — especially since InputWidget is used by virtually every non-boolean, non-select property in the editor.

---

Minor nits

• widgetProps spreading (...(isSelectorType ? { onBlur } : { onChange })) is clear and idiomatic — nice pattern.
• The value: this.state.value in render (removing the redundant || '') is safe since stringifyValue guarantees a string.

---

Summary

The core approach is correct and well-reasoned. The main item I'd prioritise before merging:

1. e.key === 'Enter' (trivial fix, removes deprecated API) Fix this →

Items 3–6 are follow-up improvements rather than blockers.