Deprecate and replace jQuery Joyride (for tours)

Contrib modules also use it. At least https://www.drupal.org/project/cdn does :)

Right at the very beginning of this issue thread, I do feel that two questions were asked:

A) Relying on Joyride JS-library or seek another one?
I'm not eligible to form an opinion on that.

B) Keeping the Tour functionality in Drupal core?
I vote a heartful +1 to support keeping Tour module in Drupal core as it has a great (yet underused, indeed) added value for those in the audience we'll try to engage better: sitebuilders.

It still mentioned many times in help topics issue #2920309: Add experimental module for Help Topics as better approach to help

Oh no! We were literally talking yesterday about the Tour functionality in core being key to the "Try Drupal" refresh on Drupal.org!!

Being able to guide people around a new demo install is super useful. Would be a great shame to lose it but I also appreciate that the library in question does need support for that to be safe.

Tour module has a lot of accessibility problems. Not all of them are Joyride's fault, but linking the related issue.

Hello all!
Added quick implementation of https://github.com/shipshapecode/shepherd
which seems me good alternative of Joyride.
DEMO: https://shepherdjs.dev/demo/

Looks pretty accessible and configurable out of box.
This patch is just for tests. But mostly api and configs should be same.
Use default umami profile installation for tests with drush for example:
`drush si demo_umami`

Could please look alo into this? Now the tour is hard coupled with the toolbar: #3012027: Decouple tour triggering from the toolbar.

Great research and PoC, @finnsky! That's looking very promising indeed. I'd love to see this fleshed out further.

1. There are still a few lingering references to jQuery Joyride in tour.js, but I'm sure you knew about that :)
2. The bigger concern are accessibility and usability: the current styling is problematic in terms of visual association (no arrow pointing to the targeted element, necessary for visual users) and the absence of contrast (necessary for visual users, especially visually impaired ones, but really it affects all of them).

Before

After

(Fixed two nits in the patch: Joyride was still being injected and was not yet deprecated.)

@Wim Leers i see much more abilities to optimise tour here.
https://github.com/zurb/joyride#usage Joyride required tour source to be in html list on page. But shepherd dont. we can change backend of module to send simple js object with tour items in drupalSettings. So maybe we need to rework it?

That HTML list is just an implementation detail and not an API, so I think it's definitely reasonable to rework this 👍 Thanks for taking this on!

Storing tours in drupalSettings is bad idea, because it syncs with every ajax requiest.

@finnsky Tours are markup and conversion of it to JSON looks useless browser operation

Also the related issue could change how tours could be found

Storing tours in drupalSettings is bad idea, because it syncs with every ajax requiest.

Huh? drupalSettings.ajaxPageState does, not all of drupalSettings. Or what am I missing?

@andypost thanks for review
1) get tour items from js object cheaper than search html list it on page/parse attributes/and render as tour items again.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/tour/j...
2) On shepherd side we need js object. Not markup. Example from specs:

tour.addStep({
  title: 'Creating a Shepherd Tour',
  text: `Creating a Shepherd tour is easy. too!\
  Just create a \`Tour\` instance, and add as many steps as you want.`,
  attachTo: {
    element: '.hero-example',
    on: 'bottom'
  },
  buttons: [
    {
      action() {
        return this.back();
      },
      classes: 'shepherd-button-secondary',
      text: 'Back'
    },
    {
      action() {
        return this.next();
      },
      text: 'Next'
    }
  ],
  id: 'creating'
});

3) We may use something like Drupal.TourNotRefresheableOnEveryAjaxRequest. I'm sure can be way
4) Shepherd has pretty nice default markup. We should reuse it. So current backend should be changed anyway. Right now it adds Title/Text/Pagination in predefined tags, which we will need to parse on js before send it to shepherd.

Default shepherd markup

Current tour module list.

So API will be same. Tours will be attached in same way https://www.drupal.org/docs/8/api/tour-api/overview#content-tour-yaml-do...
As @Wim Leers mentioned it will be change only implementation in module.

1) updated shepherd to 5.0.0 to disable default shepherd styles https://github.com/shipshapecode/shepherd/pull/526
2) Moved tour items to drupalSettings. Backenders help definetly needed.
3) Updated js according.
4) Updated part of css.
5) Enabled partial background shadow in shepherd. Looks nice imo. Browsers tests needed.
6) Tested a11y. On gif attached i used tabs only :)

