Better shortcut action link styles when there's limited horizontal space

Postponing this until the design is done.

Popper seems to work pretty well, particularly if one considers that hover and keyboard navigation is much less likely to occur at narrow widths, so it probably doesn't need to look amazing.

The JS is using regular classes as adding data attributes or js-prefixed won't be trivial - it's already formatted markup by the time it reaches claro and rebuilding it would necessitate a require_once of shortcut.module. If this approach looks good to others, that step can certainly happen.

@bnjmnm : After adding the patch #8, I have tested the shortcut icon on multiple pages. At the time of edit node, it appears in the next line.

Looks like the compiled JS was not in the previous patch, just the es6 file. That would explain #9

#10 is working fine on 9.1
This can be moved to RTBC

In the narrow screen shot in #8, the "Add to..." message partially obscures the primary local actions.

Does it satisfy WCAG SC 1.4.13 "Content on Hover or Focus"?

#8:

particularly if one considers that hover and keyboard navigation is much less likely to occur at narrow widths

That's just not true. Besides which, the likelihood doesn't matter; some users will employ a hover-pointer or keyboard.

Using a narrow-ish window and browser zoom, you can soon get to these proportions on a desktop browser. A typical situation would be 2 windows side by side with half the monitor width each (which nearly all desktop OS provides a keyboard shortcut for). On a 1920x1080 HD monitor, that amounts to 960 physical pixels, or 480 CSS pixels at 200% browser zoom.

There's also no accounting for the length of the page title, which isn't anything to do with the viewport size. With a longer page title, this situation will happen on a 1280px viewport.

patch #2 applied cleanly and working as expected , sharing screenshot for reference.

Applied patch #19 and it works fine.

Before patch:

After patch:

RTBC +1

Verified and tested patch #19 on Drupal 9.5.x-dev. The patch is applied successfully and looks good to me.

Testing steps:
1. Goto admin/Appearance
2. Set Claro theme in the Administration theme (Scroll down and see)
3. Save configuration
4. Observe Extend tab or any tab whichever you want
5. Check the shortcut link star mark by reducing horizontal space or with a mobile view
6. Observe the shortcut link star mark moves down when horizontal space is reduced
7. Apply the patch
8. Reload the page. (Note: Clear all caches if required)
9. Now observe the shortcut link star mark

Testing Result:
1. After applying the patch the shortcut link star mark is displayed properly even after horizontal space is reduced

Screenshots are attached for the reference
Hence moved to RTBC

#22 has a detailed list of steps taken (good!) but it doesn't mention reviewing the actual code. That should happen before any RTBC.

1. +++ b/core/themes/claro/css/components/shortcut.pcss.css
   @@ -45,6 +45,7 @@
   +  z-index: 1;
   

   Is this actually needed if Popper is helping? Not sure one way or the other.

2. +++ b/core/themes/claro/js/shortcut.es6.js
   @@ -0,0 +1,40 @@
   +      const $shortcuts = $(context).once('shortcuts').find('.shortcut-action');
   

   jQuery once is deprecated. Use drupal/once instead.

3. +++ b/core/themes/claro/js/shortcut.es6.js
   @@ -0,0 +1,40 @@
   +        const message = element.querySelector('.shortcut-action__message');
   

   It feels like overkill for a little shortcut tooltip, but ideally we'd use data attributes as classes should be for styling only. JavaScript needs to target selectors specific to functionality.

4. +++ b/core/themes/claro/js/shortcut.es6.js
   @@ -0,0 +1,40 @@
   +})(jQuery, Popper);
   

   When jquery once is refactored out, jQuery won't be needed either.

#24 Please review the patch

Nice! This can be refined a bit more, but I like the direction this is going in.

+++ 3068698-24.patch	2022-08-29 15:45:07.000000000 +0530
@@ -61,7 +57,7 @@
++      const $shortcuts = $(once('shortcuts', '.shortcut-action', context));

This is unnecessarily wrapping once in jQuery

you can just do

once('shortcuts', '.shortcut-action', context).forEach((element) => ....)

and remove the jQuery / $ from the file entirely.

#26 Please review the patch

Applied patch #26 and it works fine.

Patch #26 tested and applied successfully on Drupal 9.5.x.

Before Patch:

After Patch:

I have verified the patch #26 and tested it on Drupal version 9.4.x. The patch works fine. I have added the before and after screenshots for reference.

Verified and tested patch#25 on the Drupal 9.5.x-dev version. Patch applied successfully.

Looks like a WCAG 1.4.4 issue.

Re-rolled #26, for Drupal 10. As es6 files are no longer used in D10. Also popperjs has been updated to loadjs. Attached a patch and interdiff for same. Please review.

I will double check this issue

1. +++ b/core/themes/claro/claro.libraries.yml
   @@ -202,6 +202,11 @@ autocomplete:
   +    - core/loadjs
   

   I don't believe loadjs is needed anywhere in this fix so this doesn't need to be added as a dependency

2. +++ b/core/themes/claro/js/shortcut.js
   @@ -0,0 +1,39 @@
   +/**
   + * DO NOT EDIT THIS FILE.
   + * See the following change record for more information,
   + * https://www.drupal.org/node/2815083
   + * @preserve
   + **/
   

   In other words, use code from shortcut.es6.js from the earlier Drupal 9 patch in this shortcut.js file. This is a copy of the transpiled JS, and Drupal 10 no longer transpiles its JS (hence no .es6.js + .js files)

Made changes as pointed out in #38, thanks!

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

There are issues found in #40 that have not been resolved in the meantime, is there a reason why you'd just ignore that update @Guru2023?

The PopperJS library has been removed from the core 10.0.0
core/popperjs asset library is deprecated

The Claro theme was approved for removal in #3576460: [policy, no patch] Deprecate and remove Claro.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3576668: [meta] Tasks to deprecate Claro and the removal work in #3584638: [meta] Tasks to remove the Claro theme.