Floating toolbar shows leading divider when there are no per-type props or only lock #163

New issue

Open

opened 2026-05-06 11:49:08 +00:00 by AhmedHanafy725 · 2 comments

AhmedHanafy725 commented

2026-05-06 11:49:08 +00:00

Member

Two related divider bugs in the floating selection toolbar:

Emoji (and any selection where only the Lock button is shown) — the divider after Lock is rendered even though no per-type controls follow it.
Comments — the floating toolbar for comment markers shows a leading divider before the comment content, splitting an empty space at the start.

The divider visibility logic in selection_toolbar.js (_showPropsGroup and the equivalent for comments) should hide the divider whenever the trailing group is empty. Currently it only checks visibility, not whether propsEl has any children.

Two related divider bugs in the floating selection toolbar: 1. **Emoji (and any selection where only the Lock button is shown)** — the divider after Lock is rendered even though no per-type controls follow it. 2. **Comments** — the floating toolbar for comment markers shows a leading divider before the comment content, splitting an empty space at the start. The divider visibility logic in `selection_toolbar.js` (`_showPropsGroup` and the equivalent for comments) should hide the divider whenever the trailing group is empty. Currently it only checks visibility, not whether `propsEl` has any children.

AhmedHanafy725 commented

2026-05-06 12:03:31 +00:00

Author

Member

Spec

Root cause

DOM: [Lock] [divider] [propsEl]. The divider's visibility is coupled to propsEl via _showPropsGroup(visible) (lines 107–110), which toggles both together. Two symptoms:

Emoji: _renderForNode switch has no case 'emoji' — falls through default, leaves propsEl empty. update() calls _showPropsGroup(true) regardless. Result: stray divider + empty props. (Same applies to any renderer that early-returns or adds nothing — drawing, calendar without state, kanban, mindmap, group.)
Comments: Lock is hidden (line 213 _showLockGroup(!allComments)), but _renderComment populates propsEl and update() calls _showPropsGroup(true). Result: leading divider with no Lock before it.

Both bugs are one design defect: divider visibility coupled to props only, not to the joint condition Lock visible AND props non-empty.

Files to Modify

crates/hero_whiteboard_ui/static/web/js/whiteboard/selection_toolbar.js — only file.

Implementation Plan

Step 1 — decouple the divider from `propsEl`

_showPropsGroup(visible) toggles ONLY propsEl.style.display. Drop the divider toggle.

New helper _updateDivider():

function _updateDivider() {
    if (!dividerEl) return;
    var lockShown = lockBtn && lockBtn.style.display !== 'none';
    var propsShown = propsEl && propsEl.style.display !== 'none';
    var hasContent = propsEl && propsEl.children.length > 0;
    dividerEl.style.display = (lockShown && propsShown && hasContent) ? '' : 'none';
}

Call _updateDivider() at every site that currently mutates Lock-or-props visibility:
- End of update() after the props branch resolves (both single-unlocked and multi-/locked paths).
- End of showConnector() after _renderConnector(id) + _showPropsGroup(true).
- End of refresh() after re-rendering cachedNode.
- Inside hide() after _showPropsGroup(false).

Resulting behavior:

Emoji / empty propsEl: Lock visible, propsEl visible but children.length === 0 → divider hidden.
Comment: Lock hidden, propsEl populated → divider hidden.
Connector: Lock hidden, propsEl populated → divider hidden (already correct).
All other types (sticky/text/shape/frame/calendar/kanban/mindmap/group/webframe/drawing): Lock visible, propsEl populated → divider shown (preserved).
Locked single: Lock visible, propsEl hidden → divider hidden (preserved).
Multi or mixed: Lock visible, propsEl hidden → divider hidden (preserved).

Acceptance Criteria

Emoji selection: only Lock visible. No divider.
Comment selection: only Resolve/Unresolve + Delete visible. No leading divider.
Connector selection: unchanged.
Sticky / Text / Shape / Document / Frame / Calendar / Kanban / Mindmap / Group / Webframe / Drawing single (unlocked): Lock + divider + per-type props.
Locked single: Lock (Unlock state), no divider, no props.
Multi/mixed: only Lock, no divider.
Empty selection: toolbar hidden.
Calendar/Kanban/Mindmap with missing state: no orphan divider.

What NOT to break

Lock toggle handler (lines 145–160).
Popover stack lifecycle and _clearProps()'s call to _closeOpenPopover().
is-visible class toggling via show()/hide().
reposition() depending on show() being called first.
_showLockGroup(false) for comment and connector modes.
setColor() / activeColorSetter lifecycle in _clearProps().
Existing tests asserting toolbar shape for sticky/text/shape.

