Clean up the CSS for Contextual module

please note: #1290562: Contextual: remove duplicated css

subscribe

Correcting link to CSS Cleanup

Starting in on this process, likely will make some mistakes as this is my first attempt to clean up the CSS. But I'm following the guidelines above.

First attempt: This patch does the following

• removes browser specific shorthands for curved borders
• removes extra margin clearning (margin: 0)
• removes extra line-height normalization: (line-height: 100%)
• moves css from contextual.css to contextual.theme.css

I tested all of these changes in IE7, IE8, IE9, and Chrome (canary) using Stark. I'll go back to testing the rest of the browsers I have access to later tonight.

Sorry, this one's a better patch. Used git format-patch to make this one.

Looks like I submitted the patch in the wrong format. Here's another attempt, this time using better syntax.

I should also note that the above patch doesn't strip the contextual links module of it's current look and feel. After re-reading 2. Remove Assumptions instruction, I could have done more to strip out the extra formatting that is present in this module.

Curved corners, padding for the li and links, are possibly more extraneous formatting are not needed for Contextual links to be functional even though it makes it more user friendly. If I'm reading the instruction for 2 correctly, I should remove anything that imposes a style or design choice. In Stark it should merely be functional.

@cosmicdreams: Patch is 0 bytes ...

This one is not empty

set to review so bot can test patch. Looks like the previous patch didn't include the new code, only removed the old code. I'll need to redo it again.

This next patch is verbose, but it does the job of removing the contents of one file and adding the renamed file. I've tried to use git format-patch -C -M --stdout > mypatch.patch so many times and I don't know why that isn't working.

Need to provide a proper patch that has to goal of :

• Have a module.base.css that provides all the styles that are needed to have this feature work properly
• Have a module.theme.css that makes the module look like it currently does.

Hi cosmicdreams,

Have you followed the steps in http://drupal.org/node/1054616? They work for me.
I will chip in with my attempt to a patch. Hopefully I have more luck :)

What to find in the patch:

• renamed contextual.css to contextual.theme.css
• as everything in the css file is theming for the frontend, there does not seem to be a need for a base/admin css,
• removed all 'div' references.
• Not sure if it is out of scope for this cleanup task, but I have added !important to all styling as it is extremely irritating that the contextual links layout breaks when you start to do custom theming. I'm open for discussion on this one.

Hi jyve:

Thanks for the effort, I was going to take another try at this during the weekend. I can share a few notes with you for guidance on what they're looking for in this patch:

Create a module.base.css that includes all of the styles that make this feature work.
Create a module.theme.css that only makes the feature look exactly like it did before.

In this patch changes like including !important to the styles would not be advised for two main reasons:
1. The styles would end up being different than the original styles. Make large changes to the styles are not in scope
2. Adding !important would make it more difficult for other modules to override the style of the contextual menu.

So with those details in mind. We should make the patch as slim as possible while focus on refactoring the existing styles into the base.css and the styles that could potentially NOT be loaded and everything would be fine. Does that make sense?

I totally agree with the fact that css for core modules should be easy to overwrite. However, I do believe that contextual links is a bit of an exception since it the HTML for this module is mixed in with all other fronted HTML. If we make it too easy to overwrite, a lot of people will see their contextual links layout break whenever they add styling to their .item-list .node-article li for example. So the !important to me just makes sure that the layout of these links don't break. They can still be overwritten if someone wants to do so.

For the base/theme css file, my feeling is that the module works without any styling, since the links are printed at the top of the entity that you can edit. All css that has been added, simply add nice styling, so should be in theme.css?

So the !important to me just makes sure that the layout of these links don't break. They can still be overwritten if someone wants to do so.