TODO:
1) Manage js logic in case of query in url http://example.com/foo?tour=1&tips=bar
2) Manage data attributes.
3) Update css in module, stable theme, and in umami installation profile.
4) Update functional tests. Here should be same problems as in https://www.drupal.org/project/drupal/issues/3074267#comment-13232996

Wow, that auto-highlighting feature is excellent! That was often lacking in existing tour integrations.

I'm not sure it is equally clear for low vision users though. This will need accessibility review. I see you already tested as a keyboard-only user for the GIF in #20 — that's already an excellent piece of validation, thanks! :)

It'd be great to fix those last 4 failures while we wait for accessibility review :)

Updated a bit backend part + little changes to js.

Sorry, missed to add library in patch

Tried to fix the failures.

Quick review of #28 on Safari 12.1.2:

1. The auto-highlighting function is AWESOME
2. We probably need to look at a border or something around the actual speech bubble, to separate it from the highlighted region
3. On Safari at least, the scroll-to-region is not working properly in the Umami demo. Seems to scroll consistently with the highlighted region (and bubble) off the top of the screen
4. Apostrophes etc are being escaped. Probably need to look at how we can avoid this or add it into documentation

Just one more failing test to fix!

Yay, this is green now! Well done, @Oleksiy! 👏

@Oleksiy Do you think you could also address @rachel_norfolk's feedback in #29?

+++ b/core/modules/tour/tests/tour_test/src/Plugin/tour/tip/TipPluginImage.php
@@ -35,14 +35,14 @@ class TipPluginImage extends TipPluginBase {
+      'title' => Html::escape($this->get('label')),

Although I think this already addressed her point 4 :)

Also worth taking a look in places that might still need some JoyRide artefacts pulling out?

Places I think look like candidates:

1. core/modules/tour/tour.libraries.yml -- references jquery but do we still need it?
2. core/modules/tour/css/tour.module.css
3. core/modules/tour/js/tour.es6.js -- is _removeIrrelevantTourItems() still needed or need updating?
4. core/core.libraries.yml -- line 400ish
5. core/themes/seven/css/components/tour.theme.css -- hmm, what do we do about this?
6. and the actual joyride lib!

Thanks, @Wim Leers

@Oleksiy Do you think you could also address @rachel_norfolk's feedback in #29?

Yes. It seems related to the point 4.

#34 All good feedback! Only point 6 is not quite true: we can't remove an asset library, we can only deprecate it. Removing it would be a backwards compatibility break.

Deprecating asset libraries is possible since about two months, see https://www.drupal.org/node/3064022 :)

Ah that is a good point about deprecation (though it does remind me we *really* need to declare what is a public and what is a private api, so we can replace the implementation without changing the public interface of the tour module!)

It might mean that 2,3,5 and 6 should all remain ( as "part of the library") ?

Rerolled from #32 and made a small change below.

1. +++ b/core/assets/vendor/shepherd/shepherd.min.js
   @@ -0,0 +1,4 @@
   +/*! shepherd.js 5.0.0 */
   

   included version is 5.0.0

2. +++ b/core/core.libraries.yml
   @@ -923,6 +924,16 @@ picturefill:
   +  version: "4.6.0"
   

   in lib, it's 4.6.0

So changed 4.6.0 to 5.0.0

Changed the library version to 7.0.1

As https://github.com/shipshapecode/shepherd/releases/tag/v7.0.1 was released, let's work on the new version if it's ok. But:

Shepherd v6.x no longer ships with the required polyfills to work with IE 11 out of the box

See https://github.com/shipshapecode/shepherd#ie-11

Updated issue summary to document @todo items referencing this issue.

Also regarding #40. First, good catch! One of the three polyfills needed - Object.assign is already part of core drupal.object.assign, but I'm setting this to needs work to add the two additional ones needed, Symbol and element.matches and to make them dependencies of core/shepherd. For reference, polyfills were recently added as part of a library update in this issue: #3108402: Update to Popper.js to 2.0.0

Also wanted to make sure shepherdjs meets the criteria here: https://www.drupal.org/core/dependencies#criteria, I may have missed this in the issue but wanted to mention just in case as it would be unfortunate if all this effort was directed towards it only to discover it didn't meet the criteria necessary to be included in core.

