Change contextual links background color to match the styleguide.

Looks good to me.

Thanks :-) This was bothering me to. RTBC++

Contextual.module's CSS is front-end CSS.

Seven = back-end theme.

So applying the Seven style guide to Contextual's CSS … we violate this separation.

IMHO we should apply the same technique that ckeditor.module has introduced to Drupal: allow (front-end) themes to define a CSS file to be used in CKEditor when it's in iframe mode, so that you can see what it'd look like on the front-end:

ckeditor_stylesheets:
  - css/ckeditor-iframe.css

(in a theme's .info.yml file)

We could then allow back-end themes to define

contextual_stylesheets:
  - css/contextual.css

@Wim This to me is very gray area, because the current styling is not in line with anything else either. I think we are all fine, with the idea of separating the styling like you did with ckeditor, but to me this feels like a separate issue. This issue focuses on changing the default, that can be in line with Seven - I don't see anything wrong with that, as long as its a default that isn't highly opinionated and easy overwritable per theme, which I thought contextual links already was.

I think it's important that we achieve a consistent admin user experience, not just a consistent theme. We already have some new "Seven styling" that bleeds through into the front-end. Messages, overlay, and the toolbar are some examples.

Maybe there is a better way to achieve this technically but in my eyes it makes sense to have this bleed through of styles for administrative components.

#6: First: that is utterly incorrect. Stark is a front-end theme. Second: every site has an admin theme. I'm saying that it's up to the back-end theme to style contextual links. Third: no, I'm not saying it should continue to look like this, I'm saying that contextual.theme.css should be removed altogether, and it should be moved into Seven.

#7: Yes, it is a gray area, and precisely for that reason it is so problematic. The way it is currently implemented, it is impossible for another admin theme to make these "admin tools on the front-end" look in line with the back-end.
I do see the case for making that a separate issue, but at the same time, this issue is the first time I've explicitly seen "Let's style front-end things according to a back-end theme's style guide", which means we're explicitly, willingly violating the separation between front- and back-end themes. That's why I'm advocating we should start doing this in a better way in this issue.

#8: I agree with the bleed through. I'm not arguing against that anywhere. I'm just saying we're doing it wrong here. We're making it hard for anybody else out there to replace the back-end theme and again have that bleed through to the front-end.

Implemented #5:

• Admin themes can now specify contextual_stylesheets, and Seven does so (based on the patch in #0).
• This is achieved by letting contextual.module implement hook_library_info_alter(), to alter its own library in case an admin theme implements contextual_stylesheets. An admin theme cannot achieve this on its own because it is completely ignored on front-end pages.
• (This is completely inspired by how quicksketch achieved this for CKEditor, and while implementing it I noticed that param docs were missing, so fixed that.)
• (The before vs. after screenshot in #0 showed that the resulting styling of "on hover" in current HEAD is broken, because of an inappropriate !important elsewhere in the CSS, so removed that.)

The end result looks identical to #0/#1, but the difference is that any admin theme can now tune Contextual Links to match! :)

And then I forget the patch :P Stupid!

Whoo. Well happy we found an implementation that works, this probably needs some review/feedback from people who understand this - instead of me :)

… and I still forgot to add Seven's new contextual.css file.

+++ b/core/modules/contextual/contextual.module
@@ -357,3 +373,42 @@ function _contextual_id_to_links($id) {
+    $theme = Drupal::config('system.theme')->get('admin');
+  }
+  if ($theme_path = drupal_get_path('theme', $theme)) {

This patch ignores any other enabled themes. It's really easy to have multiple themes in your site (and also really easy to have no 'admin' theme btw, but let's keep that aside).

So, we should inspect all enabled themes and store the css and not focus on the admin them

If this is the same behavior that ckeditor does, then we actually have a bug there.

#13:

before (i.e. without Seven theme) vs. after (i.e. with Seven theme)

I don't think many themes want to style contextual links differently but it's nice we can do it easily now.

Absolutely! But the point is that it is possible at all, whereas right now it is impossible :)

aren't we duplicting most of the code in "_contextual_theme_css"?

Yes, a large part is the same. But not everything. There's little code to abstract. I will add a third case in #2080217: Polish entity toolbar visually, so I'll try to abstract it there.

#14: I think you're missing the point :)

If this is the same behavior that ckeditor does, then we actually have a bug there.

CKEditor uses this whenever it's in "iframe mode", which is usually on the node/x/edit form. It is impossible to determine which theme is going to be applied to the node you're editing when it's displayed on the front-end. The best thing we can do, is make it look like it would in the default front-end theme. So that's what's happening in the CKEditor case.
Am I missing something?

we should inspect all enabled themes and store the css and not focus on the admin theme

No. That would be flawed in two significant ways:

1. It does not make any sense to add the theme-specific contextual links styling of *every* available enabled theme. You'd end up with *all* styles being applied to the contextual links. How could that even work?
2. The whole point is that contextual links are an "admin thing on the front-end", hence the admin theme should be applied. So the code in the current patch is good to go IMHO.

Again, am I missing something?

The whole point is that contextual links are an "admin thing on the front-end", hence the admin theme should be applied. So the code in the current patch is good to go IMHO.

And that explains the current flaw of the patch: you're not sure that the user has access to the admin theme either.

Assigning to Wim for feedback.

Are we possibly over complicating the issue? We currently define a hover colour in contextual.theme.css and all we really need to do is chance it. We could open up another issue to figure out how to deal with admin elements appearing on the front-end, which we have all over the place right now.

#18: I don't think so. From #9:

I do see the case for making that a separate issue, but at the same time, this issue is the first time I've explicitly seen "Let's style front-end things according to a back-end theme's style guide", which means we're explicitly, willingly violating the separation between front- and back-end themes. That's why I'm advocating we should start doing this in a better way in this issue.

If people feel strongly that shouldn't be the case, then I'll accept that, even though it's inherently incorrect. But then it's up to you guys to rectify this — I doubt we'll ever get sufficient reviews for a separate issue to fix this.

#16: Interesting point :) I think you're referring to the "view the administration theme", which I didn't even know existed :D
I was going to say: "but if you go to node/add/article as a user who only has the permission to create article content, you will get to see the administration theme even if you don't have that permission?" — which turns out to be wrong. You continue to see the front-end theme, but things are horribly broken in the filter guidelines and vertical tabs: see the attached screenshot.

My point being: that's theoretically true, but I think that in practice anybody who may access contextual links will also be allowed to view the administration theme. I think it's an assumed dependency in practice.

Back to Bojhan.

The implementation of this is a little beyond me, I will go with whatever you guys settle on :) But as far as I know, is that permission "view administrative theme" not required to get the CSS, and is it reasonable to expect people have access to it - when they see contextual links