This is actually the precise reason to have a *.base.css. I agree that Contextual links are somewhat of a special case, but there's no reason we can't do this smarter. IMO, the layout aspects of this belong in contextual.base.css (we can make them as strong as they need to be so they're not easily broken), and then all the colors and other crap should go in contextual.theme.css.

The idea is that if we want to change the look of it in our themes, we don't have to loose all future layout fixes. We can just remove contextual.theme.css and easily provide our own presentation code. A good example of this can be found in page 2 of this PDF (Collapsible fieldsets): http://bit.ly/cshEyL

And once we're done with this throughout core, a base theme can remove all the *.theme.css files and get a fantastic starting point without breaking JavaScript functionality or having so many preset presentational aspects. Does that make sense?

Apparently the base vs theme css was not really clear to me yet, thanks for clarifying! Attached is a new patch that seperates functionality (base.css) from style (theme.css). I've removed all the !importants from the previous patch, but did make the selectors on the ul and li slightly more specific to compensate :).

All feedback is welcome.

Thanks @jyve! Just took a quick glance and this is looking great! :D

Found a couple of minor code style issues. Will try to test more later!

+++ b/modules/contextual/contextual.base-rtl.cssundefined
@@ -0,0 +1,9 @@
\ No newline at end of file

Needs an empty line at the bottom of the file.

+++ b/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,76 @@
+	text-align: left;

Indentation needs work.

-14 days to next Drupal core point release.

New patch attached to fix the formatting issues @Jacine mentioned.

#20 is a solid patch. When I test against stark and delete the contents of the contextual.theme.css the feature still functions. I don't get the fancy gears or anything but the module isn't broken. Exactly the aim of this issue.

I recommend RTBC.

There's only CSS in there so it doesnt need tests , either.

I agree the patch does look good, but haven't tested it and one person testing isn't enough yet. Let's not rush. This RTBC is coming very soon, don't worry. In the meantime, can we get a couple of screenshots - one with the contextual.theme.css and one with just contextual.base.css?

Thanks!

Attached is a screenshot of the contextual links with and without the theme.css.

That's great jyve, but can you submit set of screenshots that demonstrate the with and without contextual.theme.css using the Stark theme. We're supposed to base our refactoring work off of Stark, not Bartik.

If I have time tonight, I might beat you to it.

New screenshot attached, in the Spark theme this time.

Thanks Jyve. In addition to this screenshot, seeing the patch in play is really illustrative. Everything works properly without the contextual.theme.css. Further optimizations could happen in future patches.

shouldnt the fact that the links are positioned right be part of the theme.css file?

The positioning to me is about good usability, not about styling so I believe it belongs in the base.css

I disagree ... by putting the positioning rules in contextual.base.css instead of contextual.theme.css you are making a decision about what is most usable NOT what is the minimum amount of css needed to make these links function properly.

Consider the perfectly valid case of the site that wants their contextual links to be positioned on the bottom left of all blocks (etc...). Why is that unusable?

If others disagree, I will not press any further ...

Ok, you make a valid point there. My opinion is partly based on the fact that when looking at other base.css files, I noticed that positioning is considered 'basic behaviour'.

Let's see if we can get more opinions on this :)

I'd agree that the positioning probably does not belong in the .base.css in this case.

Tagging as Needs Manual Testing to get a few more people testing the changes as Jacine recommends in #23. (All these CSS cleanup issues probably should have that tag, actually.)

Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

3 changes in this new patch:

1. Rerolled the patch to match the new Drupal folder structure.

2. Moved the positioning from base.css to theme.css, as rightfully mentioned in #29.

3. Added @file headers.

A new screenshot is attached.

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,89 @@
+  z-index: 999;  ¶

Oops, some trailing whitespace here.

Other than that, the patch code style looks good, and the updated screenshots look good as well. Let's have a couple people test it and maybe review the CSS to check whether there's anything that could be optimized.

trailing white-space removed.

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,89 @@
+  width: 28px;
+  -khtml-border-radius: 4px;
+  -moz-border-radius: 4px;
+  -webkit-border-radius: 4px;
+  border-radius: 4px;
+}

css should be placed in an alphabetical order.

Hmm I'm not sure Jacine will like this like jyve noticed told me before to not change the behaviour/looks to much in stark.
She needs to shim in. Adding sprint tag for that.

18 days to next Drupal core point release.

Why are we even bothering with browser specific border radius?

http://caniuse.com/#search=border-radius says the only browsers we have to be concerned with not supporting are IE8 and 7 and there's now way to get curved corners to work performantly for those.

If you keep in mind that Drupal 8 is targetting future browers and I think you'll see how much a waste of space browser specific curved corners are.

The styles are in alphabetical order already. See #746768: Determine coding standards for CSS3 properties for more information on CSS3 and browser-specific properties.

Patch looks good, but I didn't test it.

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,89 @@
+  background-color: #fff;
+  border: 1px solid #ccc;
+  -moz-border-radius: 4px 0 4px 4px; /* LTR */
+  -webkit-border-bottom-left-radius: 4px;
+  -webkit-border-bottom-right-radius: 4px;
+  -webkit-border-top-right-radius: 0; /* LTR */
+  -webkit-border-top-left-radius: 4px; /* LTR */
+  border-radius: 4px 0 4px 4px; /* LTR */

Then we are not very consistent if the specific border stuff needs to be in the end

17 days to next Drupal core point release.

Needs review...

There is a issue related to border-radius:
#1290506: Remove webkit-specific border radius from CSS

While I am pro removing all browser specific css (-webkit and -moz), I would prefer to await the conclusion of the discussion in #1290506: Remove webkit-specific border radius from CSS and then update all core css as a separate task.
The goal of this issue is not related to that, and should not be slowed down by it.

Agree this patch test (manually) good RTBC++

okay this is maybe a new issue but is this really not rtbc were kinda forgetting a lot if the stuff with the cleanups that we wanna do in D8 ?
Im probably gonna be beaten with a stick now, anyways here goes
So I rolled a patch not to hijack, but it was a bit quicker this way with all the cross browser testing.

