Contextual Links Style Update

This is not yet ready from a design side

Removing old Figma links.

Uploading the designs.

Updating issue summary.

This patch implements proposed resolution.

In contextual_links_style-3023322-10.patch, I have missed the background color of the pencil icon and this patch implements the proposed resolution.

Ignore this

Ignore this

This is the interdiff for the patches in comment #10 and #11

@Devipriya Rajamanickam: Drupal Core now supports PostCSS. Claro uses this to compile its files. Check Frontend Developer tools for Drupal core for how to use this.

Gonna update everything.
1 hour.

This looks great, thanks for working on this!

Reviewing the patch I've noticed the element moves depending on the state, and it shouldn't happen: the link should stay on the same position regardless its state. Here's a video showing it:

So moving it back to Needs work to fix this.

Also, adding Needs Accessibility review because I think there's already enough code to review it.

Re #20 :
Honestly i can't reproduce this issue, is it "/node" page ? And tell me please which browser and which version on your side?
But better re-test it, since some code was reworked)

For the requested accessibility review, I tested it with keyboard-only navigation on Safari and Chrome on a Mac and then with Safari with the VoiceOver screen reader. Here's a video of it: https://www.youtube.com/watch?v=CgQNJ49L5r4

The list items are not getting the blue background on keyboard focus. They're getting a green border instead.

Re #24:

Figma is not really clear for me in that case. Well, green border - it is just default state for any focusable elements of claro, so on hover links becomes with blue background, but just on focus - green border, no? And in case if hover + focus = blue background + green border around.

Please provide more information about how it should looks. Thanks!

I added new patch since i started to work on it earlier and with another approach.

So what is done:
1) Added common component `Icon Link` https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system... as separated entity, not strictly related to contextual links.
To check its styles/variations create new node with basic html content from https://gist.github.com/iberdinsky-skilld/74326099e87a1078d1540bac3d113c1c

2) Updated styles for contextual region and menu. I reused styles from dropdowns https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/Drupal-Design-system...

3) Added `core/themes/claro/css/theme/contextual.icons.theme.css` with icons to keep toolbar styles fine.

4) Added js theming function for this icon. But core isn't fine with it, since `AuralView` of contextual module force rewrites content of button. https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/modules/cont...
So i had to change contextual module js a bit just for example. Maybe we need to make followup issue in contextual module because currently Drupal.theme.contextualTrigger function is useless https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/modules/cont...

5) Tested changes in ChromeVox. Seems still fine.

in addition to #28 : 4

we can add rewrite of AuralView inside theme as well to avoid contextual module changes.

Thanks @finnsky for adding Action link component and reusing the dropdown styles! Your strategy to keep components isolated is a really good approach. I would move the work here based on this approach.

@ckrina asked for an accessibility review in #20.

Also, adding Needs Accessibility review because I think there's already enough code to review it.

You don't need to wait for code to do an accessibility review :-)

I don't understand all the designs. The last 2 scenarios don't have labels or explanations, so I'll call them pic-6 and pic-7.

1. In pic-6 and pic-7, the contextual button has a strong blue background. What does that signify?
2. In pic-6 and pic-7, the edit link has a strong blue background. What does that signify?
3. What is actually happening in pic-7? Focus is on the contextual button, as it has a green outline. But if focus is on the button, what is happening to make the edit link have a blue background?

Comments #24 - 25 also query this...

The list items are not getting the blue background on keyboard focus. They're getting a green border instead.

That's desirable, and we should keep it. The green border is the focus indicator, and we're aiming for a consistent focus indicator throughout the theme. (The intention is to avoid the Seven theme's problem of wildly inconsistent focus styles; those are confusing to follow because you don't know what the focus style actually is for a given element.)

I don't know what the blue background in the designs represents. Note that in pic-7 the focus appears to be on the button, so whatever the blue background on the edit link represents, it isn't focus.

Re. #28 - 29:
The patch doesn't apply today, so I haven't manually tested it yet.

we can add rewrite of AuralView inside theme as well to avoid contextual module changes.

Forking the AuralView in Claro will increase the maintenance burden in the long term. The small changes to the module's JS in #28 look OK to me:

• The addition of a nested visually-hidden span seems non-disruptive. It also improves upon the off-screen text-indent method of hiding text (which sometimes doesn't play very well with focus styles in user stylesheets.)
• The change to AuralView is a trivial re-ordering of two steps, which also seems non-disruptive.

It will be worth getting the front-end framework manager's view on this.

(Aside: "AuralView" is a poor name, but that's for another issue.)

Leave the "needs accessibility review" in place please.

1) added small fix in aural view.
2) Fixed SettingsTrayBlockFormTest. It had
- `span` selector inside block
- `seven` theme disabled for checking. But claro has blocks contextual disabled aswell https://git.drupalcode.org/project/drupal/-/blob/8.8.x/core/themes/claro...
3) Fixed patch after stable removing
4) Added small nightwatch test for contextuals. Not sure about its location and tag. Please review.