Tagging for Drupal 10 since this is one of about two ways we can replace our jquery cookie shim (the other would be relying on joyride's fallback behaviour).

current patch is broken. since ol#tour was removed and some code still expect it. gonna fix.

Reworked patch.

1) Added `Shepherd` in eslint globals
2) Updated library to 7.0.4
3) Updated css for module, stable, claro and umami
4) Fixed js.
Now all previous features work. Like:

   * - tour: When ?tour=1 is present, the tour will start automatically after
   * - tips: Pass ?tips=class in the url to filter the available tips to the

Removed jQuery where it is possible.

Current results:

Seven:

Claro:

Umami:

We need to update specs and tests. Patch is mostly ok.

Hi all!

1) Fixed coding standard messages.
2) Managed focus states for all themes.
3) Removed `console.log`. Sorry:)
4) Fixed tests
5) Moved tour.theme.css to /theme dir for claro

Patch became ready to implement now. Please review.

The tests still need work

Fixed TourViewBuilder and tests.

(Sorry, for the empty comments above. There was some issue with the comment submit)

Patch #53 rerolled.

After a test, I found all right execpt by one issue.

At the home screen of unamy profile, the first tour message has a bad space at the right.

In evaluation for the use of `shepherd`, I'd like to throw my evaluation for the use against Drupal's Criteria for adding dependencies.

Maintainership of the package

The package is managed by Ember Consulting, and features 79 contributors with 200 applications on GitHub marked as direct dependencies of `shepherd`. It features the use of automated dependency management and has no security advisories active. It features a wealth of commits and has been recently committed to by varying contributors - not only dependabot.

Security policies of the package

Packages with security vulnerabilities are publicly disclosed via the built-in vetting functionality provided by GitHub, and the management of those dependencies is automated with the use of Dependabot. The commit history shows there's a significant amount of this work happened and without issue.

Expected release and support cycles

Releases are infrequent but they consistently have occurred at least once every couple of months throughout the current year (2020). Release notes however are a little inconsistent - as some lack detail.

Code quality

`shepherd` features the use of many open source static analysis tools in its workflow and uses CI as part of development workflow. With the security measures and tools in place to secure the analysis of the code quality and security - not limited to maintainability rating scores and code coverage tests. `shepherd` appears to be a very good library if it's fit for purpose.

This

• Rerolls
• Addresses #55 and other similar styling issues that I identified while fixing that one
• Updated to latest version of Shepherd
• Adjusted the config so arrows don't overlap with the element they are tethered to.
• Confirmed the accessibility is significantly better than Joyride. The modal receives focus on activation, the tour can be navigated via keyboard, and the close/next controls correctly use a <button> element instead of <a>. I'd want to spend a bit more time with it before granting an accessibility signoff, but this is promising.

That looks great! love the highlight of the element. Just a few details:

