Allow modules to alter EditorLinkDialog to specify link text

What exactly were you trying to do? I don't quite understand.

Override it in the EditorImageDialog form's submit handler.

Oh wait, I see you're asking it for #2653246: Override link text using the title attribute. Which means you don't care about how editor module does things, you care about actual CKEditor plugin code.

My answer is valid for LinkIt 5, you won't need to modify the CKEditor plugin anymore.

I'd recommend looking at https://www.drupal.org/project/editor_advanced_link as an example. That's how you can get additional attributes to work.

But I think that is not what you are asking. I think you're asking for help with CKEditor JS code. This is not the right place for that. I believe they provide support on Stack Overflow.

@stBorchert : The patch you submitted in Linkit queue seems to be perfectly valid in core link plugin.

Additionally, it would be a nice addition for the users of editor advanced link.

Attached, your patch, just rerolled.

The reason the LinkIt module doesn't do that anymore is explained in detail elsewhere. In short: it doesn't work when the title of linked content changes, or when a translated version is linked.

I don't see a valid use case for this?

@Wim Leers: As I see it, it is a request for adding "other" text to the element, not the source link or selected value.

Cosider the following where the bold text is the selected text.

"Lorem ipsum dolor sit amet, consectetur adipiscing elit."
Adding a link to /example

"Lorem <a href="/example">ipsum dolor</a> sit amet, consectetur adipiscing elit."

"Lorem ipsum dolor sit (Cursor here) amet, consectetur adipiscing elit." (Nothing selected)
Adding a link to /example

"Lorem ipsum dolor sit <a href="/example">/example</a> amet, consectetur adipiscing elit."

See the second example. Here the user has not selected any text to use for the element text. The URL will then be used by the default link plugin as the element text.

This is what the issue is about. The ability to configure the plugin to use some other texts here.

Ahhh! Now I get it. Thanks, that really helps :)

So then the proposal here is that if there is a value specified for the title attribute, that we use that as the default link text instead of using the URL itself as the default link text?

Basically yes.

Tho I'm not sure if the patch provided in #11 is the correct way to go. There might be a better structure that don't include the "attribute" name as it could be kind of confusing to other devs.

So maybe:
If there is a value specified for the element text, we use that as the default link text instead of using the URL itself as the default link text.

You mean: you'd like to be able to specify link text, independently of the title attribute?

Ok, so that then is acceptable. It's a valid use case, although an edge casey one. We should retain the current behavior by default (changing it would break the expectation 99% of users have), but it's fine for contrib modules to override this in particular cases.

So then this would need to:

1. Send the current link text when making the request to /editor/dialog/image/basic_html, because the dialog needs to be able to show the current data.
   Currently:

   editor_object[href]:http://foobar.com
   

   we'd need to add to that:

   editor_object[text]: CURRENT_TEXT_OR_EMPTY_STRING
   

   I think text is acceptable, since it's not a valid attribute on <img>, so we can repurpose it. Thoughts about that?

2. We then need to modify core/modules/ckeditor/js/plugins/drupallink/plugin.js much like the existing patch does.

Most painfully… this will need tests. JavaScript tests. Of which we have none yet for CKEditor yet. Because we've only had the ability to do JS tests since a few months. Sorry about that :(

Send the current link text when making the request to /editor/dialog/image/basic_html, because the dialog needs to be able to show the current data.

Does this means that there will be an additional input field in the dialog used for "texts"?

How should other modules alter this?

Is it an absolute requirement for js-tests in order to get a possible path committed to 8.2.x?

Does this means that there will be an additional input field in the dialog used for "texts"?

Not in Drupal core. But contrib modules could do that, yes. Of course, a contrib module can choose to not expose an input[type=text] and instead set a value based on some logic in the submit handler.

How should other modules alter this?

See above.

Is it an absolute requirement for js-tests in order to get a possible path committed to 8.2.x?