* Remove div.foo
as the guidelines says we should remove the div.foo naming & generally look at the classnames & markup so gets an overhall of fresh eyes.

* readability
the classnames is now shorter so the wrapper is now contextual-links which makes the css a hellof a lot more readable

Theres only one spot left where theres a div.class and thats the div.contextual-links ul.links, caused by bartiks idea of using .block ul as a basic

Theres one "huge" problem and thats the hardcoded prefix / suffix - i mean hardcoded markup we cant change in the theme wtf ;)
maybe is should be wrapped with theme_container() instead so we at least can touch it ?

function contextual_element_info() {
  $types['contextual_links'] = array(
    '#pre_render' => array('contextual_pre_render_links'),
    '#theme' => 'links__contextual',
    '#links' => array(),
    '#prefix' => '<div class="contextual-links">',
    '#suffix' => '</div>',
    '#attributes' => array(
      'class' => array('links'),
    ),
    '#attached' => array(
      'library' => array(
        array('contextual', 'contextual-links'),
      ),
    ),
  );
  return $types;
}

Browser vendor prefixes
supporting that old versions of safari & khtml really dont make any sense - so lets cut the crap now & remove the vendor prefixe's - contextual links would be a good place to start ;)

Im sorry about this not sure if it should go in as a new issue (im still new at this core stuff)

btw are we there yet where html.js is not nessesary and we can use .js instead ?

@mortendk I mostly agree and I'm totally with you on the topic of squashing the browser specific prefixes. However there is a seperate issue for that: #1290506: Remove webkit-specific border radius from CSS

Should we trim the fat during this cleanup effort or commit mass genocide on all prefixes in that issue?

@cosmicdream Well i would like that we killed it right now and take on the others - im gonna wave in for a kill browser prefixes for border-radius in the other issue. the good thing here is that we have a real issue not just an discussion ;)

Then I'll advance the issue to let the bot test the most recent patch.

fair enough -moz should not be dropped yet for the 6-7% firefox 3.6 users so its added in again

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,79 @@
+  -moz-border-radius: 4px 4px 0 0; /* FF3.6 */

prefix stuff needs to come before the real border radius stuff

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,79 @@
+div.contextual-links ul.links{ ¶

trailing whitespace

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,79 @@
+  -moz-border-radius: 4px 0 4px 4px; /* FF3.6 */

Same and our current standards place properties that have prefix variants at the end

13 days to next Drupal core point release.

to be an even better boy towards the 1% with safari4 based browsers the vendor prefix -webkit crap for each corner is added again untill theres a consensus in the development about removing support for that kinda nonsens ;)
#1344520: Drop support for safari4 in core

Feedback on the last patch:

- My only concern with generalizing the HTML and CSS is that it will break more easily when theming. On the other hand, your changes seem logic, so maybe it is up to the themers to write decent CSS so that contextual links CSS is not overwritten.
- The hardcoded prefix/suffix can be overwritten in your custom module if you want, sounds like an edge case anyway, so doesn't bother me too much.
- The browser vendor prefixes are indeed a separate issue :)
- Not sure why .js would not be enough?

Also some minor issues:
- Need to alphabetize this:

+.contextual-links .trigger {
+  background: transparent url(images/gear-select.png) no-repeat 2px 0;
+  border: 1px solid transparent;
+  height: 18px;
+  margin: 0;
+  padding: 0 2px;
+  outline: none;
+  text-indent: 34px; /* LTR */
+  width: 28px;
+}

- No need to have two lines between the code blocks (same in contextual.base.css):

}


/**
 * Contextual links.
 */
.contextual-links {

- A white space is missing after the selector:

}
div.contextual-links ul.links{
  background-color: #fff;

- White spaces missing before the values (same in the LTR version):

}
div.contextual-links ul.links{
  background-color: #fff;
  border: 1px solid #d6d6d6;
  -moz-border-radius: 4px 0 4px 4px; /* FF3.6 */
  -webkit-border-top-left-radius:4px; /* safari4 */
  -webkit-border-bottom-left-radius:4px ; /* safari4 */
  -webkit-border-bottom-right-radius:4px ; /* safari4 */

well i dare you ;) dobble dare you to try and an accedential overwrite of it - lets kick the tires a bit and see what falls off :)

hmm but i cant overwrite it in my theme right (hence the module thing in 52) - I know its an edge case, but its kinda a principle for me that everything is ease to overwrite / figure out in the theme layer - anyways its not like we dying at it is right now ;)

anyways fixed it up & alse fixed the ul li a i kinda gave a block status for all *ups*

review pls

