[UX][PS] Replace the current token browser in core with the Fast Token Browser implementation from contrib

[herbdool]

Description of the need

I think https://github.com/backdrop-contrib/fast_token_browser is an improvement over the core version. It loads faster, has a more intuitive UI, and will be easier to make accessible to use via the keyboard (maybe it does already).

Proposed solution

Put FTB into core.

Additional information

FTB loads the tokens via ajax which makes it faster when there are a lot of tokens. And it also changes the way tokens are inserted. First the person selects the token - it shows up as selected. Then they click on / select the text field. It then gets inserted. Core has a clunkier approach which requires the unintuitive approach of selecting the field first then finding the token.

Also the modal window can be resized and moved.

[klonos]

I really like this suggestion 👍🏼

...but for the sake of playing the devil's advocate:

• The contrib module counts only 65 active installations at the moment, which given the ~2500 Backdrop sites reporting stats makes it less than 3%, which is too low. But then again, this might be one of the cases where if people knew about the existence of this module, they'd use it more 🤷🏼
• Not sure whether the way the tokens are being inserted in FTB is ideal 🤔 ...the token seems to always be appended to the end of the existing text in the target text field/area, which makes it really cumbersome and annoying when the intention is to add things in the middle or start of the text.

[yorkshire-pudding]

Please see https://github.com/backdrop-contrib/fast_token_browser/issues/3 - I do think the UX should be improved before it is merged into core.

As @klonos is the devil's advocate, I should probably be the angel's advocate and say that core breaks if you have a lot of tokens which this fixes. If we can get the UX improved, then it is not a new feature per se but a replacement of an existing feature.

[argiepiano]

the token seems to always be appended to the end of the existing text in the target text field/area

This should be pretty simple to fix - coming up with a combination of the behaviors of the current token browser and FTB

[klonos]

Yes, if we managed to address that UX issue, I'd be happy for this to get into core. The way that FTB works (loading tokens via AJAX as needed) sounds like how things should be working to begin with.

[klonos]

...I've added the [PS] (Performance & Scalability) "tag" in the title of this issue, as per https://docs.backdropcms.org/documentation/how-to-use-the-core-issue-queue, based on the fact that the implementation of loading tokens in FTB via AJAX is a performance improvement.

[yorkshire-pudding]

@klonos - the latest release with the changes from @argiepiano , I believe, have addressed your concerns.

[klonos]

Thanks @yorkshire-pudding 🙏🏼 and thank you @argiepiano 🙏🏼 🙏🏼

I've given the FTB module another spin, and the UX I had concerns for has solved, but there is still a small bug for a follow-up:

1. place the cursor in the middle of the existing text in a token-enabled field
2. trigger the token browser
3. click on a random token -> the token gets added to the place where the cursor was placed in step 1 👍🏼
4. click on another token -> the token gets added to the start of the text 👎🏼 ...I'd expect it to be added right after the token added in step 3

But perhaps we can leave that to be solved in the PR for merging the functionality into core. Anyone has the time/energy to work on that?

Other UX/consistency things that I would like us to consider when in core:

• Rename the 1st column in the token browser window from "Name" to "Token"
• Merge the human-readable name of the token and the token itself into the 1st column.
• The token should be rendered via theme_label_machine_name(), and not be clickable.
• A new column should be added to the table, with "Place token" buttons that are styled the same as the regular "operations" buttons we have in other tables in the admin UI (instead of the links used now to add the tokens)
• The machine names for the top-level tokens should be changed from node, current-date, current-page to [node:...], [current-date:...], [current-page:...] etc. instead.
• Add zebra stripping to the table rows.
• Add hover-over CSS color to the table rows.
• Add hover-over indication to the expand/collapse chevron icons.
• Add vertical scroll within the token browser modal. Currently, it keeps getting longer and longer as you expand more token categories, and at some point the tokens you are after are past the point where the target field is visible. You cannot drag and drop the token browser window, since it cannot be dragged above the top of the page.
• Adjust the z-index of the token browser window, so that it is below the admin bar and any menu item in the admin bar.
• Add the name of the target field to the title of the browser window. So instead of the static "Token Browser" title, there should be a "Add tokens to field xyz".
• See if we can cache tokens, and provide a search (instant filter as you type) functionality, instead of people having to drill down the long list of the various tokens.