Tested patch from #33 and result is NOT OK:
1. icon should be round
2. pencil should be white

i think it is related to simplytest environment.
dev review still needed.

Thanks for the review, @andrewmacpherson! Based on your comments I've updated the Claro guide with the focus state so it's easier to point to it in the future with some examples: https://www.drupal.org/docs/8/core/themes/claro-theme/design#s-focus-state.

And also sorry for the screenshot hiding the description/title for each state. I didn't even realized! Here's a new one updating it (replaced it on the summary).

Did a quick review and spotted a few things:

1. Not running into anything reported in #34, that all looks fine. 👌

2. +++ b/core/tests/Drupal/Nightwatch/Tests/modules/contextualTest.js
   @@ -0,0 +1,23 @@
   +module.exports = {
   

   This test is failing on DrupalCI, although it is running fine for me locally. It's not really in the scope of this issue (doesn't even test Claro) so it may be easier to just remove. If there's significant interest in having this, the fastest way is probably adding this to an existing FunctionalJavascript test. That can spare Nightwatch environment headaches.

3. +++ b/core/themes/claro/css/theme/contextual.theme.pcss.css
   @@ -0,0 +1,120 @@
   + * Contextual links.
   + *
   + * The following selectors are heavy to discourage theme overriding.
   

   It looks like this comment is no longer relevant as the selectors aren't heavy like they are in core. It sounds like there may have been a good reason for having them be more specific, so it would be worth seeing if it's better to remove the comment or bring back the longer selectors.

I removed Nightwatch as recommended in the #39 comment.
@KondratievaS Can you check this comment as well. I think we can move the test to the `FunctionalJavascript test`.

Tested patch from #39 and got same result as on #34

Re #40 @KondratievaS , the issues you've posted screenshots for there and in #34 can probably most easily be explained due you your Tugboat (and I'm guessing Simpletest) instance running 8.9. This patch was being done on 9.0 (which I'm actually now changing to 9.1 since this isn't a change that can get in post-beta).

Regarding #39 It shouldn't be necessary to add any tests. The Nightwatch test that was previously there only covered functionality that already has coverage in \Drupal\Tests\contextual\FunctionalJavascript\ContextualLinksTest. Contextual links already have strong test coverage. This patch only impacts styling, so my feeling is there are no functional differences that would require test coverage. I'll wait a few days to see if anyone feels otherwise, but if I don't see anyone advocating for the specific changes in this patch to get test coverage, I'll continue reviewing this with the assumption it does not.

Create patch for the issue. Refer the screen shot.

@kostyashupenko it does not look like you are building from the previous patch as there are only a few changed lines. Perhaps that is your interdiff? It is also not clear what you patch is intending to address - the issues reported in #34 and #40 were due to them applying the patch to an incorrect branch, which was pointed out in #41. The next patch should build from #39.

@HOG could you explain what you changed in #45 and #46,? it's not clear what exactlywas changed. Part of this is due to the interdiff not being what we'd expect.

I'm not sure how you're currently generating the interdiff, but to generate an interdiff that will work best on this issue: While on the branch with the most recent changes git diff (branch with the previous changes) > interdiff_(before)-(after).txt.

Thanks for continuing to work on this issue!

@komalkolekar seems you accidentally removed part of previous work on AuralView fixes and tests

@finnsky please review the new patch.

@komalkolekar hi! Thanks for patching this. But actually I'm not sure why you added that background images. #39 and previous patches adding inline svgs approach, which seems more modern and flexible. Imo background image is a step back.

Patch from #46 still valid and should be reviewed.

I think inlining image is already in progress at

• #3085245: Un-inline SVGs in pcss.css files, add build tool to inline them when compiled

Update: sorry for misunderstanding, about the background-image: url(../../images/core/bebebe/pencil.svg); i think it need to be removed.

Hi komalkolekar, #51 have some issues:

remove newline at the end of files:

• contextual.js
• AuralView.js

Update: You added background-image: url(../../images/core/bebebe/pencil.svg);, pencil icon is inlined in the button DOM element core/themes/claro/js/contextual.es6.js.