1. +++ b/core/modules/tour/js/tour.es6.js
   @@ -25,10 +25,12 @@
   +      Array.prototype.forEach.call(
   +        context.querySelectorAll('body:not(.js-tour)'),
   

   We have a polyfill for that core/drupal.nodelist.foreach, also feels like a no-jquery once? if that's the case I wouldn't change it here at the moment.

2. +++ b/core/modules/tour/js/tour.es6.js
   @@ -148,26 +154,71 @@
   +            const tour = new Shepherd.Tour({
   

   Can we put the default settings at the top of this file? not a fan of putting a whole bunch of configuration values in the middle of code logic, makes it easier to find as well if people want to change it somehow.

3. +++ b/core/modules/tour/js/tour.es6.js
   @@ -202,7 +253,7 @@
          _getDocument() {
   -        return $(document);
   +        return document;
   

   do we still need this method?

#61.1 Looks like that was trying to get around using once(), so I agree that should be switched back and taken care of in #3183149: Deprecate jquery.once and use the new once lib

#61.2

Can we put the default settings at the top of this file? not a fan of putting a whole bunch of configuration values in the middle of code logic, makes it easier to find as well if people want to change it somehow.

This could be seen as out of scope as the existing joyride tour accepts config in this manner, but this also seems like a potentially useful non-risky change so I've added it in the initialization of tour's JS settings in \Drupal\tour\TourViewBuilder

#61.3 May technically be out of scope too, but looks like _getDocument() hasn't been needed for ~7 years #2088121: Remove Overlay and seems like a reasonable thing to address as it's incorporated into code blocks we're already refactoring.

This provides a reroll, addresses several things caught by the new custom commands on DrupalCI, and a few other small fixes.

I also did an accessibility review and compared it against the many accessibility bugs found in the Joyride implementation: #2961001: [META] Improve accessibility of tour module

Here's a list of accessibility issues with the Joyride implementation that are fixed with the Shepherd implementation here:
Joyride accessibilty issues that are fixed here

• When the toolbar button has keyboard focus, activatig the button makes the tooltip appear, but focus remains on the button. Sighted keyboard user can see the first tip, but effectively cannot advance to the next tip. Focus will move from tour toolbar button to user account link
• Should move focus to start of dialog
• For assitive tech, there's no indication that anything has happened. There is an aria-pressed="false", but this DOES NOT CHANGE when pressing it. With screen reader, when you arrive at the button, it says "tour, button, not pressed". Press it, the dialog appears, but focus remains on the toolbar button, and the screen reader says "button, not pressed"
• We would expect it to announce a "dialog". Actually it has a dialog role, but it isn't announced because focus hasn't moved there. If you use a mouse when running a screen reader, you can hear the dialog being announced.
• Focus not constrained in dialog
• No focus style for the tour tip close button.

I tested with browser Zoom and there it's not always ideal at higher magnifications when the tourtip text is longer. This is also a problem with Joyride, but the symptoms are different with Shepherd.
Shepherd: The tip may overlap the content it's highlighting
Scrolling will align it, however

Joyride: the tip may not be fully in view
Similar to Shepherd, scrolling will get these to a good place.

While zoom isn't perfect with Shepherd, it does not appear to be a step down from Joyride.

While Shepherd seems to address all accessibility "bugs" There are still some good suggestions in #2961001: [META] Improve accessibility of tour module, but are better categorized as improvements. That issue should get updated when this one is committed.

1. +++ b/core/modules/tour/js/tour.es6.js
   @@ -122,6 +125,7 @@
   +        this._removeIrrelevantTourItems(this._getTour());
   

   Any reason why this moved from toggle to initialize? I think the idea was that the page can be dynamic and the moment the tour is triggered, some items could have gone missing.

2. +++ b/core/modules/tour/js/tour.es6.js
   @@ -148,26 +152,49 @@
   +            $tour.forEach((step) => {
   

   here $tour is the settings right? should be tourSettings or something.

3. +++ b/core/modules/tour/js/tour.es6.js
   @@ -148,26 +152,49 @@
   +              if (step['#wrapper_attributes']['data-class']) {
   

   any chances we can use something more meaningful than #wrapper_attribute? it's too render-array-y.

4. +++ b/core/modules/tour/src/TourViewBuilder.php
   @@ -15,58 +15,79 @@ class TourViewBuilder extends EntityViewBuilder {
   +      $build['#attached']['drupalSettings']['tourShepherdConfig'] = [
   

   Putting that in the PHP could be good, what I meant was to put that at the top of the tour.es6.js file, so that it's not mixed with the initialization logic.
   I don't think there is a lot of value to make it go through drupalSettings.

5. +++ b/core/modules/tour/tests/src/Functional/TourTest.php
   @@ -75,33 +75,57 @@ public function testTourFunctionality() {
   +    $tips = $this->getTourTips();
   ...
   +    $elements = $this->findTip($tips, [
   

   with this we're not using the DOM anymore to check for elements, it's only checking that a value is in an array.

   I'm ok with it, just that the test is very different.

#65.1

Any reason why this moved from toggle to initialize? I think the idea was that the page can be dynamic and the moment the tour is triggered, some items could have gone missing.

It looks like it was removed from toggle in #24 and I'm not sure why. I agree with the reasoning that this should be in toggle, so I've returned it there.

#65.2
Improved the variable naming

#65.3 This has been changed as the use of #wrapper_attributes was for joyride, which required an already-rendered HTML list. After making these changes I realized it may be a BC break as custom/contrib may be attempting to override this property. I'm leaving the changes in for now as this will likely require some discussion and that will make the discussion easier. The tour is not rendered as an HTML list, so there are some definite differences already occurring. I find @lauriii is quite helpful when I'm trying to figure out acceptable levels of BC changes

#65.4

I don't think there is a lot of value to make it go through drupalSettings.

My thought is that the tour items are already configured via tourSettings, so it would be structurally consistent to approach global tour config the same way. It's not a particularly strong opinion, though.

#65.5
These tests were added well before I started working on the issue, but the comment motivated me to look at the tests a little closer. My takeaway is similar, it's quite different but the changes seem like a good way to adjust to the new library. It's quite possible tests written from scratch would be structured quite differently, but I'm glad so much of the existing test code still works.

Fixing a stray keystroke that got into the patch in #66

#67 addressed an issue that allowed tests to run, and once the tests run there were failures due to #wrapper-attributes being removed since tips are no longer a render array. This suggests it may be necessary to keep that property for BC purposes. I'll look into that further.

1. +++ b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
   @@ -120,9 +120,11 @@ public function getAttributes() {
   -    return ['#markup' => $output];
   ...
   +      'title' => Html::escape($this->getLabel()),
   +      'body' => $this->token->replace($this->getBody()),
   +      'location' => $this->getLocation(),
   

   We should update docs in Drupal\tour\TipPluginInterface given that the expected return value has changed. However, the fact that we have to make changes to an interface points out that this is a BC breaking change.

   Given that the name of the method name is ::getOutput, it seems that it would be better to split this method into three different methods for each of the array items (to not have to deal with arrays). I'm not sure what to do with the current method given that it would have to be deprecated.

2. +++ b/core/modules/tour/src/Plugin/tour/tip/TipPluginText.php
   @@ -120,9 +120,11 @@ public function getAttributes() {
   +      'title' => Html::escape($this->getLabel()),
   +      'body' => $this->token->replace($this->getBody()),
   +      'location' => $this->getLocation(),
   
   +++ b/core/modules/tour/tests/src/Functional/TourTestBase.php
   @@ -33,13 +33,14 @@ abstract class TourTestBase extends BrowserTestBase {
   -      // Tips are rendered as <li> elements inside <ol id="tour">.
   ...
   +      // Tips are rendered as drupalSettings values.
   

   Adding a reminder for myself to check all of the potential impacts of this change later.

3. +++ b/core/themes/seven/css/components/tour.theme.css
   --- a/core/themes/stable/css/tour/tour.module.css
   +++ b/core/themes/stable/css/tour/tour.module.css
   

   The fact that we have to make this many changes in Stable suggests that this patch is introducing BC breaking changes for markup. Is there way for us to revert those changes in Stable?

Regarding the TipPlugin BC concerns from #70 #70, I've thought of a potential approach.

Given that:
Tip Plugins extend TipPluginBase.
TipPluginBase implements TipPluginInterface

1. Deprecate the joyride-specific getOutput() method in TipPluginInterface.
2. Create a TipPluginShepardCompatibleInterface with new Shepard-specific methods. This interface would be implemented by individual plugin definitions, not TipPluginBase
3. The tour module's one Tip plugin, TipPluginText, would implement TipPluginShepardCompatibleInterface
4. TourViewBuilder would build tips differently if the tip was instanceof TipPluginShepardCompatibleInterface
5. In Drupal 10, TipPluginBase would implement The TipPluginShepardCompatibleInterface in addition to TipPluginInterface

I think #71 sounds like a great approach, but note we have the 1:1 rule here, so we could do 5 now, with a best-intentions approach by putting 4 inside the base plugin as a bridge

@tim.plunkett suggested adding a new getConfiguration() method to TipPluginBase that defaults to returning null. Then the checks for old/new plugin can be made based on what getConfiguration() returns. That is the approach in this patch.

This patch also added shims to Stable that rebuild the markup to match Joyride. Accomplishing this is different depending on whether the TipPlugin uses getOutput()
or getConfiguration(). Tests were added to confirm both scenarios result in the same markup that Joyride would have been provided.

Some of the remaining tasks (assuming the approach is sound)

• JS can probably be cleaned up
• Any changes to Stable also need to go into Stable 9
• Needs more comments
• Deprecations haven't been added yet

Fixed CS issues.

1. +++ b/core/modules/tour/src/TipPluginBase.php
   @@ -79,4 +79,14 @@ public function set($key, $value) {
   +   * @return array|null
   ...
   +  public function getConfiguration() : ?array {
   

   For better DX, instead of returning an array, we could split this into multiple methods.

2. If the interface doesn't contain ::getOutput method in Drupal 10, people writing code using Drupal 10 could end up writing code incompatible with Drupal 9 LTS because the interface in Drupal 9 would still require ::getOutput. It's also not ideal to leave classes with stale ::getOutput methods which would have to be cleaned up at a later point when Drupal 9 is not supported anymore.

   Solution for this would be to deprecate Drupal\tour\TipPluginInterface as a whole, and replace with a new interface. If we also created a new base class for the new interface, Drupal\tour\TipPluginBase we wouldn't have the problem with stale ::getOutput methods getting left in the code base. Then we would check in Drupal\tour\TourViewBuilder::viewMultiple which interface the plugin is implementing as proposed in #71. This way also any code written in Drupal 10 would be compatible with Drupal 9 LTS because it would include the new interface.

This goes with the direction in #75 and the result is a chunky 62k interdiff. Some of the 62k is copying the Tour JS from Stable to Stable 9, but that's certainly not all of it.

Also tried to expand the docs+comments to make this increasingly large patch (even without the Shepherd library) reviewable.

Still needs review from someone that didn't write the past several patches, but spotted these two things in my own patch:

1. +++ b/core/modules/tour/js/tour.es6.js
   @@ -147,27 +150,64 @@
   +                  `<p>${step.body}</p><div class="tour-progress tp-js">${step.counter}</div>`,
   

   tp-js class is not neccessary

2. +++ b/core/modules/tour/tests/src/Functional/Update/TourTipSelectorConfigUpdateTest.php
   @@ -0,0 +1,62 @@
   +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz',
   

   A 9.0.0 fixture was just added to core, probably best to switch to that.

1. +++ b/core/modules/tour/src/TourTipPluginBase.php
   @@ -0,0 +1,189 @@
   +abstract class TourTipPluginBase extends PluginBase implements TipPluginInterface, TourTipPluginInterface {
   

   I'm fine with prepending tour to the class names but I wanted to mention that Shepherd is calling these as steps. I'm wondering if it would make sense for us to rename Tips to Steps?

2. +++ b/core/modules/tour/src/TourTipPluginBase.php
   @@ -0,0 +1,189 @@
   +  public function getAttributes() {
   +    return $this->get('attributes') ?: [];
   +  }
   ...
   +  final public function getOutput() {
   ...
   +    return [];
   +  }
   

   Why do we allow using attributes with the new abstract class but not output?

3. +++ b/core/modules/tour/src/TourTipPluginBase.php
   @@ -0,0 +1,189 @@
   +   * options. Accepted values: 'auto', 'auto-start', 'auto-end', 'top',
   +   * 'top-start', 'top-end', 'bottom', 'bottom-start', 'bottom-end', 'right',
   +   * 'right-start', 'right-end', 'left', 'left-start', 'left-end'.
   ...
   +    return $this->get('location');
   

   Should we add an assert that checks if a valid location has been defined?

4. +++ b/core/modules/tour/src/TourTipPluginInterface.php
   @@ -67,11 +59,32 @@ public function get($key);
   +   * @return string
   +   *   The body content of the tooltip.
   +   */
   +  public function getBody();
   

   Is there a particular reason we want this method to return a string rather than render array? That seems like better DX because it would rely on pre-existing XSS handling in render arrays.

5. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -137,6 +137,10 @@ public static function getSkippedDeprecations() {
   +      // @todo remove the Tour related items below in https://drupal.org/node/3195823
   

   Is there a particular reason for handling these in a follow-up rather than on this issue?

This covers #78, and I confirmed that the deprecated properties I planned to change in #3195823: [PP-1] refactor all tours to not use deprecated code/properties should happen here as it's policy to not have core code use anything deprecated, so this patch now has many new changes to existing tours.

I've run into one snag with this, specific to the deprecated schema properties. I have a test, TourLegacyMarkupTest, that runs a tour that uses these deprecated properties. If I create the tour with a file in config/install DefaultConfigTest, which tests EVERY config in core (tests included) will fail with a deprecation error. I attempted to work around this by removing the tour with deprecated properties from config/install and manually adding it in the test.

 public function setUp(): void {
    parent::setUp();

    // Manually install a tour using legacy properties. If this config is
    // located in config/install there will be deprecation errors in
    // DefaultConfigTest will re
    $config_path = drupal_get_path('module', 'tour') . '/tests/fixtures/legacy_config';
    $source = new FileStorage($config_path);
    $config_storage = \Drupal::service('config.storage');
    $this->assertTrue($source->exists('tour.tour.tour-test-legacy'));
    $config_storage->write('tour.tour.tour-test-legacy', $source->read('tour.tour.tour-test-legacy'));
    drupal_flush_all_caches();
}

I've confirmed the config is added to the database, but the tour that should accompany the page still doesn't load. This is why TourLegacyMarkupTest is failing in the attached patch. Hopefully there is an obvious fix I'm overlooking.

Setting this to needs review. The failing test flipped it to Needs Work, but that was an expected fail and a review/help with #79 is welcome.

Hiding old patch files now that this issue has an MR

Needs rebase against 9.3.x branch. The target branch of MR also requires to be updated to 9.3.x.

Discussed with @alexpott about the changes to the plugins. We talked about various potential approaches, and tried to find one that would be least disruptive to site maintainers, as well as module authors.

Because Drupal\tour\TourInterface::getTip() has a return signature of \Drupal\tour\TipPluginInterface, replacing the interface with another is a bit tricky. Therefore, we thought it would be better to deprecated individual methods from the interface rather than the whole interface. This can be done by adding @deprecated to the method documentation.

New interface is needed for the new methods that we want to add. We should create a new interface that extends \Drupal\tour\TipPluginInterface. Now that the interface is not deprecated, we don’t necessarily need a new base class. Instead of the base class implementing the new interface, we should add a check in the __constructor for whether the class is implementing the new interface, and throw a deprecation error in the cases where it doesn’t. The action that we expect plugin implementation to take is to continue extending the base class, and make the plugin implementation implement the new interface.

We should add empty methods to the base class for deprecated methods in \Drupal\tour\TipPluginInterface that trigger warnings for any code calling them, and returns empty values that are compatible with the signatures defined in the interface. Instead of deprecation errors, the functions should use trigger_error() with E_USER_WARNING severity to make sure that the errors are logged, since the empty values would lead into some functionality rendered unusable.

To avoid the error from being triggered, we should try to find a way to implement BC layers in the core plugin implementations. However, even those functions should call the base class function for the warning to be triggered.

D10 Only (prior to major release)

1. Move the new interface methods upstream to TipPluginInterface. This is allowed because we added the check to the constructor and in D9 you'll get a deprecation warning if you tip plugin doesn't implement the new interface.
2. Remove the deprecated methods and any implementations of them.
3. Consider changing plugins to implement TipPluginInterface only.

D10 Follow-up

1. Deprecate the "new interface" which at this point will be empty but extending TipPluginInterface.

D11 Only (prior to major release)

1. Remove the new interface.

#88 makes sense. I have one concern regarding how to best warn plugins that use getOutput() instead of getBody() / getTitle()
(as a list just to separate points for readability)

• getOutput() is a requirement of TipPluginInterface
• TipPluginBase, implements TipPluginInterface, but it's and abstract class and leaves it up to the classes that extend it to implement getOutput()
• Triggering an error from a TipPluginBase implmentation of getOutput() would never be triggered, as all plugins at the moment must implement their own getOutput(), and there's no parent method to call
• getOutput() is the method that would most benefit from having E_USER_WARNING severity warning trigger, but custom Tip plugins extending TipPluginInterface won't get this warning (Plugins that extend existing tip plugins such as the one in tour_ui will work)

This might be a really bad idea, but could we in __construct() call $this->getOutput() and emit a warning if it is non-empty?

Noticing this in test runs:

  8x: The tour.tip 'location' config schema property is deprecated in drupal:9.2.0 and is removed from drupal:10.0.0. Instead use 'position' with the opposite value of 'location' (top becomes bottom, left becomes right, and vice-versa). See https://www.drupal.org/node/3204093
    4x in TourTest::testTourFunctionality from Drupal\Tests\tour\Functional
    4x in TourTest::testTips from Drupal\Tests\tour\Functional

  6x: Tip plugins implementing Drupal\tour\TipPluginInterface that don't also implement Drupal\tour\TourTipPluginInterface are deprecated in drupal:9.2.0. See https://www.drupal.org/node/3204096
    3x in TourTest::testTourFunctionality from Drupal\Tests\tour\Functional
    3x in TourTest::testTips from Drupal\Tests\tour\Functional

I manually tested Shepherd vs Joyride using the Views tour, the Example tour from examples module and the overview and tour edit pages from the Tour UI module.

Shepherd is visually an improvement, the default modal and highlighting are nice.
With a window resize Shepherd is able to re-orient itself while Joyride cancels entirely.

The "next" button maintains focus on each step, unlike joyride, which make it more accessible.

The main changes in core/modules/tour/js/tour.es6.js are related to how joyride gets the steps from markup on the page while shepherd loads an object stored in drupalSettings.
Shepherd's approach simplifies adding and filtering the steps in toggleTour and _removeIrrelevantTourItems.

Adding a query string ?tour=anything to any page with a tour works for autostarting the tour.
The class name filter just needs a change to the property name, class --> classes, I added a suggestion in the issue fork. With that change it works as well.

This file also checks for convertToJoyrideMarkup to convert the markup for any themes that use stable or stable9 as a base theme.

This is needed for BC, so any themes based on stable or stable9 that style a tour don't have changes to markup.
The markup is similar, but when converted to joyride the <header>, body div, and <footer> elements are removed, the <h3> replaced with <h2>. All the joyride classes are present and on the correct elements, plus the added shepherd classes.

I noticed that in joyride, the <h2> has an id that corresponds to the step tour-tip-first-item-label and the body has an id like tour-tip-first-item-contents. This increments first, second, third etc. In the converted shepherd markup the ids are generated with a uuid like step-0f62c302-ee2e-4d56-9a4f-5f12e4f4e6a9-label. Not sure how important it is to match the ids, it also seems like these are internal for Shepherd.

The javascript and css in stable are identical to stable9.

All the tour.tour yml files were updated, I didn't find any occurrences of attributes, data-id, data-class or location in any files except for where they are supposed to still be in:

• core/modules/tour/tests/tour_legacy_test/config/install/tour.tour.tour-test-legacy-location.yml
• core/modules/tour/tests/fixtures/legacy_config/tour.tour.tour-test-legacy-location.yml

The docs page at https://www.drupal.org/docs/8/api/tour-api/overview will need to be updated after this lands.

Attached side by side screenshots for each theme I tested, Claro, Seven, Bartik, test theme based on stable, and wild west (not based on stable).

The latest changes: https://git.drupalcode.org/project/drupal/-/merge_requests/400/diffs?dif...

• Update variable name for "step" to be more clearly separate from a Shepherd Step
• Improve jsDoc comments, add additional helpful explanation of properties and where they come from
• Fix "classes" property name bug mentioned in #93, adding ?tips=some-class-name now works!

I didn't look at the hook update changes, but the javascript changes look good!

Looks like @bnjmnm addressed all of @lauriii 's comments, I wasn't able to mark them resolved but I commented when I checked each change.
I made a small change to match formatting and comment in tour.es6.js between stable and stable9, and fixed the casing in TourJavascriptTest.

Re adding to the skippedDeprecations - we shouldn't be adding to that. As per the UpdatePathTestBase documentation ALL update path tests should be in the legacy group...

 * - Ensure that the test is in the legacy group. Tests can have multiple
 *   groups.

Dependency evaluation in #58, the IS link didn't go anywhere.
Updated the IS and tagging needs release manager review

The dependency evaluation looks great.

Change records: https://www.drupal.org/node/3204096 - this still needs a description.

https://www.drupal.org/node/3195234 and https://www.drupal.org/node/3204093 - both of these are very clear.

Left a couple of comments on the PR. Can't usefully review the js, but it looks like the bc layer is pretty comprehensive.

Untagging for release manager review - looks good to go with the minor points above.

Updated CR per #101: https://www.drupal.org/node/3204096
Updated IS to include API changes and Data model changes section.

added a release note

I think that's everything taken care of! Marking this RTBC, thanks.

Heard back from Shepherd maintainer about the dependency evaluation, updated the IS with a link.

Went through all recent commits and all of the changes looked good 👌

OK @lauriii and @alexpott asked me to review this since they didn't feel comfortable committing having both done recent work on the patch.

Did not really do a full review (and can't usefully review JavaScript recently), but found a couple of minor issues (now resolved) and not much else to complain about.

So.. Committed/pushed to 9.3.x and cherry-picked to 9.2.x, thanks!

Not clear why 2 change records are not published but have a lot of mentions in code

- https://www.drupal.org/node/3204093
- https://www.drupal.org/node/3195234

Faced in #3261249: Remove deprecated tour.module functions

I've published the mentioned + 1 CR as D 9.2.0 is long out

- https://www.drupal.org/node/3204096
- https://www.drupal.org/node/3195234
- https://www.drupal.org/node/3204093

@andypost we can do this too ;-)

Hmmm ... there are more draft CR regarding Tour 28-Jun-2016 between 30-Nov-2021 ... anyone?

See https://www.drupal.org/list-changes/drupal/drafts?keywords_description=tour

Maybe we could reopen and repurpose #3261249: Remove deprecated tour.module functions for 9.4.x (property drprecate tour parts)

And use #3195193: Remove Shepherd shim code from Tour to remove the deprecations