Skip to content

Close RTE dropdowns on mousedown instead of click#5926

Open
akolson wants to merge 3 commits into
learningequality:hotfixesfrom
akolson:auto-close-rte-menus-on-click
Open

Close RTE dropdowns on mousedown instead of click#5926
akolson wants to merge 3 commits into
learningequality:hotfixesfrom
akolson:auto-close-rte-menus-on-click

Conversation

@akolson
Copy link
Copy Markdown
Member

@akolson akolson commented May 20, 2026

Summary

  • RTE dropdown menus (font, paste, "More" overflow) now close when clicking anywhere outside them — including in the editor content area, on toolbar buttons, or outside the editor entirely

Root cause: Two separate preventDefault() calls suppress the click event inside the editor, per the HTML spec:

  1. ToolbarButton.vue uses @mousedown.prevent on every toolbar button — this prevents click from firing after any toolbar button press
  2. ProseMirror calls preventDefault() on mouseup when repositioning the cursor — this prevents click from firing when clicking into the editor content area

How it works: Switched the document-level handleClickOutside listener from 'click' to 'mousedown' in useDropdowns.js (font & paste dropdowns) and EditorToolbar.vue (More overflow dropdown). The mousedown event always propagates to the document regardless of any preventDefault() calls, making dropdown closing reliable for all interactions.

Screen.Recording.2026-05-20.at.11.09.34.mov

References

Fixes #5886

Reviewer guidance

  • Open an exercise and click into an answer RTE
  • Open the format dropdown (heading selector) — click anywhere outside it (editor text area, Bold/Italic buttons, padding, outside editor) — dropdown should close immediately
  • Same for the paste dropdown and the "More" overflow dropdown (narrow the window to trigger it)
  • Verify clicking inside an open dropdown does not close it
  • Verify Bold, Italic, and other toolbar buttons still apply formatting normally

AI usage

Implemented with Claude Code. I reviewed the generated diff and confirmed the root cause analysis (ProseMirror mouseup preventDefault + ToolbarButton @mousedown.prevent both suppressing click events).

@akolson akolson changed the title fix(rte): close dropdowns on mousedown instead of click Close RTE dropdowns on mousedown instead of click May 20, 2026
@akolson akolson changed the base branch from unstable to hotfixes May 20, 2026 07:52
@akolson akolson marked this pull request as ready for review May 20, 2026 08:14
@akolson akolson force-pushed the auto-close-rte-menus-on-click branch from 48c8e59 to 1cc7d04 Compare May 20, 2026 08:21
Copy link
Copy Markdown
Contributor

@rtibblesbot rtibblesbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean fix with a thorough root cause analysis.

CI passing. No UI files changed — visual inspection not applicable.

  • praise: see inline


onMounted(() => {
document.addEventListener('click', handleClickOutside);
document.addEventListener('mousedown', handleClickOutside);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The root cause analysis here is excellent — tracing the missing click events to two independent preventDefault() paths (ProseMirror's mouseup and ToolbarButton's @mousedown.prevent) and choosing mousedown as the fix point is well-reasoned. The change is minimal and precise.

@rtibblesbot
Copy link
Copy Markdown
Contributor


@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly

How was this generated?

Reviewed the pull request diff checking for:

  • Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
  • Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
  • Architecture: duplicated concerns, minimal interfaces, composition over inheritance
  • Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
  • Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
  • Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
  • Checked CI status and linked issue acceptance criteria
  • For UI changes: inspected screenshots for layout, visual completeness, and consistency

Copy link
Copy Markdown
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes make sense to me, and manual QA confirms. The only small nitpick is that in the useDropdowns composable, we check all open dropdowns to confirm the click is outside, which leads to the dropdown not being closed if you are opening another dropdown (which should be)

Grabacion.de.pantalla.2026-05-21.a.la.s.10.10.34.a.m.mov

In general, I wonder whether we should use KDropdownMenu here, which already handles many of these things? cc: @marcellamaki @rtibbles. (not that we should do it in this PR to hotfixes, but in a follow-up to unstable, or in #5707 where we'll also need to make some decisions around these dropdowns)

@rtibbles
Copy link
Copy Markdown
Member

Yes - I think that's reasonable, and having it be able to handle the specific styling/layout constraints that would be required for it in the RTE seems like good disciplining for the component!

@akolson akolson force-pushed the auto-close-rte-menus-on-click branch 2 times, most recently from f219add to 248ef75 Compare May 26, 2026 11:27
@akolson
Copy link
Copy Markdown
Member Author

akolson commented May 26, 2026

Thanks @AlexVelezLl for raising this. The nitpick comes from the fact that useDropdowns is called independently in both FormatDropdown and PasteDropdown, giving each component its own isolated state and separate mousedown listener. The previous handleClickOutside implementation used document.querySelectorAll('.dropdown-container') to detect all dropdown containers on the page. As a result, when PasteDropdown's trigger button was clicked while FormatDropdown was open, FormatDropdown's listener interpreted the click as happening “inside a dropdown container” and skipped closing, leaving both dropdowns open simultaneously.

This was fixed by adding a containerRef to the composable and having each component bind its root element to it. handleClickOutside now checks only against its own container, so clicking PasteDropdown's trigger is correctly treated as an “outside” click by FormatDropdown's listener, causing it to close before the other dropdown opens.

@akolson akolson requested a review from AlexVelezLl May 26, 2026 11:38
akolson and others added 2 commits May 26, 2026 14:40
ProseMirror calls preventDefault() on mouseup when repositioning the
cursor, and ToolbarButton uses @mousedown.prevent — both suppress the
click event per the HTML spec. Switching the document listener to
mousedown makes dropdown close reliable for all interactions inside the
editor.

Fixes learningequality#5886

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replaces the global querySelectorAll('.dropdown-container') with a
per-instance containerRef, so clicking one dropdown's trigger correctly
closes the other instead of being treated as an inside click.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@akolson akolson force-pushed the auto-close-rte-menus-on-click branch from 248ef75 to 4b3b2f8 Compare May 26, 2026 11:40
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@akolson akolson force-pushed the auto-close-rte-menus-on-click branch from 0ada02e to f90ab05 Compare May 26, 2026 14:57
Copy link
Copy Markdown
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @akolson! Code changes make sense, and manual QA checks out!

@akolson akolson added this to the Studio: Patch Releases milestone May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exercises - New Rich Text Editor - The opened editor menus don't get closed

4 participants