<button class="trigger contextual-region__trigger focusable icon-link icon-link--size--small visually-hidden"
    type="button" aria-pressed="false" style="">
    <span class="trigger__text icon-link__text icon-link--size--small visually-hidden">Open configuration options</span>
    <svg class="icon-link__icon" viewBox="0 0 28 28" xmlns="http://www.w3.org/2000/svg">
        <path
            d="M27.3252 3.7535l-3.172-3.17c-.778-.778-2.05-.778-2.828 0l-2.586 2.586 6 6 2.586-2.586c.778-.778.778-2.052 0-2.83z">
        </path>
        <path d="M2.73435 19.1723l5.9996 5.9996L22.7311 11.1747l-5.9996-5.9996L2.73435 19.1723z"></path>
        <path d="M.0512003 27.2195c-.1740003.524.1099997.794.6319997.624l4.002-1.334-3.3-3.292-1.3339997 4.002z"></path>
    </svg>
</button>

Tested patch from #46 for desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK

IE 11:

Safari 13:

I think #46 is the best patch to work from going forward. There's no details on what #49 and #51 intended to do but I see they undo fixes that were added in earlier patches.

1. But actually I'm not sure why you added that background images. #39 and previous patches adding inline svgs approach, which seems more modern and flexible. Imo background image is a step back.

   . The background image url approach is preferable, although I still acknowledge that the current approach has its benefits. The precedent was set for Claro in #3036732: Action link component and #3083657: Devise strategy to address several SVG loading+usage issues (where the pros and cons of many SVG approaches were considered). A postcss build tool for inlining on build will be added in #3085245: Un-inline SVGs in pcss.css files, add build tool to inline them when compiled. Also worth mentioning that providing an SVG in a theme function would likely require several additional maintainer/managers to approve as it introduces a new pattern and seems like something that could get some pushback.

2. If item #1 is addressed, this next one may be easier: Adding theme functions to another module isn't in the scope of this issue and would need to be a separate issue filed to the Contextual component, particularly since it's making changes that extend beyond the addition of the theme function. These changes may not even be necessary the background-image approach is used.

3. +++ b/core/themes/claro/claro.libraries.yml
   @@ -192,6 +192,14 @@ checkbox:
   +contextual-trigger:
   +  version: VERSION
   +  css:
   +    component:
   +      css/components/icon-link.css: {}
   +  js:
   +    js/contextual.js: {}
   +
   

   Since icon-link is abstracted into something that is applicable to more than contextual links, that asset should become its own library that contextual-trigger depends on.

4. +++ b/core/themes/claro/images/core/bebebe/pencil.svg
   @@ -0,0 +1 @@
   +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><g><path fill="#bebebe" d="M14.545 3.042l-1.586-1.585c-.389-.389-1.025-.389-1.414 0l-1.293 1.293 3 3 1.293-1.293c.389-.389.389-1.026 0-1.415z"/><rect fill="#bebebe" x="5.129" y="3.8" transform="matrix(-.707 -.707 .707 -.707 6.189 20.064)" width="4.243" height="9.899"/><path fill="#bebebe" d="M.908 14.775c-.087.262.055.397.316.312l2.001-.667-1.65-1.646-.667 2.001z"/></g></svg>
   diff --git a/core/themes/claro/images/core/ffffff/pencil.svg b/core/themes/claro/images/core/ffffff/pencil.svg
   
   diff --git a/core/themes/claro/images/core/ffffff/pencil.svg b/core/themes/claro/images/core/ffffff/pencil.svg
   new file mode 100644
   
   new file mode 100644
   index 0000000000..229e480913
   
   index 0000000000..229e480913
   --- /dev/null
   
   --- /dev/null
   +++ b/core/themes/claro/images/core/ffffff/pencil.svg
   
   +++ b/core/themes/claro/images/core/ffffff/pencil.svg
   +++ b/core/themes/claro/images/core/ffffff/pencil.svg
   @@ -0,0 +1 @@
   
   @@ -0,0 +1 @@
   +<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"><g><path fill="#ffffff" d="M14.545 3.042l-1.586-1.585c-.389-.389-1.025-.389-1.414 0l-1.293 1.293 3 3 1.293-1.293c.389-.389.389-1.026 0-1.415z"/><rect fill="#ffffff" x="5.129" y="3.8" transform="matrix(-.707 -.707 .707 -.707 6.189 20.064)" width="4.243" height="9.899"/><path fill="#ffffff" d="M.908 14.775c-.087.262.055.397.316.312l2.001-.667-1.65-1.646-.667 2.001z"/></g></svg>
   diff --git a/core/themes/claro/js/contextual.es6.js b/core/themes/claro/js/contextual.es6.js
   
   diff --git a/core/themes/claro/js/contextual.es6.js b/core/themes/claro/js/contextual.es6.js
   new file mode 100644
   
   new file mode 100644
   index 0000000000..2b5dd10b5b
   
   index 0000000000..2b5dd10b5b
   --- /dev/null
   
   --- /dev/null
   +++ b/core/themes/claro/js/contextual.es6.js
   

   There's no need to create icons that already exist in core. This was a temporary workaround when Claro was contrib and it couldn't be reliably counted on to be in a specific path. These can be removed and the background: urls can reference the icons in core/misc. Any new icons can be added to a new images/icons dir that similarly sorts by color.