## Spec ### Root cause DOM: `[Lock] [divider] [propsEl]`. The divider's visibility is coupled to `propsEl` via `_showPropsGroup(visible)` (lines 107–110), which toggles both together. Two symptoms: 1. **Emoji**: `_renderForNode` switch has no `case 'emoji'` — falls through `default`, leaves `propsEl` empty. `update()` calls `_showPropsGroup(true)` regardless. Result: stray divider + empty props. (Same applies to any renderer that early-returns or adds nothing — drawing, calendar without state, kanban, mindmap, group.) 2. **Comments**: Lock is hidden (line 213 `_showLockGroup(!allComments)`), but `_renderComment` populates `propsEl` and `update()` calls `_showPropsGroup(true)`. Result: leading divider with no Lock before it. Both bugs are one design defect: divider visibility coupled to props only, not to the joint condition `Lock visible AND props non-empty`. ### Files to Modify - `crates/hero_whiteboard_ui/static/web/js/whiteboard/selection_toolbar.js` — only file. ### Implementation Plan #### Step 1 — decouple the divider from `propsEl` 1. `_showPropsGroup(visible)` toggles ONLY `propsEl.style.display`. Drop the divider toggle. 2. New helper `_updateDivider()`: ```js function _updateDivider() { if (!dividerEl) return; var lockShown = lockBtn && lockBtn.style.display !== 'none'; var propsShown = propsEl && propsEl.style.display !== 'none'; var hasContent = propsEl && propsEl.children.length > 0; dividerEl.style.display = (lockShown && propsShown && hasContent) ? '' : 'none'; } ``` 3. Call `_updateDivider()` at every site that currently mutates Lock-or-props visibility: - End of `update()` after the props branch resolves (both single-unlocked and multi-/locked paths). - End of `showConnector()` after `_renderConnector(id)` + `_showPropsGroup(true)`. - End of `refresh()` after re-rendering `cachedNode`. - Inside `hide()` after `_showPropsGroup(false)`. Resulting behavior: - **Emoji** / empty `propsEl`: Lock visible, propsEl visible but `children.length === 0` → divider hidden. - **Comment**: Lock hidden, propsEl populated → divider hidden. - **Connector**: Lock hidden, propsEl populated → divider hidden (already correct). - **All other types** (sticky/text/shape/frame/calendar/kanban/mindmap/group/webframe/drawing): Lock visible, propsEl populated → divider shown (preserved). - **Locked single**: Lock visible, propsEl hidden → divider hidden (preserved). - **Multi or mixed**: Lock visible, propsEl hidden → divider hidden (preserved). ### Acceptance Criteria - [ ] Emoji selection: only Lock visible. No divider. - [ ] Comment selection: only Resolve/Unresolve + Delete visible. No leading divider. - [ ] Connector selection: unchanged. - [ ] Sticky / Text / Shape / Document / Frame / Calendar / Kanban / Mindmap / Group / Webframe / Drawing single (unlocked): Lock + divider + per-type props. - [ ] Locked single: Lock (Unlock state), no divider, no props. - [ ] Multi/mixed: only Lock, no divider. - [ ] Empty selection: toolbar hidden. - [ ] Calendar/Kanban/Mindmap with missing state: no orphan divider. ### What NOT to break - Lock toggle handler (lines 145–160). - Popover stack lifecycle and `_clearProps()`'s call to `_closeOpenPopover()`. - `is-visible` class toggling via `show()`/`hide()`. - `reposition()` depending on `show()` being called first. - `_showLockGroup(false)` for comment and connector modes. - `setColor()` / `activeColorSetter` lifecycle in `_clearProps()`. - Existing tests asserting toolbar shape for sticky/text/shape.

AhmedHanafy725 commented

2026-05-06 12:06:16 +00:00

Author

Member

Done

Commit b5c9f32 on development. _showPropsGroup now toggles only propsEl; a new _updateDivider() helper checks Lock-visible AND props-visible AND props-non-empty, and is called from hide(), tail of update(), refresh() (node path), and showConnector().

cargo check --workspace: pass

## Done Commit `b5c9f32` on `development`. _showPropsGroup now toggles only propsEl; a new _updateDivider() helper checks Lock-visible AND props-visible AND props-non-empty, and is called from hide(), tail of update(), refresh() (node path), and showConnector(). - `cargo check --workspace`: pass