Clean up contextual CSS inline with our CSS standards

Added patch.

@lewis you are right the issue image issue is happening with the non-active state. I updated my patch fixing that and also reverting the vendor prefixes.

Updating the patch with the comments mentioned in #4

Guys please don't forget to upload interdiff along with patch. It became easy to other reviewers to review you patch.

@lewis Can you look into this screenshot .. Don't know about whether is it issue or not :P Can we solve it ?

@Manjit.Singh This is not happening for me, how to reproduce this ? Any specific browser ?

@saki007ster Set contextual toolbar to be active. Check the contextual links in the footer blocks. Screenshot attached.

Updating patch as per #14 comment by lewis :)

Including css and markup changes.

add interdiff

Alright, this is looking better and better. (Lets not worry about the failing test just yet, it is because of the html-changes.)

Some thoughts:
1) In #18, I see that we no longer use the .is-open-class ? Don't think this is intentional? Lets double-check that we use one class in js and css.
2) Is there a possibility that we change the following:
.contextual__region .contextual .contextual__links with
.contextual__region .contextual__links
(The whole point is to not overspecify, so in this scenario the class can maybe be deleted (haven't tested it...))
3) In the same way: can't we change: .toolbar .toolbar-bar .contextual-toolbar-tab by .contextual-toolbar-tab or something similar (css-specificity-issues?)

1) change .isOpen to .open in js files
2) fixed
3) almost fixed, need to overwrite other css styles, left comments in patch

Also i have a question about .contextual__links class. When i applied patch #18 in css and js this class was changed, but in source code i still have .contextual-links class (so js and css didn't work for me). I can't found where .contextual-links generated so i left it as it is now. Could someone explain me why .contextual-links is changed to .contextual__links now.

Hey @Kurt,

The contextual-links-class should no longer be used see #14. So If you still find this class in the markup, this should indeed be changed too. (Sure you cleared the cache after applying the patch? I see in the patch that the class should be changed)

Can you explain what the problem is in this ticket? I am not really finding your comment in css. (You should also avoid adding extra comments in patches)

Thanks for the work!

Fixed the patch so it now applies again, not sure why exactly it was failing, but redid the rejected hunks.

Please review, not very sure everything is intentional?

Thanks for the work so far!

Don't understand why the above failed testing.

typo :(

The test failed because it was expecting the class contextual-links, but this is now changed to .contextual__links.

Ignore #35, wrongly patched...

@MathieuSpil
i meant i applied patch, cleared cache, and still see .contextual-links class in source code, see 2015-06-12_1446.png. Is it proper behaviour or what i'm doing wrong.
About my comments in css, see lines 166, 174, 183 in patch #22

I will have a looksee!

Patch re-roll.

Ok, we have this regression inside bartik because of the removal of the .contextual__region-class that is proposed in #39 So maybe we should check carefully where we can delete this class in CSS without losing necessary specificity.

Also: #42 and #41 still need to be implemented.

Specificity issue with contextual links on hover here.

Re-assigning, because of hangovers.

@Aliyakhan: Thank you, I think the error is caused by the css-specificity issue addressed in #48.

Hmmm, re-applying the patch causes more stuff to go wrong... Won't be needing screenshots for now.

Rerolled

1. +++ b/core/modules/contextual/contextual.module
   @@ -127,7 +127,7 @@ function contextual_preprocess(&$variables, $hook, $info) {
   -    $variables['attributes']['class'][] = 'contextual-region';
   +    $variables['attributes']['class'][] = 'contextual__region';
   

   I think at this point we'll need to retain the old classes too, even if we have no CSS for them, otherwise it will be a BC break for existing sites.

2. +++ b/core/modules/contextual/src/Element/ContextualLinks.php
   @@ -24,7 +24,7 @@ public function getInfo() {
   +      '#attributes' => ['class' => ['contextual__links']],
          '#attached' => [
   
   +++ b/core/modules/contextual/src/Plugin/views/field/ContextualLinks.php
   @@ -72,7 +72,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -    $class = 'contextual-region';
   +    $class = 'contextual__region';
   

   here too

I'm working on a reroll. I'll send it here soon.

Edit.: Sorry, I didn't mean to change the status before sending the patch.

I ended up doing a reroll both for Drupal 9.3 and 10.0
I also addressed @larowlan suggestion on #74

I'll see what I can do to reduce the failing tests

I made some more replacements to see if I get less errors (I know I can test locally, and I did it for some tests, but I was not able to configure JavascriptFunctional tests to work on my Lando environment)

reroll the patch

Working on it

Fixed the phpcs issues with #84

Reroll the patch #87 with Drupal 9.5.x and Fixed custom commands.

Reroll the patch #88 with Drupal 9.5.x and Fixed custom commands.

There are commit check failures in the last patch

I've rerolled the patch #89 on Drupal 9.5.x as It was not getting applied, also addressed the Drupal CS issues of patch.

This will have to go into 10.1.x now. Still doesn't pass commit checks, and @larowlan's comments about backward compatibility in #74 still need addressing.

Submitting a D9 patch, tried to fix the CI errors and the feedback on #74

There was an issue with the above patch, created the patch again

Fixing a small css issue in the above patch

Back to NW because of #83

This will have to go into 10.1.x now. Still doesn't pass commit checks, and @larowlan's comments about backward compatibility in #74 still need addressing.

So rerolling for D9 isn't going to be useful.