address #57.

1. replace current approach with background image
2. revert contextual module modifs
3. move icon-link into it's own library
4. move icons inside core/misc

Nice quick turnaround on the feedback in #57 @boulaffasae! One thing I noticed is that the system tray changes are still there. Those can be reverted as well as they were only necessary because of the changes in the contextual module.

The mouse over effect doesn't get the mockup effect (blue re-salted)

Unless I'm misunderstanding what is being pointed out in #62, the hover behavior in the attached .gif appears to match the designs. Hovering over the icon or list items changes the background to a pale blue. If I'm missing something, perhaps you can detail exactly where a problem was found.

Here's some additional things I found on review:

1. +++ b/core/themes/claro/css/components/icon-link.css
   @@ -0,0 +1,140 @@
   +.icon-link {
   +  display: flex;
   +  padding: 0;
   +  border: 1px solid #d4d4d8;
   +  border-radius: 50%;
   +  background-color: #fff;
   +}
   +
   +.icon-link:hover {
   +  border-color: rgba(212, 212, 218, 0.8);
   +  background-color: #f0f5fd;
   +}
   +
   +.icon-link:focus {
   +  box-shadow: 0 0 0 1.5px #fff, 0 0 0 3.5px #26a769;
   +}
   +
   +.icon-link:active,
   +.open > .icon-link {
   +  border-color: #003cc5;
   +  background-color: #003cc5;
   +}
   ...
   +.icon-link--size--small:focus {
   ...
   +}
   

   These are the only rules in the file that impact elements that exist in Claro. Everything else is hypothetical and unrelated to Contextual links. The styles that are not applicable to contextual should be removed from the patch and dealt with in its own issue where every style variation can be reviewed and approved.

