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
b1ea1ce to
41dca05
Compare
41dca05 to
d006c5d
Compare
|
2 participants
Includes a bunch of unrelated fixes and improvements to message-list and message-input, vaadin-message-input-button, as well as upload-file-list.
Opening this PR so that I can get Claude and the team to point out all the flaws and errors :)
The changes should be picked individually where possible.
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.--vaadin-message-max-widthproperty instead. Should consider which one makes more sense to devs.footerslot: sticky part where you can place non-message content. Shares the same max-width as the messages. The idea is that you can put the Message Input component here, so that it’s within the same scrolling container. I’m not 100% sure about this, though, and perhaps this should be left for application code, as we would need to add a resize observer that measures the footer content height and adjusts the list scroll-margin so that messages are scrolled into view when navigating with the keyboard. Both Claude and ChatGPT have this behavior, that the scrolling region covers the input area as well.typingslot 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).userTypingJS 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.typingMessageTextJS property: a string which is appended after the list of users' names who are currently typing a message.typingMessageThemeJS property: a string that sets thethemeattribute on the message in thetypingslot (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.typing-ellipsistheme variant: hides the text from the message in thetypingslot, and shows an animated ellipsis instead.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).