FEATURE: rich editor link ui for editing it #32583

renato · 2025-05-05T21:24:10Z

Displays a floating toolbar when the selection position is in a link, on desktop. On mobile, replaces the existing toolbar with the link toolbar temporarily.

Desktop

Kapture.2025-04-26.at.15.49.04.1.mp4

Mobile

jjaffeux · 2025-05-13T06:10:05Z

We should remove the rounded corners here:

jjaffeux · 2025-05-13T06:11:00Z

When Im in this state:

Pressing tab should bring me to the first item, atm I need multiple tabs.

jjaffeux · 2025-05-13T06:11:23Z

The last button has no hover state on desktop.

jjaffeux · 2025-05-13T06:12:51Z

Can we avoid destroying/re-creating when my click ends up not changing the caret position?

Kapture.2025-05-13.at.08.12.22.mp4

jjaffeux · 2025-05-13T06:15:31Z

@chapoi It's not due to this work, but it's very annoying that showing a modal is removing the browsers scrollbars, it makes the page all jumpy, I know there are techniques to avoid this, could designers try to give it a go please?

Kapture.2025-05-13.at.08.14.01.mp4

jjaffeux · 2025-05-13T06:16:35Z

When I had focus through tab on one of the items menu and I press escape, I don't get back my caret position in the composer.

jjaffeux · 2025-05-13T06:19:17Z

Im not sure we should have cursor: pointer on the link given we now treat is as text. It's debatable, given this is still an action. But my reasoning is that seeing the cursor makes me think it's going to open the link right away. What do you think @chapoi ?

jjaffeux · 2025-05-13T06:22:28Z

When I edit a link by opening the modal, when I close it I don't get back my caret position in the composer.

jjaffeux · 2025-05-13T06:24:11Z

We have no spacing between the back button on edit mode:

While in normal mode we have a lot of spacing:

Doesnt feel right @chapoi

jjaffeux · 2025-05-13T06:30:41Z

What is this button, didnt get it in my tests?

chapoi · 2025-05-13T08:22:24Z

Im not sure we should have cursor: pointer on the link given we now treat is as text. It's debatable, given this is still an action. But my reasoning is that seeing the cursor makes me think it's going to open the link right away. What do you think @chapoi ?

Tricky one… I'd say keep the pointer, indicating something can be done feels most important. We could consider using context-menu maybe?

While in normal mode we have a lot of spacing:

Yes, I know, but that left side spacing is very hard/impossible to get right due to the use of grid. I'll see if I can tweak it a bit.

What is this button, didnt get it in my tests?

Shouldn't that be "unlink"?

jjaffeux · 2025-05-13T08:53:58Z

Im not sure we should have cursor: pointer on the link given we now treat is as text. It's debatable, given this is still an action. But my reasoning is that seeing the cursor makes me think it's going to open the link right away. What do you think @chapoi ?

Tricky one… I'd say keep the pointer, indicating something can be done feels most important. We could consider using context-menu maybe?

FWIW slack is doing the regular text cursor, and some styling on hover of the link.

jjaffeux · 2025-05-13T08:54:09Z

What is this button, didnt get it in my tests?

Shouldn't that be "unlink"?

Yes it should, but never got it showing

renato · 2025-05-13T09:35:36Z

Yes it should, but never got it showing

@jjaffeux I’ll respond to the other things today, but getting this out first: Unlink is only available for non-autolinks, aren’t they working with [regular](links)?

I get it may be confusing, but I think supporting removing this autolink will still be confusing, with the caveat of being more work. Making a URL not autolink requires escaping it in the markdown, and I didn't think of a good way to make the escaped text linkable again/autolink on the rich editor - I think it's just simpler to omit the option in this case.

jjaffeux · 2025-05-13T10:31:59Z

Yes it should, but never got it showing

@jjaffeux I’ll respond to the other things today, but getting this out first: Unlink is only available for non-autolinks, aren’t they working with [regular](links)?

