backdrop-contrib / fast_token_browser

An improved user interface for Backdrop CMS's Token browser
https://backdropcms.org/project/fast_token_browser
GNU General Public License v2.0
1 stars 5 forks source link

Fieldsets on screen can't be expanded or collapsed after token browser has been used #10

Closed yorkshire-pudding closed 1 year ago

yorkshire-pudding commented 1 year ago

Steps to reproduce

  1. Install Metatag
  2. Override defaults
  3. Check fieldsets can expand
  4. Browse tokens and insert
  5. Attempt to open or close fieldsets

Expected result Fieldsets work normally

Actual result Fieldsets don't work and console error is generated

collapse.js?v=1.25.1:96 Uncaught TypeError: Backdrop.toggleFieldset is not a function
    at HTMLAnchorElement.<anonymous> (collapse.js?v=1.25.1:96:22)
    at HTMLAnchorElement.dispatch (jquery.js?v=1.12.4:3:12444)
    at r.handle (jquery.js?v=1.12.4:3:9173)
(anonymous) @ collapse.js?v=1.25.1:96
dispatch @ jquery.js?v=1.12.4:3
r.handle @ jquery.js?v=1.12.4:3
argiepiano commented 1 year ago

I've been able to duplicate this problem, and also I have checked, and it doesn't exist in Drupal.

I've been doing a bit of digging. Both in Backdrop and Drupal, Fast Token Browser "hijacks" the link to load the token browser dialog. It basically creates a dialog on its own and then does an ajax call to load the page HTML. All of this is done in fast-token-browser-dialog.js.

This is true both in D7 and B. The difference is that, somehow, the Javascript namespace Backdrop is wiped out - all its objects are removed after doing the ajax page load. This includes removing the function Backdrop.toggleFieldset - which is why it's not found. In Drupal, on the other hand, the Drupal js namespace remains intact, meaning that Drupal.toggleFieldset still exists after the ajax load of the browser.

I need to do more digging to find out where the Backdrop js name is wiped out. But I suspect this is done in core, as a different approach from Drupal. Therefore I believe the solution is to adapt Fast Token Browser to adapt and render the token browser using the standard core dialog approach. I haven't tested this, but I believe this will prevent the wiping-out of the namespace.

argiepiano commented 1 year ago

I've been able to find how/where Backdrop wipes out the Backdrop javascript namespace and all its function. It's happening when Backdrop includes the JS settings, in common.inc, in backdrop_pre_render_scripts(), line 4927. The approach in B is different from D7. Backdrop puts the settings script at the top of the list of scripts to be loaded and executed. The approach in B is to basically create the namespace window.Backdrop from scratch, wiping out anything that was in that namespace previously. In a typical page request, this is no problem, since the rest of the scripts will take care of creating and initializing all the functions within that namespace. But because of the way Fast Token Browser loads the dialog (by doing a "normal" page request and then manually placing the response inside a dialog) the previously initialized functions inside the Backdrop namespace will not be preserved, except for a few ones that are being loaded in the Fast Token Browser page request (which are initialized after Backdrop is wiped out).

There are two solutions:

  1. Modify Fast Token Browser to do a "real" dialog ajax page load the same way the core browser does. This preserves the functions in Backdrop js namespace. OR
  2. Modify core so as to not wipe out the Backdrop namespace. This is unlikely to happen, but @yorkshire-pudding, we can try to see if there is traction. Basically the change is in common.inc from
    $element['#value'] = 'window.Backdrop = {settings: ' . backdrop_json_encode(backdrop_array_merge_deep_array($item['data'])) . "};";

    TO:

    $element['#value'] = 'window.Backdrop = window.Backdrop || {}; window.Backdrop.settings = ' . backdrop_json_encode(backdrop_array_merge_deep_array($item['data'])) . ";";
yorkshire-pudding commented 1 year ago

Thank you @argiepiano for your investigations. That explains it though I'm not sure what the best solution is.

argiepiano commented 1 year ago

A solution I'd like to try is to do away with fast_token_browser_endpoint_output(), which is a custom callback used to deliver the themed token browser content to the browser (which gets manually inserted into the dialog), and instead adapt the content delivery by using core dialog API.

yorkshire-pudding commented 1 year ago

I'm open to any ideas. Thank you @argiepiano

argiepiano commented 1 year ago

@yorkshire-pudding I've created a PR that modifies the approach used by Fast Token Browser. It now uses Backdrop's core Dialog API to display the token browser. The look and feel is exactly the same, but the way the code is sent to the browser now uses the correct means, so that the problem with fieldsets (and other JS UI stuff) is fixed.

Please give this a test and let me know.

Note: once you apply the patch you'll need to clear the browser's cache and also the site's cache, and then reload the pages. Do this a couple of times.

PR #11

(NB: the changes make several parts of the module code obsolete, including one js file, all which I have removed)

yorkshire-pudding commented 1 year ago

Thank you so much @argiepiano - it works a treat. I've found a couple of minor PHPCS issues, but other than those it looks great.

argiepiano commented 1 year ago

Oops! Sorry, I think I pressed the wrong button and closed the PR!!!

argiepiano commented 1 year ago

I actually merged it, sorry! Early morning brain....

argiepiano commented 1 year ago

I actually merged it, sorry! Early morning brain....