I'm not sure. It's the early days of JS tests, but I know that several other patches that were RTBC were un-RTBC'ed specifically because we can now write JS tests and hence they should have JS tests.

#22: My best guess is that CKEditor hasn't finished loading yet? Can you let your functional JS test first verify that the expected "DrupalLink" button actually exists? You already verify all that! No idea then :(

However, if we look at the test output, it actually looks like you're hitting a bug in GastonJS:

fail: [Other] Line 25 of core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php:
Drupal\Tests\ckeditor\FunctionalJavascript\DrupalLinkTest::testOverridingLinkText
Zumba\GastonJS\Exception\BrowserError: There was an error inside the PhantomJS portion of GastonJS.
This is probably a bug, so please report it:
click
html.touchevents.details.js body.user-logged-in.path-node div.ui-dialog.ui-widget.ui-widget-content.ui-corner-all.ui-front.editor-link-dialog.ui-dialog--narrow.ui-dialog-buttons div#drupal-modal.ui-front.ui-dialog-content.ui-widget-content div#editor-link-dialog-form form#editor-link-dialog--_bnF8vu081Q.editor-link-dialog

/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:122
/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserBase.php:99
/var/www/html/vendor/jcalderonzumba/gastonjs/src/Browser/BrowserMouseEventTrait.php:17
/var/www/html/vendor/jcalderonzumba/mink-phantomjs-driver/src/MouseTrait.php:31
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:153
/var/www/html/vendor/behat/mink/src/Element/NodeElement.php:161
/var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:115
/var/www/html/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php:62

However, this is also causing a fail in CKEditorTest. Because there's now two text formats/editors being installed, instead of one. You shouldn't modify ckeditor_test to install additional formats/editors. You should instead let your test create the desired formats/editors! Then that problem goes away. :)

Are you at DrupalDevDays Milan? Find dawehner, he will know how to debug that PhantomJS/GastonJS bug.

Forgot to say: awesome work here — thanks! :)

Great — thanks so much!

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,173 @@
   +    // Create text format, associate CKEditor.
   +    $llama_format = FilterFormat::create([
   +              'format' => 'llama',
   +              'name' => 'Llama',
   +              'weight' => 0,
   +              'filters' => [],
   +    ]);
   +    $llama_format->save();
   +    $editor = Editor::create([
   +              'format' => 'llama',
   +              'editor' => 'ckeditor',
   +    ]);
   

   Indent errors.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,173 @@
   +    // Assign widget settings for the 'default' form mode.
   +    EntityFormDisplay::create([
   +              'targetEntityType' => 'node',
   +              'bundle' => 'page',
   +              'mode' => 'default',
   +              'status' => TRUE,
   +            ])
   +            ->setComponent('body', ['type' => 'text_textarea_with_summary'])
   +            ->save();
   

   Indent errors

3. +++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
   @@ -0,0 +1,107 @@
   +    $this->installSchema('node', array('node_access'));
   

   Use arrays are use everywhere eles, should be used here aswell.

I have fixed this issues in this patch.

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +    $this->assertTrue($ckeditor_loaded, 'The editor has not been loaded.');
   

   +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +    $this->assertTrue($ckeditor_loaded, 'The editor has not been loaded.');
   

   The message should be positive.
   The editor has not been loaded.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +    $this->assertEquals('/node/add/page', $link_text, 'Inserted link text does not equals expected text.');
   

   +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +    $this->assertEquals('Add page', $link_text, 'Inserted link text does not equals expected text.');
   

   The message should be positive.
   Inserted link text does not equals expected text.

Fixed some coding standards and the test messages.

Should we open a new issue to add a name to the iframe? Other tests would benefit from that, i.e. DrupalImage and contrib modules like Linkit.

1. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +use Drupal\simpletest\ContentTypeCreationTrait;
   ...
   +  use ContentTypeCreationTrait;
   

   Is it ok to use simpletest classes in these tests?

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,169 @@
   +    $link_button = $page->find('css', 'a.cke_button__drupallink');
   +    $link_button->click();
   ...
   +    $button = $page->find('css', '.editor-link-dialog')->find('css', '.button.form-submit span');
   +    $button->click();
   ...
   +    $link_button = $page->find('css', 'a.cke_button__drupallink');
   +    $link_button->click();
   ...
   +    $button = $page->find('css', '.editor-link-dialog')->find('css', '.button.form-submit span');
   +    $button->click();
   

   Could be single lines?

@stBorchert: Great, thanks!

Lets see what they say about this. If they don't like it, can Drupal add a name attribute itself in any way?

This patch contains fixes mentiond in #33.

Crap, messed up when re-rolled.

There is no need to create a node type in EditorLinkDialogTest.

Also made some minor cleanup.

Retriggered tests.

Wouldn't it be a good idea to always show that text field?
I mean, on most common tools (Word, GMail, Gdocs, etc.) when you want to create a link, the UI shows you the text of the link, prepopulated if something was selected. I think it's more convenient for the end user and probably less scary.

Google Docs add a link while a text is selected

Google Docs add a link while no text is selected

LibreOffice add a link while a text is selected

LibreOffice add a link while no text is selected

GMail add a link while a text is selected

GMail add a link while no text is selected

What your propose based on Gmail and Libre Office seems good to me. However, the maintainer rejected an additional input box like this in #20.

How about we keep this issue confined to adding to the API the ability to specify the text; then in an follow-up issue we can endlessly debate the best default UI for core? That way we unblock contrib/custom more quickly.

Needs reroll against 8.3

@jonathanshaw I think that's a good idea... Let's re-roll this in the meanwhile.

Hi, I don't think this needs a re-roll since it applied cleanly on 8.3.x. Here is an interdiff for the last two patches though.

root@masky-VirtualBox:/home/drupal/cap/drupal# git apply -v allow_modules_to_alter-2741945-42.patch 
Checking patch core/modules/ckeditor/js/plugins/drupallink/plugin.js...
Checking patch core/modules/ckeditor/tests/modules/ckeditor_test.module...
Checking patch core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php...
Checking patch core/modules/editor/src/Form/EditorLinkDialog.php...
Checking patch core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php...
Applied patch core/modules/ckeditor/js/plugins/drupallink/plugin.js cleanly.
Applied patch core/modules/ckeditor/tests/modules/ckeditor_test.module cleanly.
Applied patch core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php cleanly.
Applied patch core/modules/editor/src/Form/EditorLinkDialog.php cleanly.
Applied patch core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php cleanly.

This patch is related to this issue, but reduces the scope of the changes significantly. I just need a way for other modules to override the default text, specifically linkit. If you click in ckeditor without highlighting text, it displays the href as the text of the link. There's no way, short of patching core for other modules to change this.

This is same as #58, but rerolled because #58 wasn't working on Drupal 8.4.2

Was reminded of this via #2640398-21: Allow <drupal-entity> elements to be inline CKEditor Widgets. Completely lost track of this. My focus has been API-First for the past two years. Very sorry.

This looks great.

@oknate: while well-intentioned, those other changes are just test coverage. I think having test coverage for this makes sense. So I'm bringing back @anon's patch from #42, but I do prefer your handling of the text over @anon's, because there is no text attribute.

Rerolling…

This is a straight rebase of #42.

1. +++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
   @@ -113,7 +113,13 @@
   +                // Allow overriding the link text by using the special attribute "text".
   +                if (returnValues.attributes.hasOwnProperty('text') && returnValues.attributes.text.length) {
   +                  linkText = returnValues.attributes.text;
   +                }
   

   This only allows the server to override the text on the client; the client doesn't pass the current value to the server.