That's it for now. Curious to see what people think about the above suggestions, and I'd be happy to do another round of review and make further UX suggestions once we have a PR sandbox to play with.

[argiepiano]

@klonos said:

click on a random token -> the token gets added to the place where the cursor was placed in step 1 👍🏼 click on another token -> the token gets added to the start of the text 👎🏼 ...I'd expect it to be added right after the token added in step 3

But but but... the issue was to mirror the behavior of the core browser, and this is exactly what the core browser does!

[argiepiano]

@klonos , thanks for the "laundry list" of changes. I was going to take a stab at this, but this is a rather daunting list now - it forshadows an issue that will be in the queue for years, so perhaps it's best to leave this as contrib? Or perhaps others feel that this list is not a "sine qua non"?

Anyway, besides those changes, there are a few important code-wise changes to make to the JS. The JS is very readable and clean, but unfortunately doesn't use namespacing for variables or functions - instead it stores things as global window variables that could be accidentally overwritten, making the code less than safe. Even if this remains in contrib, this needs to be fixed. @yorkshire-pudding I'll submit another PR fixing some of those issues.

[yorkshire-pudding]

I'm a firm believer that we shouldn't let the desire for perfect stand in the way of incremental improvement (I actually think that this should be a "Backdrop principle").

With the wonderful improvements already by @argiepiano and the suggested code namespacing refinement, perhaps this should just focus on anything important that is missing from this module when compared to core, with anything else being separated for later improvement.

[klonos]

... I was going to take a stab at this, but this is a rather daunting list now - it forshadows an issue that will be in the queue for years ...

Apologies @argiepiano ...I didn't mean to deter you or anyone else from working on this. Please do not let my OCDs and extensive list of "ideal" get in the way - everything I listed in my last comment can be follow-ups (or I can file PRs against any PR created by others here for consideration). I simply wanted to document these points, since I took the time to do a UX review. And since we do not have any other ticket to do that, here seemed like a relevant place to do it (it'd be silly to file a follow-up for an issue that doesn't even have a PR yet).

I'm a firm believer that we shouldn't let the desire for perfect stand in the way of incremental improvement (I actually think that this should be a "Backdrop principle").

Yes! That ^^ 👍🏼

[quicksketch]

I think this would be a great 1.30.0 feature. Is anyone interested in being the advocate?

[herbdool]

I think refactoring the JS might be a blocker https://github.com/backdrop-contrib/fast_token_browser/issues/19.

[yorkshire-pudding]

I think refactoring the JS might be a blocker backdrop-contrib/fast_token_browser#19.

I love the Backdrop community. Yes, this was a blocker but within a few hours of this message, there were two PRs to fix it; thanks to both @herbdool and @argiepiano .

[argiepiano]

I think refactoring the JS might be a blocker https://github.com/backdrop-contrib/fast_token_browser/issues/19.

That issue has been fixed. [jinx, @yorkshire-pudding 😆 ]

[argiepiano]

