Text Command Popover Fixes/UI Improvements #7728

twschiller · 2024-02-25T13:59:53Z

What does this PR do?

Text Command Fixes/Improvements:
- Switches command key to \ to not conflict with common key on Zendesk, Notion, etc.
- Layout: changes location to be above the command. That placement is more robust because you don't have to worry about font size/line height or moving the popover as the user types
- Enables flip/shift position middleware to accommodate iframed edit areas (e.g., CKEditor, TinyMCE)
- Fix placement bug when triggering the menu at the beginning of a line in a content editable
- Fix bug were Tab submission wasn't working for editors that use Tab for indent (e.g., CKEditor)
- Disable popover for readonly/disabled fields
Text Selection Popover Fixes/Improvements
- Debounce selection handler to avoid lag on large selections
- Hide on selectionstart
New Editor support
- CKEditor 4/5
Refactoring
- Refactors shared code between text selection popover and text command popover
- Introduces a insertAtCursorWithCustomEditorSupport method called from InsertAtCursorEffect and command popover

Remaining Work

Fix text insertion position on content editable. Sometimes surrounding characters/newlines get deleted
Fix text insertion in CKEditor 5 (used by Zendesk). Also tested on CKEditor 4 on playground
Fix placement of popover when textarea is scrolled
Fix text insertion for Salesforce's editor(s): TinyMCE 6 uses a srcdoc iframe, so to register snippets in that frame, the trigger needs to use <all_urls> instead of *://*. We might consider changing our "All URLs" shortcut to use <all_urls>
Fix CSS for empty state, running state, and error state

Discussion

We'll eventually need to land on consistent naming for tooltip vs. popover in the code. Most likely anything working with the DOM should us "popover" because it will eventually use the web popover API

Demo

https://www.loom.com/share/7c8b7168558c4f93909ad9cd47730a2e

Future Work

Support configuring the text command
Update insertAtCursorWithCustomEditorSupport as needed to work with other custom editors
Text Selection popover doesn't run action in Gmail email composer #7729

Checklist

Add tests: N/A: tests will need to be in Playwright due to JSDOM limitations for selections and document.execCommand
New files added to src/tsconfig.strictNullChecks.json (if possible)
Designate a primary reviewer: @fregante

codecov · 2024-02-25T14:06:19Z

Codecov Report

Attention: Patch coverage is 18.06167% with 186 lines in your changes are missing coverage. Please review.

Project coverage is 72.14%. Comparing base (c1745c9) to head (32a39a6).
Report is 3 commits behind head on main.

Files	Patch %	Lines
...contentScript/commandPopover/commandController.tsx	8.10%	68 Missing ⚠️
src/contentScript/commandPopover/commandUtils.ts	11.11%	32 Missing ⚠️
...ntentScript/selectionTooltip/tooltipController.tsx	13.63%	19 Missing ⚠️
...rc/contentScript/commandPopover/CommandPopover.tsx	28.57%	15 Missing ⚠️
src/contrib/ckeditor/ckeditorProtocol.ts	43.47%	13 Missing ⚠️
...c/contentScript/commandPopover/useKeyboardQuery.ts	0.00%	11 Missing ⚠️
src/contentScript/textEditorDom.ts	33.33%	10 Missing ⚠️
src/contentScript/tooltipDom.ts	12.50%	7 Missing ⚠️
src/pageScript/pageScript.ts	0.00%	5 Missing ⚠️
src/bricks/effects/InsertAtCursorEffect.ts	25.00%	3 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7728      +/-   ##
==========================================
- Coverage   72.21%   72.14%   -0.08%     
==========================================
  Files        1264     1267       +3     
  Lines       39628    39688      +60     
  Branches     7343     7355      +12     
==========================================
+ Hits        28617    28631      +14     
- Misses      11011    11057      +46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

twschiller · 2024-02-25T14:07:52Z

src/contentScript/selectionTooltip/tooltipController.tsx