2. +++ b/core/themes/claro/css/theme/contextual.icons.theme.pcss.css
   @@ -0,0 +1,36 @@
   +.open > .icon-link .icon-link__icon { */
   

   This commented bit should be removed

3. +.icon-link--size--small:focus {
   ...
   +}
   

   For BEM, .icon-link--size--small should be changed to .icon-link--size-small, in the CSS and the JS that adds this class

4. +++ b/core/themes/claro/css/theme/contextual.icons.theme.pcss.css
   @@ -0,0 +1,36 @@
   +/**
   + * Toolbar tab icon.
   + */
   +.toolbar-bar .toolbar-icon-edit:before {
   +  background-image: url(../../../../misc/icons/bebebe/pencil.svg);
   +}
   +.toolbar-bar .toolbar-icon-edit:active:before,
   +.toolbar-bar .toolbar-icon-edit.is-active:before {
   +  background-image: url(../../../../misc/icons/ffffff/pencil.svg);
   +}
   

   These styles should be removed, they are in the scope of a different issue: #3020422: Toolbar style update

5. +++ b/core/themes/claro/css/theme/contextual.icons.theme.pcss.css
   @@ -0,0 +1,36 @@
   +  height: 2rem !important;
   

   It is best to avoid using !important, but if it's necessary it should have a comment explaining why it is needed. I removed the !important on my local and did not have a problem, but perhaps there's a specific use case this is needed for? If there is, that's what should be documented. If not, then remove the !important

6. +++ b/core/themes/claro/css/theme/contextual.theme.pcss.css
   @@ -0,0 +1,118 @@
   +.contextual .trigger {
   +  position: relative;
   +  right: 0; /* LTR */
   +  float: right; /* LTR */
   +  overflow: hidden;
   +  cursor: pointer;
   +}
   +[dir="rtl"] .contextual .trigger {
   +  right: auto;
   +  left: 0;
   +  float: left;
   +}
   

   The right: and left: rules here aren't needed because the position is relative and not changed in other styles, so it's just declaring a style that is already present. Avoiding float: would also be nice, which can be done be replacing float: left; with margin-right: auto; (and vice versa for RTL)

7. +++ b/core/themes/claro/css/theme/contextual.theme.pcss.css
   @@ -0,0 +1,118 @@
   +.contextual-links {
   +  position: relative;
   +  top: 0.125rem; /* 2px */
   +  right: 0; /* LTR */
   +  float: right; /* LTR */
   +  clear: both;
   +  margin: 0;
   +  text-align: left; /* LTR */
   +  white-space: nowrap;
   +  border: 1px solid var(--contextual-links-border-color);
   +  border-radius: var(--contextual-links-border-radius);
   +  background-color: var(--color-bg);
   +  box-shadow: 0 2px 4px var(--contextual-links-shadow-color);
   +}
   +[dir="rtl"] .contextual-links {
   +  right: auto;
   +  left: 0;
   +  float: left;
   +  text-align: right;
   +}
   

   This would require some manual testing, but it doesn't look like it's necessary to use float:, right:, left: or clear: here. This could potentially be needed if the contents of .contextual-links were narrower than the trigger icon, but that would not happen as the padding alone makes it wider.

8. +++ b/core/themes/claro/js/contextual.es6.js
   @@ -0,0 +1,15 @@
   +   * Constucts a contextual trigger element.
   

   s/Constucts/Constructs

#63
@bnjmnm
--size--small is a valid BEM "--mod-name--mod-val"
https://en.bem.info/methodology/naming-convention/#two-dashes-style

@bnjmnm
I fixed your feedbacks in #63
About icon-link component: It's part of contextual component, why not to add this component in these tasks?

1. Re: #64
   

   --size--small is a valid BEM "--mod-name--mod-val"
   https://en.bem.info/methodology/naming-convention/#two-dashes-style

   Good point, this doesn't deviate from BEM. It does, however, deviate from Drupal's conventions. I think this should still change so it's consistent with the similar CSS in Drupal core, such as buttons. The only place that style is used in core (based on a grep of css for \..*--.*-- is layouts.

2. Re: #66

   About icon-link component: It's part of contextual component, why not to add this component in these tasks?

   There are 14 CSS rules in that file. There is no way to review/test most of those rules because the selectors do not seem to be targeting anything that currently exists in Drupal core (happy to be proven wrong). Either those rules need to be removed, or there needs to be some means of testing them. There are currently no primary or large icon-links, and selectors like .icon-link--primary .icon-link__icon may not even reflect how icon links will be structured.

   The rules in icon-link.pcss.css that target something that exists in Claro can definitely stay. The rest can either be removed or we'd need examples of how to find them in use so they can be reviewed.

Discussed this on the admin UI weekly meeting today with @ckrina, @bnjmnm, @nod_, @saschaeggi and Gábor Hojtsy. On the call we agreed that we should implement the contextual links tray based on the dropbutton tray implementation for consistency. We should also open a follow-up for discussing whether we should use the Absolute Zero blue in future for some of the states in the tray for both, contextual links and dropbuttons.

Guess follow-up exists

This patch changes the links styling to match the dropbutton tray, as mentioned in #68

Also, per #67, a classname was changed to match Drupal core's BEM conventions, and several CSS rules were removed as no elements exist for them to style and wouldn't be testable/reviewable as a result.

Overall this looks great. Just a few points.

1. +++ b/core/themes/claro/css/theme/contextual.theme.pcss.css
   @@ -0,0 +1,120 @@
   +.contextual .trigger {
   ...
   +[dir="rtl"] .contextual .trigger {
   ...
   +.contextual.open .trigger {
   

   Maybe we could add a new contetual__trigger class to clean up this?

2. We might have to wait till we have decided on an approach in #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future because I think this change too should be applied on the front-end theme too.

Nice, #71.1 made it possible to reduce the specificity of several rules!

Regarding #2, unlike Toolbar, I'm not entirely convinced this would need to wait on #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future, because:

• Contextual links are not used in admin contexts that much (Layout builder uses the FE theme), so there would only be an inconsistency if Claro was the default and admin theme
• Even if there were differences, it would be less jarring than Toolbar, which is in a fixed position and is mentally imprinted in many users workflows. There's already an expectation that contextual links will be different (position, link) depending on the page.

I can still understand if the above doesn't supersede consistency concerns, but figured I'd mention it.

I agree that the consistency aspect isn't as critical here as it is on #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future. I think we need at least a follow-up for implementing the contextual links designs in the contextual links module. The reason I'm not sure whether we should do it here or not is that since contextual links are not used in the admin UI that often, it might not make sense to implement the contextual links in Claro without also implementing them in the contextual links modules.

Postponed on #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future. Any further steps require knowing how these styles will be applied on front end themes, which that issue will decide.

While we're working on an update to contextual links, we should ensure that the icons have correct contrast in Windows High Contrast mode. In the current patch link and toolbar icons can't be seen in IE11, and don't meet contrast requirements in Windows High Contrast mode.

Implements the default theme styling based on the pattern from #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future and adds test coverage for it.

Added support for icons in high contrast mode. This required overriding AuralView's render method. This can be simplified to just a theme function override once #3172956: Add theme function to process contextual trigger text. lands.

Also cleans up several unnecessary rules.

Note that the interdiff is based on a reroll of #72 that reflects the addition of #3171366: Comments from variables.pcss.css create nonuseful noise in compiled css, so the patch/interdiff sizes may not appear to line up, but they actually do.

The previous reroll had problems. Here is a different reroll.

1. 
   I assume that the icon-link is not supposed to look like this when contextual link is focused without hovering the region.
2. 
   The contextual links tray could be rendered on top of other elements such as images. For example in this case it seems like the focus effect is difficult to distinguish from the background.

3. 
   

   Some Bartik and Umami styles override contextual links styles. We will have to decide how to handle these.

Moving to needs work since #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future was committed. Removing needs frontend framework manager review tag because the approach was already reviewed in that issue.

Re-Rolled #79.

The reroll in #83 had two instances of a function being declared twice. A quick manual test after rerolling usually surfaces these if they somehow get past git.

Reroll based on #78, which appears to address #80 (I suspect those issues were part of a flawed reroll in #79)

Argh

Updated:

• Fixed "patch does not apply".
• Centering pen
• Remove padding-left for <ul>

We have a probleme with the focus

Too many files 🥶

Hello,

I've applied the patch in #88 and it applied cleanly. But found some issues with those. Attaching the screenshots below,

After the patch,

1. I see additional padding to the left of each menu items.



2. Also, icon-link is not supposed to look like this when contextual link is focused without hovering the region. It's also mentioned in #80

I think it needs work!

Thanks @sulfikar_s! This takes care of the padding, but I'm not able to reproduce the focus issue, this is how focus appears with or without hovering:

Something that could help: First, confirm the testing of this patch is happening on the Drupal 9.2 branch. This requires the changes in 9.2 to fully work correctly.

If this is definitely happening on 9.2, this would help narrow down the issue: Inspect a contextual link with this issue to identify the styles/css file causing the problem. Focus is either adding a bad style or removing a good one. If I know where that's happening on other users sites, I should be able to figure out why it's not happening on mine.

Hello @bnjmnm, I've tested the last 2 patches of your's on Drupal 9.2.x-dev. I've applied your new patch and it applied cleanly even though it failed the testing!

I'm attaching a video where you can see the issue I mentioned in my previous comment

Also, icon-link is not supposed to look like this when contextual link is focused without hovering the region. It's also mentioned in #80

I've also addressed your suggestions in that video.

The video in #92 provided exactly the info I needed, thanks!
This should take care of it, and the test failure in #90 looks to be an unrelated test, so I think things will be in good shape again(?)

Hi Everyone,

I found some issues:

1. Conflict between :

         .contextual-links a {
           color: #545560;
         }
         .region-header a {
           color: #fffeff;
         }
         // The two declarations have equal selector type counts, but the .region-header a selector is declared last.
       

2. @TODO: When the space is not enough for displaying the dropdown, it shows up top or right if necessary to get enough space.

The dropdown must shows up right to get enough space :

Hello,

I fixed issues found at #95

1. When the space is not enough for displaying the dropdown, it shows up top or
   right if necessary to get enough space.

           // Is offset a negative number
           +if (this.$el.find('.contextual-links').offset().left < 0)
           +  this.$el.find('.contextual-links').addClass("left")
           // RTL, Is it bigger than window width
           +else if (this.$el.find('.contextual-links').width() + this.$el.find('.contextual-links').offset().left > $(window).width())
           +  this.$el.find('.contextual-links').addClass("right")
           // LTR
           +.contextual-links.left {
           +  left: 100%;
           +}
           // RTL
           +[dir="rtl"] .contextual-links.right {
           +  right: 100%;
           +}
       

2. Conflict between Bartik theme and Claro theme

           // Default
           -  color: #545560;
           +  color: #545560 !important;
           // Hover
           -  color: #222330;
           +  color: #222330 !important;
           // Focus & Active
           -  color: #545560;
           +  color: #545560 !important;
       

1. +++ b/core/themes/claro/css/theme/contextual.theme.css
   @@ -64,19 +64,27 @@
   +.contextual-links.left {
   +  left: 100%;
   +}
   +
    [dir="rtl"] .contextual-links {
      right: auto;
      left: 0;
      float: left;
    }
    
   +[dir="rtl"] .contextual-links.right {
   +  right: 100%;
   +}
   +
   
   +++ b/core/themes/claro/js/contextual.es6.js
   @@ -13,6 +13,11 @@
   +      if (this.$el.find('.contextual-links').offset().left < 0)
   +        this.$el.find('.contextual-links').addClass("left")
   +      else if (this.$el.find('.contextual-links').width() + this.$el.find('.contextual-links').offset().left > $(window).width())
   +        this.$el.find('.contextual-links').addClass("right")
   +
   

   It's great that you're approaching this with such attention to detail, but these particular changes are out of scope for this issue. Contextual links items flowing outside of the viewport is not unique to these Claro styles. It's something that happens in all of Drupal core. If this is a problem you feel is worth addressing, it should be in a separate issue that attempts to fix it for all of core.

   If you want to try fixing this in another issue, Drupal core already has a great library for positioning: core/popperjs, and that should be used instead of custom logic.

2. +++ b/core/themes/claro/css/theme/contextual.theme.css
   @@ -64,19 +64,27 @@
   -  color: #545560;
   +  color: #545560 !important;
   
   @@ -102,7 +110,7 @@
   -  color: #222330;
   +  color: #222330 !important;
   

   Using !important should be avoided whenever possible, and I suspect there are other ways to get the same results differently , such as increasing specificity by doubling the .contextual-links selector. When a situation arises where !important appears to be the only option, it should be accompanied by a comment explaining why

Hi bnjmnm,

I combined ul.contextual-links with the div.contextual above :

-.contextual-links a {
+.contextual .contextual-links a {

And reverted the code that address the Contextual links items flowing outside of the viewport issue

Adding NorthAmerica2021 and Easy Out of the Box tags for visibility.

DrupalCon NA is April 12-16 with a focus on EOOTB on Wednesday, April 14.
Thanks

After DrupalCon, I'll remove the Easy Out of the Box, thanks!

After review, this issue looks good to me. Works and the code looks fine.

@vacho thank you for taking the time to review this! In order to help the core committers (so they don't have to test&review the whole patch, or at least help them a bit) it would be great if you could:

• Post some screenshots to showcase the patch working, ideally with different browsers and/or viewport sizes.
• Confirm requests asked at #3023322: Contextual Links Style Update have been adressed.

Re-rolled the patch and opened a MR as @Cristina Chumillas mentioned in the easy-out-of-the-box meeting.

Hi! I tested this issue on Firefox, Chrome, Safari and Internet Explorer 11 and can say that it matches the Figma and the links are accessible both with the mouse and the keyboard. The only detail to mention is that the "pencil" icon can be triggered with the spacebar or enter key while the contextual links ("edit view" in this case) are triggered only by the enter key since it's a regular link and not a button. But maybe this is a separate issue.

Regarding Safari on Mac, I had to enable two settings, one at OS level and a Safari option as well for the tab key to focus on links, it's described in this article. If the contextual links were buttons instead, it would work no matter the settings I think.

Below are screenshots of the different browsers:

Chrome keyboard

Chrome mouse

Firefox keyboard

Safari keyboard without enabling OS / safari settings

Safari keyboard enabling OS / safari settings

Patch rerolled to 9.3.x version

@vacho, that rerolled patch was not necessary. Comment #106 has a merge request that applies against 9.3 without issue

Thanks everybody for all the work here! I've reviewed the MR and left a few CSS feedbacks.

This also still needs to solve @bnjmnm's feedback from #98 (point 2) where he asks to avoid using so many !important in the code (and suggests a way to avoid it).

Using !important should be avoided whenever possible, and I suspect there are other ways to get the same results differently , such as increasing specificity by doubling the .contextual-links selector. When a situation arises where !important appears to be the only option, it should be accompanied by a comment explaining why

The MR/patch still has a lot of !important, so it should be addressed too.

I also think that several of the suggestions from #23 have been addressed. And because the code has changed so much from then, a new accessibility review might be good. Specially based on #108 comments.

@ckrina Reviewed the variables and updated the MR

Tagging for Design4Drupal 2021. Contributions are Friday, July 22
https://design4drupal.org/

Correcting tag Design4Drupal2021

+@media screen and (-ms-high-contrast: active) {
+  .contextual__trigger::after {
+    content: url(../../images/icons/ffffff/pencil.svg);
+  }
+  .contextual__trigger:active,
+  .open > .contextual__trigger {
+    background-color: var(--color-white);
+  }
+  .contextual__trigger:active::after,
+  .open > .contextual__trigger::after {
+    content: url(../../images/icons/000000/pencil.svg);
+  }

Please don't specify any actual colours inside a high-contrast/forced colours media query.

Setting an explicit black/white icon and/or background is missing the point of a forced-colours mode. The colours are supposed to be the user's choice, not the author's. Users can devise any custom colour scheme they please; mine is a lovely yellow-on-maroon today. So unless black and white are essential to understanding what the button is for (they aren't), don't set these black and white colours.

Note that "Windows high contrast" is a misnomer. Some people use it to achieve a low contrast colour scheme. If a user needs sepia-on-beige because of photosensitivity, then forcing a bright white button background is a bit aggressive.

Use system colour keywords - fill="ButtonText" for the icon in this case, since it's used on a <button>.

Please add a forced-colors media query to do the same for modern browsers which follow W3C CSS. Between forced-colors:active and -ms-high-contrast:active we can reach all of our supported browsers on Windows. (Or at least we will when the next Firefox ESR comes out soon.)

+  .contextual__trigger:active,
+  .open > .contextual__trigger {

Remind me please, what are the different situations which these two selectors represent?

I don't think there's any need for swapping the colours around when the list is open. It's apparent from the fact that you can see the list contents.

Please don't specify any actual colours inside a high-contrast/forced colours media query.

Setting an explicit black/white icon and/or background is missing the point of a forced-colours mode.

Thanks for that explanation. Very educational. In addition to the reasons noted, I also found that the styles present for -ms-high-contrast mean that the pencil icon isn't visible in the active or open state for "High Contrast Black" theme (the default) in Windows:

Of course, it could actually work well depending on the users color scheme choices.

Without any -ms-high-contrast styles for the .contextual__trigger class, you end up slightly better off at least for "High Contrast Black" theme in Windows:

Ideally the icon would have color applied in the default styles via fill and/or stroke CSS properties, and then get adjusted for state (:active, :focus, etc.). That would allow us to make any necessary adjustments in MS high contrast / forced-colors by adjusting fill and/or stroke using CSS system colors. However, the SVG use here is applied in CSS using a data URI which doesn't allow adjustment to fill or stroke as far as I'm aware :/.

I'll push a commit to remove the relevant -ms-high-contrast styles for folks to look at. Beyond that, not sure how to resolve ^.

Remind me please, what are the different situations which these two selectors represent?

The first is the active state of the pencil icon, eg. when a user presses their mouse button. The later is the open state of the contextual links, eg. after the user finishes their button press and the contextual links are visible. It remains in open state until the user removes focus.

On second though, I was inspired by #3171726: Claro Shortcuts star fails WCAG Use of Color and Non-text contrast to add the high contrast styles in a style tag inside the SVG files. This should do nicely for high contrast mode I think.

Chrome, Contextual links open:

Chrome, Contextual links open on hover:

Edge, Contextual links open:

Edge, Contextual links open, hover:

Adding tag for DrupalCon Europe2021. Thanks

Patch won't apply

Either we need to change the base branch of https://git.drupalcode.org/project/drupal/-/merge_requests/829 or we need to review newly created MR - https://git.drupalcode.org/project/drupal/-/merge_requests/1960

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.

Imo it should now definitely use floating UI lib. Which now in core.

MR has failures.

Did not review or test

Removing stable blocker because claro is marked stable
Removing NorthAmerica2021, Design4Drupal2021, Europe2021 as previous events this was worked on

#134 should be considered too or mentioned why not I think.

#3023322-134: Contextual Links Style Update We cannot use floating UI lib, that is a functionality change and this is specifically for styling.

Tests are passing now. Ready for review.

Added some comments. Take a look.

Also please. Why MR modifies core/misc/jquery.form.js ?

Hi, I have addressed all the feedbacks but I am not sure about the style that was added to the SVG, I removed those for now and added stroke attribute, it is working same. So is that fine, or am I missing something here?

Can the issue summary be updated using the standard issue template please. As a UI fix would be super useful to have before/after screenshots as part of the summary.

Wonder if the "Needs followup" tag can be removed.

There's an open thread around the new function extend_library() which I believe was being used to deprecate a library but we have a mechanism for that now.

Thanks!

I still see imported variables in
core/themes/claro/css/theme/contextual.icons.theme.css
core/themes/claro/css/theme/contextual.theme.css

and some unexpected changes in core/themes/claro/css/theme/media-library.css

Addressed all the feedbacks in #145.
Regarding

and some unexpected changes in core/themes/claro/css/theme/media-library.css

the changes are made to make the pencil visible in high contrast setting of the browser.Before that it was styled using inline css.
For now I have removed the use of stroke and used fill.

Attaching the screenshots of the state when the stroke/fill is not used

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

As a UI change can the User section of the issue summary template be added back and screenshots be included.

Added standard IS template.