I'm going to assume this needs a reroll. Promise I'll get someone to review promptly.

12: contextual_links_seven-2078803-11.patch queued for re-testing.

Working on a reroll.

Reroll attached.

@Wim Leers I'd really like to fix this but don't want to continue down the road of every other module defining an admin stylesheet. In https://www.drupal.org/node/2195695#comment-9037233 I suggested an alternative. How would you feel about implementing that here?

Replied there! :)

Can we turn this issue back into a simple color swap and leave the other issue to sort out the best way to handle this?

Yes, let's do that. Sorry for holding this up for that reason — it was with the best intentions! :(

add patch for contextual links background

Yes, let's do that. Sorry for holding this up for that reason — it was with the best intentions! :(

Of course! I don't doubt your intentions :)

+++ b/core/modules/contextual/css/contextual.theme.css
@@ -107,7 +107,6 @@
+  color: #000;
+  background: #d8d8d8;  ¶

There are two trailing spaces here, also I think the correct color to use is #f7fcff which is the color we use on the to hover on the table rows in Seven.
I think that means we can also remove the color change to white, because the background color is not that dark.

@LewisNyman can be use this color for background

  color: #000;
  background-image:-webkit-linear-gradient(top,#f2f2f2,#ccc);
  background-image: linear-gradient(top,#f2f2f2,#ccc);

@Sumit kumar Can we use the background color in the latest patch? Removing the whitespace is a novice issue.

So the next steps for this issue is to fix what is mentioned in #34 only, which is a Novice task :)

Removed white space from the css.

Tested patch on f678e37. Always as image below when changing admin theme between Bartik, Seven and Classy

Thanks, the correct color is #f7fcff;

Also, note the direction we are heading in #2341221: Node preview bar has usability issues, is difficult to use on mobile, not usable without Bartik, and does not align with the Seven style guide and current toolbar designs as documented in #2195695: Admin UIs on the front-end are difficult to theme. I've created a follow up to handle this here: #2539986: Move theme contextual CSS to the Seven theme

The next steps for this is to take the patch in #33, change the background to #f7fcff and post a new patch.

I've made this change and attach the updated patch and interdiff here.

The patch was created against commit 251ca21.

Applying patch worked, changing contextual links as image

I agree let's ship with standard looking good and come up with a proper solution for admin theme components being used on the front end in another issue. Committed 6e95f00 and pushed to 8.0.x. Thanks!