2. +++ b/core/modules/ckeditor/tests/modules/ckeditor_test.module
   @@ -13,3 +14,13 @@
   +  $form['attributes']['text']['#type'] = 'textfield';
   
   +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,165 @@
   +    $page->fillField('attributes[text]', 'Add page');
   
   +++ b/core/modules/editor/src/Form/EditorLinkDialog.php
   @@ -49,6 +49,13 @@ public function buildForm(array $form, FormStateInterface $form_state, Editor $e
        ];
   ...
   +    $form['attributes']['text'] = array(
   
   +++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
   @@ -0,0 +1,94 @@
   +    $this->assertEquals('some text', $form_state->getValue(['attributes', 'text'], 'not-this'), 'Attribute "text" exists and has the correct value.');
   

   text is not an attribute, so this should not be ['attributes']['text'], but ['text'].

   #58 did this better IMHO. This keeps the name text, but stops putting it under attributes.

3. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/DrupalLinkTest.php
   @@ -0,0 +1,165 @@
   +    // We can't use $session->switchToIFrame() here, because the iframe does not
   +    // have a name.
   +    $javascript = <<<JS
   +    (function(){
   +      var iframes = document.getElementsByClassName('cke_wysiwyg_frame');
   +      if (iframes.length) {
   +        var doc = iframes[0].contentDocument || iframes[0].contentWindow.document;
   +        var link = doc.getElementsByTagName('a')[0];
   +        return link.innerText.trim() || link.textContent || '';
   +      }
   +    })()
   +JS;
   +    $link_text = $session->evaluateScript($javascript);
   ...
   +    // We can't use $session->switchToIFrame() here, because the iframe does not
   +    // have a name.
   +    $javascript = <<<JS
   +    (function(){
   +      var iframes = document.getElementsByClassName('cke_wysiwyg_frame');
   +      if (iframes.length) {
   +        var doc = iframes[0].contentDocument || iframes[0].contentWindow.document;
   +        var link = doc.getElementsByTagName('a')[0];
   +        return link.innerText.trim() || link.textContent || '';
   +      }
   +    })()
   +JS;
   +    $link_text = $session->evaluateScript($javascript);
   

   This is repeating the exact same complex logic twice. Let's move it to a helper.

4. +++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
   @@ -0,0 +1,94 @@
   +  protected $format;
   ...
   +    $this->format = FilterFormat::create([
   ...
   +    $editor = Editor::create([
   ...
   +      ->addBuildInfo('args', [$this->format]);
   

   In the time that has passed, it's the Editor that became expected, not the FilterFormat.

5. +++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
   @@ -0,0 +1,94 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   ...
   +  /**
   +   * Sets up the test.
   +   */
   

   Nit: inheritdoc.

I messed that up.

+++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
@@ -0,0 +1,91 @@
+class EditorLinkDialogTest extends EntityKernelTestBase {

I believe this can become quite a bit simpler.

+1

Hi there .. I've also come to this issue via (https://www.drupal.org/comment/12335702#comment-12335702).

Feels like all of the solutions for a particular problem I'm facing point towards LinkIt as the best approach.

I'm wondering what the status of this is now? Is there anything I can help out with?

@lukus The failing test in #66 needs addressing.

Patch #66 not applying on Drupal 9.1.x.

Rerolled patch #66

Please review, Fixing the failed test cases

Thank you all for moving this forward!

I did some testing and the patches does not entirely fill the issue, though.

Plus, I've noticed that you directly edited the plugin.js file instead of the plugin.es6.js file which is the current way to contribute JS in Core.

I'll try to provide a new patch today.

Here we go.

I feel like we should extend the test coverage for some of these cases but I have no idea of how to do that for those with selected text or existing link editing.

PS: I moved the text field above the attributes in the Dialog so contrib modules won't have to play with weights to look like most common tools (cf #47) when they will transform the field's type.

+++ b/core/modules/ckeditor/tests/modules/src/FunctionalJavascript/DrupalLinkTest.php
@@ -0,0 +1,162 @@
+   */
...
+    $this->drupalGet('node/add/page');
...
+    $page->fillField('attributes[href]', '/node/add/page');
...
+    $page->fillField('text', 'Add page');
...
+    $this->assertFirstLinkText('Add page');

+++ b/core/modules/editor/tests/src/Kernel/EditorLinkDialogTest.php
@@ -0,0 +1,75 @@
+

I think these tests would be more robust and easier to understand if we were adding a link to a page other than the one we are on e.g. /admin/some/destination "Some link text"

#85

I feel like we should extend the test coverage for some of these cases but I have no idea of how to do that for those with selected text or existing link editing.

In slack @lendude said that Googling suggested that double clicking a word was perhaps the best way to accomplish selecting text in a test. He also provided these helper methods which I paste here in case they're of any use.

/**
 * Fill a CKEditor field in a container.
 *
 * @param string $label
 *   The label of the CKEditor field.
 * @param string $value
 *   The value to set in the CKEditor field.
 * @param \Behat\Mink\Element\TraversableElement $container
 *   (optional) The element to search for the field. Defaults to the current
 *   page.
 * @param bool $set_as_source
 *   Indicate if the passed value should be set as 'source', use this when
 *   adding HTML tags in the value. Defaults to FALSE.
 */
protected function fillCkeditorField($label, $value, TraversableElement $container = NULL, $set_as_source = FALSE) {
  if (!$container) {
    $container = $this->getSession()->getPage();
  }
  // Get the field wrapper for the field. The first parent is.
  $field_wrapper = $this->findParentByClass($container->findField($label), 'field--type-text-long');
  if ($set_as_source) {
    $container->find('css', '.cke_button__source')->click();
    $container->find('css', '.cke_source')->setValue($value);
  }
  else {
    // Switch to CKEditor iframe and set te value.
    $this->switchToUnnamedIframe('#' . $field_wrapper->getAttribute('id') . ' iframe');
    $page = $this->getSession()->getPage();
    $field = $page->find('css', 'body');
    $field->focus();
    $field->setValue($value);
    // Switch back to the parent frame.
    $this->getSession()->getDriver()->switchToIFrame();
  }
}
/**
 * Switches to an unnamed iframe.
 *
 * @param string $iframeSelector
 *   The CSS selector for the iframe.
 *
 * @throws \Behat\Mink\Exception\DriverException
 * @throws \Behat\Mink\Exception\UnsupportedDriverActionException
 */
protected function switchToUnnamedIframe($iframeSelector) {
  static $iframe_count = 0;
  $name = "iframeToSwitchTo" . $iframe_count;
  $function = <<<JS
  (function(){
     let iframe = document.querySelector("$iframeSelector");
     iframe.name = "$name";
  })()
JS;
  try {
    $this->getSession()->executeScript($function);
  }
  catch (\Exception $e) {
    throw new AssertionFailedError("Element '$iframeSelector' was not found. " . $e->getMessage());
  }
  $iframe_count++;
  $this->getSession()->getDriver()->switchToIFrame($name);
}

This applies well for 8.9.x.

I think that most of the time users just customize the link text.
So I changed the $form['text']['#type'] to textfield, no need to use hooks or modules to override the EditorLinkdialog

@potem upload fail for patch 2741945-92.patch. So i help him to re-upload.
This patch just update type 'value' to type 'textfield'.

CKEditor has been removed from core, CKEditor 4 is removed from Drupal Core in 10.0.0

The existing patch was failing with Drupal 9.5 - so have re-rolled for that:

Patch 2741945-98.patch failed to apply with drupal 9.5.1 php 8.1

Hi, I re-rolled the patch and tested with 9.5.0 and 9.5.2.

I needed that on D10 to delay an upgrade so I rerolled and split the patch.

If you need that feature you have to apply:

• 2741945-103-ckeditor.patch on the ckeditor module
• 2741945-103-core-10.4.patch on Drupal Core (tested on 10.4 but might also work on previous 10.x versions)