Two small changes to improve the reduction of selector specificity in this patch (which builds off of #53)

Also a question. In contextual.theme.css our selector for styling .contextual-links' ul was :

div.contextual-links ul.links 

But in contextual.theme-rtl.css our selector was :

div.contextual-links ul 

Why the difference? Should this patch's .contextual-links .links in contextual.theme.css be .contextual-links ul instead?

i just gave it a good kick with obscure css declarations to see if i could get it to break without the use of !important and other edge cases
basically if we look at a given theme having css like this

div.content div.block ul.links a{
  background-color:pink;
  color:#green;
  font-size:100px;
  line-height:100px;
  font-family: Cambria, "Hoefler Text", Utopia, "Liberation Serif", "Nimbus Roman No9 L Regular", Times, "Times New Roman", serif;

}

@cosmicdreams huh what did the patch do - or was i blind ?

It turned div.contextual-links ul.links into .contextual-links .links in contextual.theme.css and
it turned div.contextual-links ul into contextual-links ul in contextual.theme-rtl.css

As per the cleanup initiative's description, we aren't supposed to have divs in these css files anymore. For example:


+div.contextual-links ul.links {
+ -moz-border-radius: 0 4px 4px 4px; /* FF3.6 */
+ border-radius: 0 4px 4px 4px;
+ left: 0;
+ right: auto;
+}

In several places you have added back divs in your css with that latest patch.

+++ b/core/modules/contextual/contextual.js.rejundefined
@@ -0,0 +1,56 @@
+***************
+*** 7,24 ****
+   */
+  Drupal.behaviors.contextualLinks = {
+    attach: function (context) {
+-     $('div.contextual-links-wrapper', context).once('contextual-links', function () {
+        var $wrapper = $(this);
+        var $region = $wrapper.closest('.contextual-links-region');
+-       var $links = $wrapper.find('ul.contextual-links');
+-       var $trigger = $('<a class="contextual-links-trigger" href="#" />').text(Drupal.t('Configure')).click(
+          function () {
+            $links.stop(true, true).slideToggle(100);
+            $wrapper.toggleClass('contextual-links-active');
+            return false;
+          }
+        );
+-       // Attach hover behavior to trigger and ul.contextual-links.
+        $trigger.add($links).hover(
+          function () { $region.addClass('contextual-links-region-active'); },
+          function () { $region.removeClass('contextual-links-region-active'); }
+--- 7,24 ----
+   */
+  Drupal.behaviors.contextualLinks = {
+    attach: function (context) {
++     $('div.contextual-links', context).once('links', function () {
+        var $wrapper = $(this);
+        var $region = $wrapper.closest('.contextual-links-region');
++       var $links = $wrapper.find('ul.links');
++       var $trigger = $('<a class="trigger" href="#" />').text(Drupal.t('Configure')).click(
+          function () {
+            $links.stop(true, true).slideToggle(100);
+            $wrapper.toggleClass('contextual-links-active');
+            return false;
+          }
+        );
++       // Attach hover behavior to trigger and ul.links
+        $trigger.add($links).hover(
+          function () { $region.addClass('contextual-links-region-active'); },
+          function () { $region.removeClass('contextual-links-region-active'); }
+***************
+*** 37,43 ****
+  Drupal.contextualLinks.mouseleave = function () {
+    $(this)
+      .find('.contextual-links-active').removeClass('contextual-links-active')
+-     .find('ul.contextual-links').hide();
+  };
+   ¶
+  ¶
+--- 37,43 ----
+  Drupal.contextualLinks.mouseleave = function () {
+    $(this)
+      .find('.contextual-links-active').removeClass('contextual-links-active')
++     .find('ul.links').hide();
+  };
+   ¶

There's conflicts all over this. We're not making any changes here as far as I can tell, so this shouldn't be in the patch.

/**
 * Implements hook_element_info().
 */
function contextual_element_info() {
  $types['contextual_links'] = array(
    '#pre_render' => array('contextual_pre_render_links'),
    '#theme' => 'links__contextual',
    '#links' => array(),
    '#prefix' => '<div class="contextual-links-wrapper">',
    '#suffix' => '</div>',
    '#attributes' => array(
      'class' => array('contextual-links'),
    ),
    '#attached' => array(
      'library' => array(
        array('contextual', 'contextual-links'),
      ),
    ),
  );
  return $types;
}

This isn't in the patch, but as @mortendk mentioned earlier, it's a big no-no. It's not easy to deal with #theme_wrappers theme_container() here because #attributes ends up getting used in both theme_container() and theme_links(). Been thinking about this, and think it might make sense to actually implement theme_links__contextual() to handle both the wrapper and the class of the <ul> itself.

In several places you have added back divs in your css with that latest patch.

The guidelines for what we are doing to clean up are not meant to follow as a bible, it's more general. I'm pretty sure the div.contextual is contextual, per @jyve's earlier comments in this issue, and if that's the case, then it's OK.

This is somewhat of a special case, because it is heavily mixed into both front end and back end interfaces, so we need to take special care to ensure what we do here makes sense.

the reason i have added in the div. ul. li. stuff is to protect the style for a themes css so you dont suddenly gets to overwrite the contual links with you own line heights backgrounds whatever.

its against the rule of killing as much crap as possible but in this case i actually thinks it makes sense

@mortendk So... just realized that it's a .rej file. Just delete it.

hep here it is

the links class that was added to the ul is now removed cause we dont need it

I would actually make a case that we keep the classes that are attached to the li - these could be used for some pretty sweet icon interface love ;)

i have tested the css up against: - but i still dare ya to bang on it, so we dont end up with a contextual linkes beeing accedential overweritten by a module

div.content div.block ul li a{
  background-color:pink;
  color:#green;
  font-size:100px;
  line-height:100px;
  font-family: Cambria, "Hoefler Text", Utopia, "Liberation Serif", "Nimbus Roman No9 L Regular", Times, "Times New Roman", serif;
  padding:100px;
  margin:100px;
}

Go, bot, go !

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,77 @@
+  background-color: #ff0;

does it a mistake? Yellow in color, looks very strange.

9 days to next Drupal core point release.

Renamed the classes to make em a tiny bit shorter and keep with the exsiting namespacing and use the module name as base class name

*contextual-region
class added to the region around the contextual)

*contextual
the class for the div surround the contextual links

*contextual-links
class for the for the ul so we dont end up overweiting it with the ul.links inside a node

moved the text-decoration: none; out of .base.css its pure presentational and should not ever never then be in base.css moved to theme.css

Q's about base.css
base.css now have the position (out to the right on every region) so a new theme dont have to define that everytime
wonder how others feels about it

pro: its a basic drupal think to have the config of a region out to the right

con: dude base.css shoudnt do crap unless its absolutely nessasary

Re: base.css, I think it should keep the right-positioning. It's styling that ends up enabling the basic functionality.

Love this! This module really really needed this type of attention and overhaul.

I took your patch and made a few more changes. Please let me know what you think.

1. I removed the div. pieces of the selectors. I know this ends up breaking styling in Bartik, but the fact that Bartik adds padding to uls with .block ul is a bug in Bartik. A core module shouldn't need to make concessions for a theme-level issue.
2. I added outline-offset to the .contextual-region-active class.
3. I put the color hexes in uppercase to match Drupal styleguide (I don't like it, but a guide's a guide).

Other than that, the rest of the changes look great. This will go a long way to making this piece of core more friendly to themers. I have a module that builds on contextual and it was a PITA to override some of the styling in D7.

somehow i coudnt add the patch :( - oh well tried to merge in + cleaned up a little bit more ;)

So i guess we need to have a discussion about how much we want in the base.css and what were gonna do with elements like contextual and others elements that is a deep part of the drupal system.

*wonder why the last patch didnt cleared* .. oh it late better get som sleep

This is the set of commands I run to create a patch. I keep a branch per issue in the 8.x repo. When I commit changes to a branch, I commit them as amendments, so the last commit to the branch is always the amalgamate of all the changes for the patch I'm working on.

git commit -m "Issue #0000000 by {author}: Description of the issue" --amend

git format-patch drupal.org/8.x --stdout > ~/path/to/patches/issue_description-0000000-0.patch

drupal.org is the name of the remote repo. Yours might be origin if you haven't changed the default.

and a little bit more fiddeling n perfectionizing imho ;)

I rerolled the patch with git format-patch.

There is one bit I'd like a little more discussion on:

/**
 * reset the li for accedential overwriting by a theme
 */
.contextual-region .contextual ul.contextual-links li {
  background-color: #FFF;
  border: none;
  margin: 0;
  padding: 0;
}

I'm willing to bend on this given eloquent rebuttal. I don't think that a core module should be making concessions for overzealous theme styling (contextual.theme.css). I'd like to see the code above written as:

.contextual .contextual-links li {
  background-color: #FFF;
  border: none;
  margin: 0;
  padding: 0;
}

Also, we shouldn't be putting in hacks for IE7 (contextual.theme.css).

.contextual-region-active {
  *border: #D6D6D6 dashed 1px; /* IE7 */
  outline: #D6D6D6 dashed 1px;
  outline-offset: 2px;
}

If IE7 doesn't have outlines, then I'm not too worried about that. By the time D8 ships, IE7 usage will be below 4%. Maybe even 3%. Not having an outline does not diminish the functionality of this module. The outline is simply a nice enhancement for capable browsers.

Setting to needs review to rouse the bots.

Don't introduce visual regressions. This means we don't need to add an IE7 hack if it wasn't available in drupal 7 either.
When writing new stuff we should add ie7 hacks until we stop supporting ie7.

Mm totally on board in removing the border ie7 hack its only to make "the powers that wanna support ie7" happy ;)
guess thats a more principal discussion though ?
but wtf lets take it here ;)

The reason for adding the 3 class .contextual-region to the .contextual-region .contextual ul.contextual-links li is to help out the people that accedential is gonna overwrite it #675608: Contextual link colors easily overwritten.
The idea was to add 3 classes then its gonna get harder to overwrite by accedent all it else would take is someting like a .node .content li a and you overwriting the contextual links :/

... so theres a meaning behind the class madness ;)

jacine told me i should assing me self to this so i am ;)

jacine told me i should assing me self to this so i am ;)

Um, this is not a novice patch.

Anyway, I decided to roll a patch instead of just posting a review. This is a really hard one, so I am not 100% happy/sure, but we could go on forever with this. Here's the gist of what I did.

- Fixed some issues with RTL versions of files. Some of the styles were not properly matching up.
- Moves the display block styling for the list links and li's to the .theme.css. I expect to see bullets for this in a base.css.
- Set the right/top position to 0 in the base.css files.
- Changed the comments and the grouping for what's in the .base.css file.
- Remove the .js prefix from one of the selectors in the .base.css file. This doesn't work without JS so that's misleading.
- Did some minor code style cleanups.
- Removed the border fallback for the outline in IE7.
- Changed the selector Jesse mentioned to .contextual .contextual-links li
- Separated the colors out for the stuff that should actually change on hover/active/focus. I can't stand when people change margins/positioning/font in those cases:

.contextual .contextual-links a {
  display: block;
  font-family: sans-serif;
  font-size: small;
  line-height: normal;
  margin: 0.25em 0;
  padding: 0.25em 1em;
}
.contextual .contextual-links a,
.contextual .contextual-links a:hover,
.contextual .contextual-links a:active,
.contextual .contextual-links a:focus {
  background-color: #FFF;
  color: #333;
  text-decoration: none;
}

Feel free to rip on my updates, and thanks for working so hard on this one. It's definitely one of the hardest issues we've got right now.

mortendk++!

Ugh, here is the same patch without the no newlines issue.

Ugh, one more time? Sorry for the inbox spam.

My feedback on the latest patch:

- color codes are preferred as lower case, but are all uppercase here

- border-radius should be moved up to get this alphabetical:

.contextual .contextual-links {
  background-color: #FFF;
  border: 1px solid #D6D6D6;
  margin: 0;
  padding: 0.25em 0;
  position: absolute;
  right: 0; /* LTR */
  text-align: left;
  top: 18px;
  white-space: nowrap;
  -moz-border-radius: 4px 0 4px 4px; /* FF3.6 */
  border-radius: 4px 0 4px 4px; /* LTR */
}

- An extra blank line is needed here (just a detail!):

 * @file
 * RTL styling for contextual module.
 */
.contextual {
  left: 5px;
  right: auto;

- There has been some disagreement in #29-#31 with moving the position to top /right in base.css since the module also works without that positioning. Not sure how you feel about this Jacine?

For the rest: looking good!

- border-radius should be moved up to get this alphabetical:

Yep, right. I forgot where we left off with the coding standards issue on this. :/

- There has been some disagreement in #29-#31 with moving the position to top /right in base.css since the module also works without that positioning. Not sure how you feel about this Jacine?

So... yeah. I've gone back and forth on this a gazillion times, and I'm literally torn. I think both sides of this argument have equal merit. But, if I am forced to make a decision here, I'm going to say remove the top/right properties from the .base files. I am 100% against putting values like top: 2px and right: 5px in the base file, which is why I changed them to 0/0 in the latest patch. However, this ends up meaning duplicate rules in both files, and that's not exactly ideal.

okay another roll:
* fix the lowercase issue (yeah)
* moved the top/right outta base.css so a new that dont wanna use the theme.css file dont have to overwrite this

protecting the contextual links
I have added the .contextual-region to make sure that a theme dosnt accedentially overwrites contextual links
we dont want a situation where css like this overwrites the contextual links

.sidebar-left .block ul li a = 23
.contextual-region .contextual .contextual-links li a = 32

by addin the 3. class ( .contextual-region) it will not be as easy mistake to accedentially take over the links.

Well sadly enough this needs a tiny bit of work. The font-size has to be fixed.
Morten is going to do this.

There's also a spelling error in there - "accedential" should be "accidental".

Besides #87 and #88 the latest patch looks good to me

fix the spelling error in the comment...
fixed the font sizing for bartik & cleaned it up a bit with multiple font sizings.
bartik uses a combination of em & % for the font sizing on a base of font-size: small
now its cooked down to a 80% which resemples the same size as in D7

Damnit can't rtbc

+++ b/core/modules/contextual/contextual.theme.cssundefined
--- /dev/null
+++ b/core/modules/node/node.admin.cssundefined

+++ b/core/modules/node/node.admin.cssundefined
+++ b/core/modules/node/node.admin.cssundefined
@@ -0,0 +1,12 @@

@@ -0,0 +1,12 @@
+
+/**
+ * @file
+ * Styles for administration pages.
+ */
+
+/**
+ * Revisions overview screen.
+ */
+.revision-current {
+  background: #ffc;
+}

Not part of this patch

+++ b/core/themes/bartik/css/style.cssundefined
@@ -231,15 +231,11 @@ table ul.links li {
+.contextual-region .contextual .contextual-links a{
+	font-size: 80%
+	text-shadow: 0 0 0 !important;

Replace tabs with spaces + missing ; + 80% seems to small on my d7 vs d8 tests

8 days to next Drupal core point release.

about the font sizing ...sorry for the ranting but here goes on ....

I dont remember reading anywhere that the bartik theme were set in stone, or was a monolith of excellence in theme building, & we should not be afraid to change where its necessary - This is nidpicking for the sake of nidpicking- Bartik has its own set of problems and is hardly what were trying to solve here.

But to explain & show why theres a minor minor size difference between the 2 versions
Drupal7 bartik uses a 90% for the sizing of the contextual wrapper

body {
  margin: 0;
  padding: 0;
  background: #edf5fa;
  font: 76%/170% Verdana, sans-serif;
  color: #494949;
}

so its contextuals 90% on bodys 76%

the bodys font-size sizing is changed in D8's bartik (dont ask me who or why)

body {
  line-height: 1.5;
  font-size: 87.5%;
  word-wrap: break-word;
  margin: 0;
  padding: 0;
  border: 0;
  outline: 0;
}

now its 87,5% instead of the 76% in d7 (hurray)
... to make it even more fun theres a - font-size: 0.923em on ul.contextual-links in bartik - which now for real makes it fun to calculate the font-size

if we set the font-sizing to 80% this will equals to a roughly 11 px - offcourse with the subpixels rendering, which is causing this minor minor minor changes in the font sizing. I Dont think we should use our time on finding the secret sweet spot in the breaking point aroubd font-size:82.14 & font-size:82.15% (that gives the hop to 12px)

--- rant done---

To sum it up: yes the font sizing will be slightly changed for the bartik theme - and that shows one of the problems if we wanna do "pixel perfect" with em's & percentage sizings.

ooh yeah and heres a new patch changed bartik to at least use em instead of the combination of small, em & % for the contextual links - we need a principel discussion about font sizing but this is not the place to take it ;)

go bot go

Srry it took so long for a review but I can't mark this rtbc. The differences (in stark) are to big. See screenshot. But maybe that is just me...

Btw where in this issue did we decide to use a seperate font family for this. (I think it looks cool but still want to know :) )

Ow and rtl styling doesn't get triggered somehow... (and that's more serious)

Ok the rtl stuff was caused by a core bug.
I'm marking this needs review again.

Why?
1) rtl works after all :)
2) everything looks good in bartk (tiny font size difference)