I get it may be confusing, but I think supporting removing this autolink will still be confusing, with the caveat of being more work. Making a URL not autolink requires escaping it in the markdown, and I didn't think of a good way to make the escaped text linkable again/autolink on the rich editor - I think it's just simpler to omit the option in this case.

OK I see, we can for sure start like this and see the feedback we get 👍

renato · 2025-05-19T17:49:50Z

Can we avoid destroying/re-creating when my click ends up not changing the caret position?

@jjaffeux I don't think we have an easy alternative in this case, what's destroying the menu is the closeOnClickOutside.

EDIT: We can skip the menu's default closeOnClickOutside, use our own, and check the caret position before deciding to close it. Doesn't seem that much work, I can test it out.

Starts defining a more generic API, so a different toolbar instance can be used as a replacement on the main toolbar as well as a foundation for rendering the same toolbar as a floating element. This toolbar reuse started here for the link toolbar: #32583, then got extracted to this PR. --------- Co-authored-by: Sérgio Saquetim <1108771+megothss@users.noreply.github.com>

# Conflicts: # app/assets/javascripts/discourse/app/components/d-editor.gjs # app/assets/javascripts/discourse/tests/integration/components/d-editor-test.gjs # app/assets/stylesheets/common/d-editor.scss

Starts defining a more generic API, so a different toolbar instance can be used as a replacement on the main toolbar as well as a foundation for rendering the same toolbar as a floating element. This toolbar reuse started here for the link toolbar: #32583, then got extracted to this PR. --------- Co-authored-by: Sérgio Saquetim <1108771+megothss@users.noreply.github.com>

…iting modal behavior, floating positioning, etc

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

jjaffeux · 2025-06-04T21:05:26Z

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

+          if (event.key !== "Tab" || event.shiftKey) {
+            return false;
+          }
+
+          const range = utils.getMarkRange(
+            view.state.selection.$head,
+            view.state.schema.marks.link
+          );
+          if (!range) {
+            return false;
+          }
+
+          const activeMenu = document.querySelector(
+            '[data-identifier="composer-link-toolbar"]'
+          );
+          if (!activeMenu) {
+            return false;
+          }
+
+          event.preventDefault();
+
+          const focusable = activeMenu.querySelector(
+            'button, a, [tabindex]:not([tabindex="-1"]), .select-kit'
+          );
+
+          if (focusable) {
+            focusable.focus();
+            return true;
+          }
+
+          return false;


I don't have a better solution, but this part is unfortunate. Having to querySelector an element, always feels bad

jjaffeux · 2025-06-04T21:06:45Z

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

+                  updatePosition(
+                    menuInstance.trigger,
+                    menuInstance.content,
+                    {}
+                  );


I wonder if we shouldn't haven't this function on the menu instance itself, so we don't have to import float-kit internals

jjaffeux

Im pre-approving even if there are few thing to fix/improve, it's already working very well and in a good state.

martin-brennan

This is working super nice on the sandbox 👌

Minor thing I noticed on sandbox, maybe the "Insert hyperlink" toolbar item should change to "Edit hyperlink" if your cursor is inside one?

Also as Lindsey already noted, having the Ctrl+K shortcut back for insert/edit hyperlink would be 👌

martin-brennan · 2025-06-05T06:12:58Z

app/assets/javascripts/discourse/app/components/composer/toolbar-buttons.gjs

              target="_blank"
              rel="noopener noreferrer"
              class={{concatClass "btn no-text btn-icon" button.className}}
              title={{button.title}}
              tabindex={{this.tabIndex button}}
+              {{on "keydown" (or @rovingButtonBar @data.rovingButtonBar)}}


Maybe we could extract this to a getter?

get rovingButtonBar() { return this.args.rovingButtonBar || this.args.data.rovingButtonBar; }

martin-brennan · 2025-06-05T06:22:18Z

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

+                  view.focus();
+                  return false;
+                }
+                return rovingButtonBar(event);


