Show or hide close button based on destination parameter #109

donquixote · 2025-05-28T01:42:05Z

No description provided.

donquixote · 2025-05-28T01:42:48Z

collabora_online.module

@@ -41,7 +41,7 @@ function collabora_online_theme(): array {
        'accessToken' => 'test',
        'accessTokenTtl' => '86400',
        'iFrameStyle' => 'width:95%;',
-        'closebutton' => '',
+        'closeButtonUrl' => '',


The goal here was to keep the changes to template and js to a minimum.
We should rethink this in a follow-up.

donquixote · 2025-05-28T01:44:42Z

collabora_online.module

@@ -108,11 +108,14 @@ function collabora_online_entity_operation(EntityInterface $entity): array {
    return [];
  }

+  $options = [];
+  $options['query'] = \Drupal::destination()->getAsArray();
+


I looked for examples in core how they populate the 'destination' parameter.
I found only one case where they explicitly use the current url, in other cases they use that destination service.

The main difference is this (I think): If the current page already has a destination parameter, it will be kept for the link.

donquixote · 2025-05-28T01:45:26Z

js/cool.js

@@ -25,7 +25,7 @@ function postReady() {
  });
 }

-function receiveMessage(hasCloseButton, event) {
+function receiveMessage(close_button_url, event) {


We could use lowerCamelCase here, for consistency with the rest of the script.
In the end we should redo all of it.

donquixote · 2025-05-28T01:52:11Z

src/Controller/ViewerController.php

+   *
+   * @see \Drupal\Core\Form\ConfirmFormHelper::buildCancelLink()
+   * @see \Drupal\Core\EventSubscriber\RedirectResponseSubscriber::getDestinationAsAbsoluteUrl()
+   */


It would have been more straightforward to simply read the destination parameter with javascript. However, I have not found this done in Drupal core.
Reading the parameter in php makes sure that we apply the same processing as in other places.

This said, I did look for other places that parse the destination parameter, and it does not look very standardized.

donquixote · 2025-05-28T01:54:58Z

templates/collabora-online.html.twig

-    if (closebutton == 'true') {
-        options = { closebutton: true };
-    }
+    const close_button_url = {{ closeButtonUrl|json_encode|raw }};


This is the most "correct" way to escape and quote values for js.
An alternative is '{{ varname|e('js') }}', but this also escapes ':' because it is meant for a broader purpose.
Originally we are just doing "{{ varname }}" but this is wrong, e.g. "{{ '"' }}" leads to broken js.

I restored

const close_button_url = '{{ closeButtonUrl|e('js') }}';

It does indeed get escaped into

// This is defined in the js/cool.js const close_button_url = 'http\u003A\/\/web.test\u003A8080\/admin\/content\/media';

But it works just fine on the redirect?

yes, you can verify with js console: 'http\u003A\/\/' === 'http://' is true.

The |e('js') is more noisy because it is designed for a wider range of scenarios.

Also, you need to remember that you must use double quotes "{{ var|e('js') }}", to support all possible strings.
In this case we usually won't have double quotes in the url, but better to always do the right thing.

I still prefer |json_encode|raw, it relies on logic instead of how a specific twig feature behaves.

…tination.

This line should have been removed in c95f8ec.

brummbar · 2025-05-29T13:22:06Z

src/Controller/ViewerController.php

+  protected function getDestinationUrl(Request $request): ?Url {
+    $destination = (string) $request->query->get('destination');
+    if (
+      !$destination ||
+      UrlHelper::isExternal($destination) ||
+      // Instead of ltrim(), simply discard a destination with missing or
+      // unexpected leading slashes.
+      !str_starts_with($destination, '/') ||
+      str_starts_with($destination, '//')
+    ) {
+      return NULL;
+    }
+    $parsed = UrlHelper::parse($destination);
+    $options = [
+      'query' => $parsed['query'],
+      'fragment' => $parsed['fragment'],
+      'absolute' => TRUE,
+    ];
+    return Url::fromUserInput($parsed['path'], $options);
+  }


This approach seems too defensive. There is similar code in core, but for legacy urls. I don't care about legacy in our code.
Let's just use the destination service:

try { @see \Drupal\Core\Routing\RedirectDestinationInterface::get $destination_url = \Drupal\Core\Url::fromUserInput(\Drupal::destination()->get())->setAbsolute(); } catch (\InvalidArgumentException) { $destination_url = NULL; }

We are not here to provide all the possible scenarios when users mingle with the redirect url. What we care is only the security part.

By using the destination service, we get a value (and thus a close button) even if the query parameter is empty.
This is because Destination falls back to the current url.

It seems to me that:

The destination service is meant for code that wants to add a destination parameter to a url.

The actual redirect is done by other code such as RedirectResponseSubscriber, which does not use this service, and which I am replicating here more or less.

By using the destination service, we get a value (and thus a close button) even if the query parameter is empty.
This is because Destination falls back to the current url.

(and a failing test)

brummbar · 2025-05-29T13:23:40Z

tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php

+   * @testWith ["view", "/admin/structure"]
+   *           ["edit", "/admin/structure"]
+   *           ["edit", "/non/existing/path?x=y"]
+   *           ["edit", "malformed/path", false]
+   *           ["edit", "//multislash/path", false]
+   *           ["edit", "https://example.com/hello", false]


We said already that data providers eat time in these non unit test cases. Let's avoid this. How can we avoid the install loop?

Furthermore let's test only the exception case (no destination), external case (fallbacks to /) and one working one.

keep in mind this is ExistingSite, so no install loop.
The data providers are probably not much different from calling multiple cases from the same test method.
The duration is then from opening the iframe and loading the editor.

Indeed even with ExistingSite, by combining all cases into one, for me the time is reduced from 15s to 5s.
One reason is that we don't have to repeat the create user, login, and create document media.

Furthermore let's test only the exception case (no destination), external case (fallbacks to /) and one working one.

We do want to cover preview vs edit:

The UI looks a bit different, and the close button is placed differently. (this is a bit of a canary test, telling us if something changes in CollaboraOnline)

In our own code, the logic that determines close button or not also determines preview vs edit.

I would also like to cover malformed urls, but ok I removed it.

brummbar · 2025-05-29T13:24:51Z

tests/src/ExistingSiteJavascript/CollaboraIntegrationTest.php

+    // Repeat the click until the button disappears.
+    $this->getSession()->getPage()->waitFor(10000, function () use ($close_button, &$n) {
+      // On successful click, the user is redirected to the destination url.
+      // As a consequence, the close button disappears, and subsequent calls
+      // to $close_button->click() trigger a NoSuchElement exception.
+      // By relying on this exception, we avoid any kind of race condition that
+      // could lead to rare random test failures.
+      try {
+        $close_button->click();
+      }
+      catch (NoSuchElement) {
+        return TRUE;
+      }
+      return FALSE;
+    });


Odd, let's remove it. We click once, we need to be redirected.

It's not working for me, unless I wait e.g. 200ms before clicking the button.
I tried to find conditions to check whether the button is clickable, but any condition I could find (including post message telling us we are ready) is already true when this happens.

brummbar · 2025-05-29T13:38:33Z

templates/collabora-online.html.twig

-    if (closebutton == 'true') {
-        options = { closebutton: true };
-    }
+    const close_button_url = {{ closeButtonUrl|json_encode|raw }};


I restored

const close_button_url = '{{ closeButtonUrl|e('js') }}';

It does indeed get escaped into

// This is defined in the js/cool.js const close_button_url = 'http\u003A\/\/web.test\u003A8080\/admin\/content\/media';

But it works just fine on the redirect?

…ders.

…w UI.

donquixote commented May 28, 2025

View reviewed changes

donquixote force-pushed the close-button-destination branch 3 times, most recently from 1d4ee8b to f3c190d Compare May 28, 2025 10:55

donquixote added 4 commits May 28, 2025 13:41

Add destination parameter to operation links.

1bde572

Show close button depending on destination parameter, redirect to des…

81904dc

…tination.

Use @testwith to be compatible with phpunit for Drupal 10.

491b59e

Properly declare generic type for field formatters.

58f12bc

donquixote force-pushed the close-button-destination branch from f3c190d to 58f12bc Compare May 28, 2025 11:43

Remove fragile leftover assertion in WopiControllerTest.

9af234b

This line should have been removed in c95f8ec.

brummbar requested changes May 29, 2025

View reviewed changes

donquixote added 5 commits June 3, 2025 14:57

Remove leftover line in testCloseButtonDestination().

2f2d82d

Optimize test by doing all in one method, instead of using data provi…

a9e9a58

…ders.

Also grant preview permission in the test, to make sure we get previe…

b2f12e4

…w UI.

Don't cover malformed destination urls.

720d70a

Combine multiple cases into one.

8d5a1ac

Show or hide close button based on destination parameter #109

Are you sure you want to change the base?

Show or hide close button based on destination parameter #109

Uh oh!

Conversation

donquixote commented May 28, 2025

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!

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!

Uh oh!