Why not rtbc?
1) Cntextual links have the same font everywhere now (sans-serif), like the toolbar. Is that ok?
2) Is the size of the contextual link div ok (in stark)? (not to big?)

+++ b/core/modules/contextual/contextual.theme.cssundefined
@@ -0,0 +1,90 @@
+  line-height: normal;
+  margin: 0.25em 0;
+  padding: 0.25em 1em;

why do we use line-height:normal. I saw in my tests it looked better if we choose something like 0.8em.

If we play with these 3 lines we can make it like it should be.

-17 days to next Drupal core point release.

This looks better. Also fixed a small rtl issue.

Very small change that riffs off of #98. This one removes the div from the div.contextual of the attach function of Drupal.behaviors.contextualLinks. Why not get rid of all the divs?

1) your patch doesn't contain the correct css
2) The divs are added to protect the contextual. (see previous comments in this issue).

So I still think #97 is ok

agreed, #97 is the way to go.

I forgot to mention that I manually tested #97 on many different browsers (IE9, Opera, FF current, Chrome Canary, and Chrome stable), for both Stark and Bartik. Everything looked good to me.

As a result of #100, RTBC

Assigning to jhodgdon, since this is a coding standards conformance patch.

I cannot get the patch in #97 to apply to 8.x at this time. I guess it needs a reroll, or did someone already commit it?

