Close button in tour module lacks descriptive text

Thanks for the patch keeganstreet!

You will need to modify the "tour.es6.js" instead as per https://www.drupal.org/node/2815083

Hi Apply patch #6 on 8.4.x and its applied successfully.
working fine

Looks good to me

+++ b/core/modules/tour/js/tour.es6.js
@@ -155,7 +155,7 @@
-              link: '<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>',
+              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"Close\">&times;</a>',

+++ b/core/modules/tour/js/tour.js
@@ -76,7 +76,7 @@
-              link: '<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>',
+              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"Close\">&times;</a>',

We need to translate these with Drupal.t()

Here is an updated patch with Drupal.t()

+++ b/core/modules/tour/js/tour.es6.js
@@ -155,7 +155,7 @@
+              link: Drupal.t('<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>'),

+++ b/core/modules/tour/js/tour.js
@@ -76,7 +76,7 @@
+              link: Drupal.t('<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>'),

We lost the aria-label attributes.

We shouldn't be translating the whole string, just the 'Close' text.

So, probably best to evaluate Drupal.t('Close') into a variable, and then use string concatenation + in JavaScript to use the translated text with the existing one.

$ git grep “t(‘Close’”
core/modules/contextual/js/contextual.es6.js:        close: Drupal.t(‘Close’)
core/modules/contextual/js/contextual.js:      close: Drupal.t(‘Close’)
core/modules/quickedit/js/views/EntityToolbarView.es6.js:              label: Drupal.t(‘Close’),
core/modules/quickedit/js/views/EntityToolbarView.js:          label: Drupal.t(‘Close’),

Looked if we have any prior art with context (because "close" could mean "nearby" as well :D) but apparently not, so we can assume people already translate this as “Finish” not “Nearby”.

    toggleTour: function toggleTour() {
      if (this.model.get('isActive')) {
        var $tour = this._getTour();
        this._removeIrrelevantTourItems($tour, this._getDocument());
        var that = this;
		var $close = Drupal.t('Close');
        if ($tour.find('li').length) {
          $tour.joyride({
            autoStart: true,
            postRideCallback: function postRideCallback() {
              that.model.set('isActive', false);
            },

            template: {
              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"' . $close . '\">&times;</a>',
              button: '<a href=\"#\" class=\"button button--primary joyride-next-tip\"></a>'
            }
          });
          this.model.set({ isActive: true, activeTour: $tour });
        }
      } else {
        this.model.get('activeTour').joyride('destroy');
        this.model.set({ isActive: false, activeTour: [] });
      }
    },

added as per #13. Is it good so that I can create the patch

Hi @Dinesh18 in Javscript, use + not . to concatenate.

Also, use close, not $close, as its a plain variable - not a jQuery reference

Thanks larowlan. Here is an updated patch.

Let's clean up the ugly concatenation with ES6 template literals and drop the var.

Modified patch by using template literal and const.

I wasn't able to apply the patch from #22 when I tried. I believe this is because the original files have changed.

I reapplied the changes manually and then tested with a screen reader. The close buttons read correctly when navigating via screen reader and the markup looked correct to me.

I wasn't able to create an interdiff because I couldn't apply the previous patch. The patch itself is unchanged, only the files it changes are different.

This implementation does not pass our eslint coding standards:

/core/modules/tour/js/tour.es6.js
158:21 error Unexpected template string expression no-template-curly-in-string

https://eslint.org/docs/rules/no-template-curly-in-string

- Should be wrapped in backticks to be a valid template string.

• Patch adds descriptive text for screenreader
• Code passes coding standards

Screenshot of DOM for good measure:

I was wondering how did "Close" become "close" between 17 and 22, I did not find commentary about it. As per my earlier grep from above, we consistently use uppercase labels for this (as usual in Drupal). This should be a minor update so we can get this land :) (I would not make this require JS testing personally, that would be shooting a sparrow with a cannon as we say in Hungary).

$ git grep "t('Close'"
core/modules/contextual/js/contextual.es6.js:        close: Drupal.t('Close'),
core/modules/contextual/js/contextual.js:      close: Drupal.t('Close')
core/modules/quickedit/js/views/EntityToolbarView.es6.js:              label: Drupal.t('Close'),
core/modules/quickedit/js/views/EntityToolbarView.js:          label: Drupal.t('Close'),

$ git grep "t('close'"
[no results]

- c -> C

- c -> C
- for real this time.

Feedback from #28 has been addressed in the patch in #30, updating status to RTBC.

Removing 'Needs JS testing' per #28

Adding review credits to everyone whose reviews affected the patch.

Committed and pushed 987e147c0d to 8.6.x and d0af739e12 to 8.5.x. Thanks!

Backported to 8.5.x as a normal bugfix that improves accessibility and is low risk.