Just to avoid duplicating efforts - I'll try a PR to move this to core. I'll let you know ASAP if this becomes too much at the moment, so someone else can take over (it's fun to work on Backdrop, but other duties are competing for my time too).

[herbdool]

@argiepiano give it a shot. There's no rush. I'm trying to fix a storm window IRL.

[argiepiano]

I have submitted an initial PR, WIP. It moves the code from Fast Token Browser into core's system.module, system.pages.inc, system.theme.inc etc, replacing the current token browser completely. This is working on my local.

Aspects that need work:

• The flag show_restricted doesn't yet work (this was a problem of Fast TokenBrowser). This flag is used to hide certain tokens that are not supposed to be used, like [node:tnid] (translation id).
• The flag click_insert is not supported (yet). I have to figure out if this is needed, since the fast token tree approach is different. This flag basically disabled the ability to insert a token into a DOM field by clicking it on the token browser

I'm noticing that there are test failures - probably related to token UI tests. I'll check on this later today.

@yorkshire-pudding, if you fancy doing some testing, you can patch core with the PR and test the browser, for example by going to admin/config/urls/path/patterns and admin/structure/types/manage/card.

Also, you may want to enable modules like Token Help token tree that appears at admin/reports/tokens

[EDIT]: the test failures are happening only on PHP 5.6 and don't seem to be related to this PR (?).

PHP Fatal error:  Call to undefined function lock_acquire() in /home/runner/work/backdrop/backdrop/core/includes/bootstrap.inc on line 1307

[argiepiano]

I fixed the issue that prevented the sandbox from being built. It can be tested on the sandbox now.

[yorkshire-pudding]

Thanks @argiepiano. Fast work. I wasn't aware of the flag 'show_restricted' or that it was a problem with FTB. I guess I've used the module on all my sites since 2022, so I've never known anything different.

It all seems to work for me. I'll hold off adding works for me label as you've indicated it needs more work but what is there looks great.

@herbdool - as you created this issue initially, would you be willing to be the 'Advocate' for it? I'm already advocating for another issue.

[argiepiano]

I added the flags 'show_restricted' and 'click_insert'. This means that restricted tokens like [node:tnid] are not shown in the tokens opened by the "URL alias pattern" tab.

Also, the click_insert flag assures that Token Help shows a token tree without the clickable links to insert them.

Ready for testing and review.

[yorkshire-pudding]

Thanks @argiepiano . I have tested the revised PR and can confirm it all still works. Plus:

• I do not now see [node:tnid] in url alias page. also don't see on token block. I don't know if or where it actually is visible but that is not relevant.
• I do not see an insert link in Token Help - just the text of the token (I wondered about this for ages in the module)

Therefore all works.

[herbdool]

I can advocate for it.

[herbdool]

Code looks good. Though I'm wondering @argiepiano how much work it would be to first add the flags show_restricted and click_insert to the contrib module so we can review those separately from the rest of the code. Then it'll be clear that it's just moving code in.

I suppose we don't want to bring along the git history since the code is getting spread around the system module. So I'm okay with that.

[argiepiano]

@herbdool thanks. I worked on adding this functionality to Fast Token Browser. As I was working on that I discovered an issue with the token cache that Fast Token Browser uses, when adding show_restricted. In a nutshell, the tokens for any given type are cached as they are retrieved (via ajax).

The problem is that this cache doesn't take into account whether show_restricted is TRUE or FALSE. So, for example, if you open the "Content" tokens here (where show_restricted is FALSE): admin/config/urls/path/patterns, and then open the Token Help list of tokens (where show_restricted is TRUE), the latter will NOT show the restricted tokens, because it uses the cached tokens generated in the first list of tokens (where show_restricted was FALSE).

I think I know how to fix this. I'll give that a try next. Setting to NW.

[argiepiano]

PR has now been fixed. To check that it works:

• Open the sandbox for the PR
• Install and enable Token Help
• Clear caches (clear browser caches to be on the safe side as well)
• Visit admin/config/urls/path/patterns, open the "Content" token type, and verify that [node:tnid] is not included 👍🏽
• Visit admin/reports/tokens and verify that [node:tnid] is listed there (before this fix, it wasn't, as it was using the cached version of the token output with show_restricted = FALSE)

The way I solved this is by using the variable $show_restricted when building the hash that is used as the cache ID. This way, Backdrop creates two different cache entries for the same token types, when the token output includes or excludes the restricted ones.

Please test and review.

[yorkshire-pudding]

The way I solved this is by using the variable $show_restricted when building the hash that is used as the cache ID. This way, Backdrop creates two different cache entries for the same token types, when the token output includes or excludes the restricted ones.

Clever solution

Tested and works for me.

[herbdool]

All looks good to me now. Thanks @argiepiano!

[quicksketch]

I tried out the sandbox and it works pretty good! I left a bunch of feedback: https://github.com/backdrop/backdrop/pull/4884#pullrequestreview-2396584152

I didn't review all the JavaScript, quite yet, but there are several things that could be addressed.

[argiepiano]

Ok, thanks @quicksketch. Will get to this over the weekend.