Regardless of docs/standards patch or not, let's give others sufficient time to review RTBC patches, please. This patch was RTBC'ed only a few hours before the commit attempt.

See also #1399824: [policy] Formalize min/max time thresholds for RTBC patches

+++ b/core/modules/contextual/contextual.module
@@ -62,11 +63,9 @@ function contextual_element_info() {
-    '#attributes' => array(
-      'class' => array('contextual-links'),
-    ),
+    '#attributes' => array('class'=>'contextual-links'),

Missing spaces in new code. That said, this entire change looks unnecessary to me.

+++ b/core/modules/contextual/contextual.js
@@ -12,23 +12,23 @@ Drupal.contextualLinks = Drupal.contextualLinks || {};
-    $('div.contextual-links-wrapper', context).once('contextual-links', function () {
+    $('.contextual', context).once('contextual-links', function () {

+++ b/core/modules/contextual/contextual.module
@@ -62,11 +63,9 @@ function contextual_element_info() {
-    '#prefix' => '<div class="contextual-links-wrapper">',
+    '#prefix' => '<div class="contextual">',

I'm slightly concerned the generic term "contextual" as CSS class might lead to false-positives.

I don't have concrete examples at hand, but at least I'd see an increased chance for an external CSS/JS library to use exactly that class name, too.

If you don't think that's an issue, then ok.

I just to point out there is absolutely no consensus in the referenced issue that we should add even more barriers to people trying to get their patches committed. And further, that coding standards/documentation-related patches can be much more easily fixed in follow-ups post-commit than standard bug fixes and other types of patches (for example, sun's comments here could easily be a novice task tackled during office hours), so even if there were consensus around that I would strongly advocate using a different rulestick for these types of changes.

We brought Jennifer on as a core committer to help take pressure off of catch, Dries, and I, so we could focus on majors/criticals/other bug fixes, and to help with the velocity of the RTBC queue. Makes sense to let her do her that. :)

That said, the feedback seems valid, and a quick re-roll should be able to take care of it.

Rerolled patch from #97

I'm slightly concerned the generic term "contextual" as CSS class might lead to false-positives.

Contextual is the name of the module, and that is generally how we use class names for widgets modules provide. Personally, I have no concerns about clashes here TBH. Every decent jQuery plugin allows class customization, so if there ever was a clash (which I think is unlikely) it could be easily resolved.

That said, this patch needs to be rerolled to include the addition/deletion of files. See patch in #97.

rerolled using git diff --staged > new_patch.patch

One more shot:

Alright, finally we have a winner (almost)! :D

Testing-wise this is ready to go (I've browser tested this again). This last bit has to be fixed though, as @sun mentioned:

+++ b/core/modules/contextual/contextual.moduleundefined
@@ -62,11 +63,9 @@ function contextual_element_info() {
+    '#attributes' => array('class'=>'contextual-links'),

Spacing should be:

'#attributes' => array('class' => 'contextual-links'),

Here ya go

Thanks @cosmicdreams! Looks good.

#112: 1216978-112-contextual.patch queued for re-testing.

I don't think this patch is quite ready to commit.

a) In contextual.module:

-    '#attributes' => array(
-      'class' => array('contextual-links'),
-    ),
+    '#attributes' => array('class' => 'contextual-links'),

I think we still want a nested array here, so that if someone wants to page/form alter this, they can add another class. Aren't we still doing this as a general rule?

b) In bartik/css/style.css:

-.contextual-links-wrapper a {
+.contextual-region .contextual .contextual-links a{
+  font-size: 0.9em;

There needs to be a space betwee a and { in the second line.

Good catch! I've taken care of those and also 2 more things that I didn't notice before:

+++ b/core/themes/bartik/css/style.cssundefined
@@ -229,13 +228,8 @@ table ul.links li {
-.contextual-links-wrapper {
-  font-size: small !important;
-}
-ul.contextual-links {
-  font-size: 0.923em;
-}
-.contextual-links-wrapper a {
+.contextual-region .contextual .contextual-links a{
+  font-size: 0.9em;

This font-size shouldn't be messed with. It's the wrong size and doesn't have any affect on the result anyway. All we care about here is the selector change, so, I've reverted the font-size change.

+++ b/core/modules/contextual/contextual.jsundefined
@@ -12,23 +12,23 @@ Drupal.contextualLinks = Drupal.contextualLinks || {};
-      // Attach hover behavior to trigger and ul.contextual-links.
+     // Attach hover behavior to trigger and ul.links

This still does target ul.contextual-links, not ul.links. Not sure when/why this was changed, but it doesn't belong, so I've reverted it as well.

I think it matters in stark or in Bartik. Will recheck

Make sure you check the computed font-size. It's coming in at 13 px regardless from what I saw. Thanks :)

is there anything more to do here? manual testing?

@cosmicdreams Please just read the last few comments instead of asking this all the time. It doesn't help. Thanks.

Tested and retested. Also with RTL. R T B C it is.

Tagging with coding standards, for Jennifer to take a look at.

We're over threshold on critical tasks at the moment, but I'll look at committing this when the numbers fall. I don't want to commit patches that remove/add files when we're over threshold, as they might interfere with other pending patches.

It seems that none of the patches in the critical/major queue conflict with this, so I went ahead and committed it. Yeah team!

Yay, thanks @jhodgdon!! Removing the sprint tag.

With .contextual-links-wrapper being renamed to .contextual, and .contextual-links-region to .contextual-region, this broke Views, and would likely break any CSS out there in the wild.

I'd say this is worth a change notice. There is a checkbox for "Themers", we should use it.

If someone disagrees, we should come up with a standard for "we can change this without telling anyone".

We should probably create one change notification for all the CSS cleanups, and include a table of renamed classes or IDs where needed.

Apparently this is too much to ask for. In retrospect, it is *very* hard to keep track of every markup/CSS change.

A new small portion of css clean up for latest build (a lot has changed during a year).
Reducing the weight of selectors.

@oresh, please file a new issue.

Adding docs.