Design for message-list typing indication - #12418
Conversation
vaadin-review-bot
left a comment
vaadin-review-bot
left a comment
There was a problem hiding this comment.
Reviewed the changes — left 8 comments.
| Finding | |
|---|---|
The usersTyping JSDoc documents userName/userColorIndex, but the code reads user.name and feeds objects to vaadin-avatar-group.items, which expects name/colorIndex. |
|
The new host click listener runs this._textArea.focus() for any click anywhere in the component, including the new slotted controls. |
|
For a custom icon-only slot="button", the observer overwrites any developer-provided aria-label with i18n.send and never removes it. |
|
| 🧹 | New public properties usersTyping, typingMessageText, typingMessageTheme are missing from the sibling .d.ts, and the two string props lack @attr tags. |
| 🧹 | The new __colorIndexChanged observer sets --vaadin-user-color on the message, which _renderMessages already writes inline in the list template. |
| 👀 | .typing="${item.typing}" binds to a typing property that vaadin-message does not define, so the binding does nothing. |
| 👀 | @keyframes typing and @keyframes typing2 are defined but never referenced by any animation declaration. |
typingMessageText is baked into the typing element only at creation and has no observer, so later changes are ignored. |
| this._tooltipController = new TooltipController(this); | ||
| this.addController(this._tooltipController); | ||
|
|
||
| this.addEventListener('click', () => this._textArea.focus()); |
There was a problem hiding this comment.
click listener runs this._textArea.focus() for any click anywhere in the component, including the new slotted controls.
This PR adds header, prefix, footer, and custom button slots that hold interactive controls (in the dev demo: a paperclip upload button in prefix, an upload-file-list in header, prompt buttons in footer). Clicking any of them bubbles to the host and immediately pulls focus into the textarea, so the control the user just activated loses focus.
This hurts keyboard and screen-reader users and is generically wrong — the intent (focus the textarea when clicking the input's own padding) should be limited to the host itself, not the whole slotted subtree.
vaadin-message-input-mixin.js:128 · correctness · confirmed
| button.textContent = effectiveI18n.send; | ||
| if (button.localName === 'vaadin-message-input-button') { | ||
| button.textContent = effectiveI18n.send; | ||
| } else if (button.textContent.trim().length === 0) { |
There was a problem hiding this comment.
slot="button", the observer overwrites any developer-provided aria-label with i18n.send and never removes it.
A developer supplies <vaadin-button slot="button" aria-label="Send message"><vaadin-icon .../></vaadin-button>. Because button.textContent.trim().length === 0, __buttonPropsChanged overwrites the intended label with the generic "Send". In the dev demo the button has a "Send prompt" tooltip, so its accessible name becomes the mismatched "Send".
The branch also never clears the label: a button that starts icon-only (gets aria-label="Send") and later gains visible text keeps a stale aria-label, so screen readers announce "Send" instead of the visible text. Check for an existing label before setting one, and clear it when text is present.
vaadin-message-input-mixin.js:143 · correctness · confirmed
| * }> | ||
| * ``` | ||
| */ | ||
| usersTyping: { |
There was a problem hiding this comment.
🧹 New public properties usersTyping, typingMessageText, typingMessageTheme are missing from the sibling .d.ts, and the two string props lack @attr tags.
CONVENTIONS.md line 143: "When adding a public property, method, or config-object option to a .js module, it must also be declared in the sibling hand-maintained .d.ts." vaadin-message-list-mixin.d.ts still declares only items, markdown, and announceMessages, so TypeScript users cannot type-check the three new properties.
CONVENTIONS.md line 35 also requires camelCase simple-type props to carry an @attr {type} kebab-name tag; typingMessageText and typingMessageTheme (both String) have none. Add the .d.ts declarations and @attr {string} typing-message-text / typing-message-theme tags.
vaadin-message-list-mixin.js:81 · conventions · confirmed
| } | ||
| } | ||
|
|
||
| /** @private */ |
There was a problem hiding this comment.
🧹 The new __colorIndexChanged observer sets --vaadin-user-color on the message, which _renderMessages already writes inline in the list template.
vaadin-message-list-mixin.js line 258 already emits style="--vaadin-user-color: var(--vaadin-user-color-${item.userColorIndex});..." on the same <vaadin-message>, so the new observer duplicates that write on every list-rendered message — the only supported render path.
The two sources target the same inline style and must be kept in sync by future edits, or the logic silently diverges. Keep one mechanism; the observer is redundant for list-rendered messages.
vaadin-message-mixin.js:119 · reuse · confirmed
| avatars.maxItemsVisible = 100; | ||
| avatars.slot = 'avatar'; | ||
| const typingText = document.createElement('div'); | ||
| typingText.innerHTML = `<span></span> ${this.typingMessageText}`; |
There was a problem hiding this comment.
typingMessageText is baked into the typing element only at creation and has no observer, so later changes are ignored.
The label text is set once via typingText.innerHTML = \ ${this.typingMessageText}`when the element is first created. The update branch (line 226) only rewrites the inner(the user-name list), never the trailing label, andtypingMessageText` has no observer to re-render.
So setting typingMessageText after the first typing indicator has rendered silently keeps the original text. Confirming needs a run where typingMessageText is changed between two usersTyping updates.
vaadin-message-list-mixin.js:214 · correctness · plausible
41dca05 to
d006c5d
Compare
d006c5d to
10272ec
Compare
|
2 participants
Changes
Message List
--vaadin-message-list-max-widthcustom property, for restricting the width of the messages in the list and centering them in the scrolling viewport.typing-indicatorslot for a message that’s always the last one in the list, where a "user is typing" indication is shown.bubbletheme variant, which adds a visual container around the message content, and has a max-width that is less than 100%.one-to-onetheme variant: works together with the bubble variant, and hides the avatars and names of both participants in the message list (expecting the context, e.g. a header or a menu item, to indicate who you are talking with).usersTypingJS property: an array of users who are currently typing a message. When set, a message is added to thetypingslot in the DOM, with an avatar-group indicating all the users currently typing a message.typingIndicatorTextJS property: a string which is appended after the list of users' names who are currently typing a message.typingIndicatorThemeJS property: a string that sets thethemeattribute on the message in thetyping-indicatorslot (e.g., set tofull-widthin a one-to-one discussion with an AI assistant).Message
Two theme variants meant to be used together with the message list
bubblevariant:self: indicating the messages of the current user. These messages use a different background color and are aligned to the other side of the message list, and always hide the avatar and name.full-widththeme variant: hides the bubble background and allows the message to take full width of the list. Meant to be used for "assistant"/AI responses.--vaadin-user-colorcustom property set on the host, to allow custom styling based on thecolorIndexproperty.typingattribute set on the host to indicate this message is the typing indicator (used as a styling hook).ellipsistheme variant: only works together with thetypingattribute. Hides the text from the message in thetypingslot, and shows an ellipsis instead.testtheme variant: only works together with thetypingattribute. Shows the names of the users typing and the typing indicator text on a single line (no avatar, no background).Message Input
New slots for additional content:
prefix: before the input/textareaheader: above the prefix, input, and submit buttonfooter: below the prefix, input, and submit buttonUpload File List
emptyattribute applied when the list is empty, allowing styles to hide the element in that case, so that layouts can avoid adding gaps when not needed (which is the case when placing the upload-file-list in the message-inputheaderslot).