@@ -294,15 +277,6 @@ export const initSelectionTooltip = once(() => {
    { passive: true },
  );

-  // Try to avoid sticky tool-tip on SPA navigation


Moved to createTooltip so the handler is removed when not showing

twschiller · 2024-02-25T16:59:04Z

src/utils/editorUtils.ts

@@ -34,71 +32,3 @@ export function getElementText(element: TextEditorElement): string {

  return $(element).text();
 }
-


Moved to contentScript/commandPopover folder because it needs to call the pageScript

twschiller · 2024-02-25T18:31:16Z

src/contentScript/textEditorDom.ts

+ * @param element the element to insert text into. Can be a text input, textarea, or contenteditable element.
+ * @param text the text to insert
+ */
+export async function insertAtCursorWithCustomEditorSupport({


@fregante I'm assuming you might not upstream editor-specific affordances to textFieldEdit because they require a pageScript?

My idea is that it's up to the user of the library to know and work around these limitations. This means that such a library would already expect to be run in the right context, leaving messaging out of it.

As for text-field-edit, I don’t think I'll upstream it because the advanced methods (wrap, replace) might be pretty complex for each custom editor. textFieldEdit.replace() already doesn't support the basic content editable.

I could work on a dedicated get/set/delete/insert library and API for advanced editors, but it looks like you got this going in the extension already. Let me know if you'd like me to work on anything specific

As for text-field-edit, I don’t think I'll upstream it because the advanced methods (wrap, replace) might be pretty complex for each custom editor. textFieldEdit.replace() already doesn't support the basic content editable.

👍 Makes sense -- that library has a different surface area/goals

Let me know if you'd like me to work on anything specific

Will do as we come across gaps/quirks

twschiller · 2024-02-25T18:31:44Z

src/contentScript/tooltipDom.ts

+ */
+// Currently using the tooltip terminology to match the filename, but will likely switch to popover in the future
+// to match the web popover API terminology.
+export function tooltipFactory(): HTMLElement {


Consolidated command popover and selection popover's properties

fregante · 2024-02-26T02:20:12Z

src/contentScript/tooltipDom.ts

+  popover.style.setProperty("border-radius", "5px");
+  // Can't use colors file because the element is being rendered directly on the host
+  popover.style.setProperty("background-color", "#ffffff"); // $S0 color
+  popover.style.setProperty("border", "2px solid #a8a1b4"); // $N200 color


I've been using this pattern to set the style all at once:

Object.assign(popover.style, { border: "2px solid #a8a1b4", etc })

fregante · 2024-02-26T02:23:40Z

src/contentScript/textEditorDom.ts

+
+  // Ensure the element has focus, so that text is inserted at the cursor position
+  if (document.activeElement !== element) {
+    element.focus();


This is already taken care of by text-field-edit, so it's not necessary for that reason.

However it will also reset the focus to wherever it was before the insertText call.

I'd just lump the two focus calls together and keep your first comment:

// Let the user continue writing etc window.focus() field.focus()

fregante · 2024-02-26T02:25:21Z

src/contentScript/textEditorDom.ts

+  );
+
+  if (isSelectableTextControlElement(element)) {
+    textFieldEdit.insert(element, text);


I don’t think we need a dedicated call now that text-field-edit supports content editable fields. This is also missing the window.focus call seen below

fregante · 2024-02-26T02:32:48Z

src/contentScript/textEditorDom.ts

+ * @param element the element to insert text into. Can be a text input, textarea, or contenteditable element.
+ * @param text the text to insert
+ */
+export async function insertAtCursorWithCustomEditorSupport({


My idea is that it's up to the user of the library to know and work around these limitations. This means that such a library would already expect to be run in the right context, leaving messaging out of it.

As for text-field-edit, I don’t think I'll upstream it because the advanced methods (wrap, replace) might be pretty complex for each custom editor. textFieldEdit.replace() already doesn't support the basic content editable.

I could work on a dedicated get/set/delete/insert library and API for advanced editors, but it looks like you got this going in the extension already. Let me know if you'd like me to work on anything specific

fregante · 2024-02-26T02:36:09Z

src/contentScript/textEditorDom.ts

+import { isSelectableTextControlElement } from "@/types/inputTypes";
+
+/**
+ * @file file for contentScript-specific text editor DOM utilities that require Javascript API calls to editors.


I think you mean pageScript. The content script doesn't have access to CKEditor's properties. This file should probably be moved to the pageScript folder or out of contentScript anyway

I'll clarify tomorrow - this file is actually OK in the content script. It checks for CKEditor using the class name and then calls the pageScript via messenger

The @/contrib/ckeditor module could split up into pageScript-specific vs. safe methods to clarify the behavior

As you mention here: https://github.com/pixiebrix/pixiebrix-extension/pull/7728/files#r1501980861, we might instead opt to move insertAtCursorWithCustomEditorSupport to the pageScript vs. doing some logic in the contentScript and some in the pageScript.

Historically, I've preferred using contentScript where possible due to reduced likelihood of the host page JS somehow interfering. Also, injecting the pageScript has a slight overhead when it needs to be injected

fregante · 2024-02-26T02:38:28Z

src/contentScript/selectionTooltip/tooltipController.tsx

+  if (selectionTooltip) {
+    // Cleanly unmount React component to ensure any listeners are cleaned up.
+    // https://react.dev/reference/react-dom/unmountComponentAtNode
+    unmountComponentAtNode(selectionTooltip);


🙌

We should use this more. I added a few of these calls when working on #4258, but I wasn't consistent.

fregante · 2024-02-26T02:41:03Z

src/contentScript/commandPopover/useKeyboardQuery.ts

+      });
+      document.removeEventListener("keydown", handleKeyDown, {
+        capture: true,
+      });


I might prefer creating a signal and then just return () => controller.abort() to cancel all at once without gotchas/comments/forgotten removals.

fregante · 2024-02-26T02:48:48Z

src/contentScript/commandPopover/commandUtils.ts

+
+  element.focus();
+
+  if (isSelectableTextControlElement(element)) {


I think this excludes type=number fields, but then the function goes on to assume we're working on a content editable. There should probably be a "is content editable" check below

fregante · 2024-02-26T02:53:09Z

src/contentScript/commandPopover/commandController.tsx

          ? getCaretCoordinates(textControl, selectionStart)
          : {
              top: 0,
              left: 0,
+              height: Number.parseInt(
+                window.getComputedStyle(targetElement).lineHeight,


I think this returns "normal" by default. I remember calculating line-height being an issue for the longest time.

fregante · 2024-02-26T02:55:04Z

src/contentScript/commandPopover/commandController.tsx

-  void updatePosition();
+  // For now just destroy the tooltip on document/element scroll to avoid gotchas with floating UI's `position: fixed`
+  // strategy. See tooltipController.ts for more details.
+  document.activeElement?.addEventListener(


Can all these listeners be wrapped in a single call? I saw the same piece of code copy-pasted below I think

twschiller · 2024-02-26T13:31:17Z

src/contentScript/commandPopover/useKeyboardQuery.ts

  element,
  onSubmit,
  onOffset,
 }: {
  commandKey?: string;
  element: TextEditorElement;
-  onSubmit: () => void;
+  onSubmit: (query: string) => void;


Passing the query in the callback so the handler can replace only the \query

src/contentScript/commandPopover/CommandPopover.tsx

github-actions · 2024-02-26T14:54:44Z

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

twschiller added 4 commits February 24, 2024 19:08

Consolidate tooltip factory; event listener lifetime

9c57f44

Fix tab handling on Zendesk

1c842fb

Merge remote-tracking branch 'origin/main' into feature/text-command-ui

5d1459e

Disable for aria-disabled

2ad98e3

twschiller commented Feb 25, 2024

View reviewed changes

twschiller self-assigned this Feb 25, 2024

twschiller added bug Something isn't working user experience Improve the user experience (UX) labels Feb 25, 2024

twschiller added 4 commits February 25, 2024 10:30

Only replace the active query; unmount popover

d88ebfc

Fix duplicate insert

ec1151c

Switch to text-field-edit library

304a237

Add support for CKEditor 5

2d51623

twschiller commented Feb 25, 2024

View reviewed changes

vercel bot deployed to Preview February 25, 2024 16:59 View deployment

Fix textarea placement when scrolled

abcbe6b

vercel bot deployed to Preview February 25, 2024 17:43 View deployment

Selectors go fast

5a8c18b

vercel bot deployed to Preview February 25, 2024 18:01 View deployment

Fix CSS styling

7e9da9b

twschiller requested a review from fregante February 25, 2024 18:25

twschiller marked this pull request as ready for review February 25, 2024 18:26

twschiller requested a review from grahamlangford February 25, 2024 18:26

vercel bot deployed to Preview February 25, 2024 18:27 View deployment

twschiller commented Feb 25, 2024

View reviewed changes

twschiller added this to the 1.8.10 milestone Feb 25, 2024

twschiller assigned fregante Feb 25, 2024

Hide text selection popover on backspace

9acfbee

vercel bot deployed to Preview February 25, 2024 19:49 View deployment

twschiller mentioned this pull request Feb 25, 2024

Text Selection popover doesn't run action in Gmail email composer #7729

Closed

fregante reviewed Feb 26, 2024

View reviewed changes

Address PR comments

f861a1d

vercel bot deployed to Preview February 26, 2024 13:27 View deployment

twschiller commented Feb 26, 2024

View reviewed changes

grahamlangford reviewed Feb 26, 2024

View reviewed changes

src/contentScript/commandPopover/CommandPopover.tsx Outdated Show resolved Hide resolved

Extract status bar

1325fea

vercel bot deployed to Preview February 26, 2024 14:16 View deployment

Pass handler directly

32a39a6

vercel bot deployed to Preview February 26, 2024 14:18 View deployment

grahamlangford approved these changes Feb 26, 2024

View reviewed changes

twschiller merged commit 04bae0d into main Feb 26, 2024
17 checks passed

twschiller deleted the feature/text-command-ui branch February 26, 2024 15:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Text Command Popover Fixes/UI Improvements #7728

Text Command Popover Fixes/UI Improvements #7728

twschiller commented Feb 25, 2024 •

edited

Loading

codecov bot commented Feb 25, 2024 •

edited

Loading

twschiller Feb 25, 2024

twschiller Feb 25, 2024

twschiller Feb 25, 2024

fregante Feb 26, 2024

twschiller Feb 26, 2024 •

edited

Loading

twschiller Feb 25, 2024

fregante Feb 26, 2024

fregante Feb 26, 2024

fregante Feb 26, 2024

fregante Feb 26, 2024

fregante Feb 26, 2024

twschiller Feb 26, 2024 •

edited

Loading

fregante Feb 26, 2024

fregante Feb 26, 2024

fregante Feb 26, 2024 •

edited

Loading

fregante Feb 26, 2024

fregante Feb 26, 2024

twschiller Feb 26, 2024

github-actions bot commented Feb 26, 2024

		@@ -34,71 +32,3 @@ export function getElementText(element: TextEditorElement): string {

		return $(element).text();
		}


		element.focus();

		if (isSelectableTextControlElement(element)) {

Text Command Popover Fixes/UI Improvements #7728

Text Command Popover Fixes/UI Improvements #7728

Conversation

twschiller commented Feb 25, 2024 • edited Loading

What does this PR do?

Remaining Work

Discussion

Demo

Future Work

Checklist

codecov bot commented Feb 25, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2024

twschiller commented Feb 25, 2024 •

edited

Loading

codecov bot commented Feb 25, 2024 •

edited

Loading

twschiller Feb 26, 2024 •

edited

Loading

twschiller Feb 26, 2024 •

edited

Loading

fregante Feb 26, 2024 •

edited

Loading