Should we pass a containerClass here to be more specific, or would it be unnecessary?

martin-brennan · 2025-06-05T06:27:15Z

app/assets/javascripts/discourse/app/components/modal/insert-hyperlink.gjs

@@ -18,7 +18,7 @@ import { i18n } from "discourse-i18n";

 export default class InsertHyperlink extends Component {


If this is only used in core, I would advise renaming this to UpsertHyperlink or something for clarity to indicate it can be used for both insert + edit. If it's too much trouble + plugins are using it probably not necessary for now.

martin-brennan · 2025-06-05T06:33:04Z

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

+        let linkState;
+
+        return {
+          update(view) {


This function is massive...on the one hand it's kind of good because it's sequential but on the other hand it can be hard to figure out what is going on at various stages because of the large conditionals and the definition of the handlers in the middle. Two things I would personally split out:

A separate function for what we are doing on desktop (showing dmenu) vs mobile (replacing the toolbar)

Potentially moving handlers out of the stream of text, it would also make it a little easier to read because they are just passed as an argument to LinkToolbar, and you need to kind of skip over them at first to get to the meat and potatoes of what update is doing

martin-brennan · 2025-06-05T06:35:21Z

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js

+              Object.assign(linkState, attrs);
+            }
+
+            if (getContext().capabilities.viewport.sm) {


If I am reading this right, it's saying anything > small viewport (so anything not mobile) will do the DMenu stuff? I find this API a little tricky, because you can't reverse it without being verbose. For example the false condition which is a lot smaller means we are on mobile, but you can't do if (!getContext().capabilities.viewport.sm) { // replace toolbar } because that will apply to XL and so on.

Would be cool if we had a viewport.lessThanSm (with better naming) that uses max-width media queries for these scenarios.

martin-brennan · 2025-06-05T06:37:00Z

app/assets/javascripts/float-kit/addon/lib/d-menu-instance.js

@@ -37,10 +38,13 @@ export default class DMenuInstance extends FloatKitInstance {

    setOwner(this, owner);
    this.options = { ...MENU.options, ...options };
+    this.portalOutletOverride = options.portalOutletElement;


Minor, but IMO this should be portalOutletOverrideElement

martin-brennan · 2025-06-05T06:40:36Z

config/locales/client.en.yml

@@ -2795,6 +2795,9 @@ en:
      link_title: "Hyperlink"
      link_description: "enter link description here"
      link_dialog_title: "Insert Hyperlink"


Could take the opportunity here to change these to Insert hyperlink and Edit hyperlink to follow https://meta.discourse.org/t/formatting-text-in-discourse-documentation-and-uis/324637

martin-brennan · 2025-06-05T06:42:57Z

spec/system/composer/prosemirror_editor_spec.rb

+      expect(find(".d-modal__body input.link-url").value).to eq("https://example.com")
+
+      find(".d-modal__body input.link-text").set("Updated Example")
+      find(".d-modal__body input.link-url").set("https://updated-example.com")


I think for these fill_in(with: "blah") is generally better and what we use more

This will apply to all the other specs here too

martin-brennan · 2025-06-05T06:43:42Z

spec/system/composer/prosemirror_editor_spec.rb

+
+      find(".d-modal__body input.link-text").set("Updated Example")
+      find(".d-modal__body input.link-url").set("https://updated-example.com")
+      find(".d-modal__footer .btn-primary").click


If you use the PageObjects::Modals::Base page object you can use click_primary_button

This will apply to all the other specs here too

martin-brennan · 2025-06-05T06:43:56Z

spec/system/composer/prosemirror_editor_spec.rb

+      expect(find(".d-modal__body input.link-text").value).to eq("Example")
+      expect(find(".d-modal__body input.link-url").value).to eq("https://example.com")
+
+      find(".d-modal__body input.link-text").set("Updated Example")


If you use the PageObjects::Modals::Base page object you can do modal.body.find("input.link-text")

This will apply to all the other specs here too

martin-brennan · 2025-06-05T07:44:44Z

@renato another one from the call today with Sam and Joffrey, if you are hovering on text and choose "Insert hyperlink" button, we don't prefill the text in the modal with the text under the cursor

Also on the open external link button, we should change the tooltip to "Open link in external tab" or something rather than "Visit link"

renato · 2025-06-05T13:00:08Z

@martin-brennan

another one from the call today with Sam and Joffrey, if you are hovering on text and choose "Insert hyperlink" button, we don't prefill the text in the modal with the text under the cursor

I responded on dev, I don't agree with this – and it's kind of a scope creep, let's discuss and address in a follow-up PR if needed. Having a single word under the cursor but NOT selected be auto-added while opening the Insert hyperlink modal seems like a stretch to me.

renato force-pushed the rich_editor_link_ui branch 2 times, most recently from bc77e39 to 3c033a8 Compare May 5, 2025 22:43

FEATURE: rich editor link ui for editing it

7a4687d

renato force-pushed the rich_editor_link_ui branch from 3c033a8 to 7a4687d Compare May 12, 2025 17:22

DEV: tests + tweaks

3a6af69

github-actions bot added the i18n PRs which update English locale files or i18n related code label May 12, 2025

renato marked this pull request as ready for review May 12, 2025 22:33

DEV: remove redundant boolean

3c71c5c

Merge branch 'main' into rich_editor_link_ui

ce68987

renato added 3 commits May 19, 2025 14:51

Merge branch 'main' into rich_editor_link_ui

37b1c9e

Merge branch 'main' into rich_editor_link_ui

92a4b43

Merge branch 'refs/heads/main' into rich_editor_link_ui

a097b87

renato mentioned this pull request May 28, 2025

DEV: extract composer toolbar class for future reuse #32897

Merged

Merge branch 'main' into rich_editor_link_ui

8eba0e2

# Conflicts: # app/assets/javascripts/discourse/app/components/d-editor.gjs # app/assets/javascripts/discourse/tests/integration/components/d-editor-test.gjs # app/assets/stylesheets/common/d-editor.scss

renato added 4 commits June 4, 2025 17:16

DEV: refactor toolbar, using ToolbarBase; improves focus handling, ed…

1fdd38e

…iting modal behavior, floating positioning, etc

Merge branch 'main' into rich_editor_link_ui

537e3ef

DEV: lint

90c4ff6

DEV: fix test

615284f

jjaffeux reviewed Jun 4, 2025

View reviewed changes

app/assets/javascripts/discourse/app/static/prosemirror/extensions/link-toolbar.js Outdated Show resolved Hide resolved

jjaffeux reviewed Jun 4, 2025

View reviewed changes

jjaffeux approved these changes Jun 4, 2025

View reviewed changes

renato added 4 commits June 4, 2025 18:29

DEV: fix tests

f7a37de

DEV: fix tests

3bfa94b

DEV: fix tests? + remove left transition

abb7ff8

DEV: fix tests?

9d28c8a

martin-brennan reviewed Jun 5, 2025

View reviewed changes

		@@ -18,7 +18,7 @@ import { i18n } from "discourse-i18n";

		export default class InsertHyperlink extends Component {

FEATURE: rich editor link ui for editing it #32583

Are you sure you want to change the base?

FEATURE: rich editor link ui for editing it #32583

Uh oh!

Conversation

renato commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

chapoi commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

renato commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjaffeux commented May 13, 2025

Uh oh!

renato commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjaffeux left a comment

Choose a reason for hiding this comment

Uh oh!

martin-brennan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renato commented May 5, 2025 •

edited

Loading

chapoi commented May 13, 2025 •

edited

Loading

renato commented May 13, 2025 •

edited

Loading

renato commented May 19, 2025 •

edited

Loading

renato commented Jun 5, 2025 •

edited

Loading