Contrast for add & remove shortcut links on overlay not sufficient for WCAG - Regression

Just confirmed with http://webaim.org/resources/contrastchecker/

So do you think we need to go as light as #009bfb?

In D7 Seven, it looks good:
#5f605b / #fff
Contrast Ratio: 6.3:1

Can you roll a patch for the change?

Note the issue is when the overlay is enabled, otherwise it's the grey background, so be sure to target the overlay with the patch.

Oh, I see what you mean, and your reasoning for same styling makes sense, although I think it would look better matching the h1 page title in each case, white or black. I gave up trying to track down the intended style, I believe you could invent what you like and see how others like it.

I don't know how Seven got away with the non AA compliant contrast of #0074bd link color on #e0e0d8 background either, but that's another issue.

This looks good to me. Great job @pameela!

+div.add-or-remove-shortcuts a span.text {
+  float: left;
+  padding-left:10px;
+  display: none;
+}
+
+div.add-or-remove-shortcuts a:focus span.text,
+div.add-or-remove-shortcuts a:hover span.text {
+  font-size: 12px;
+  line-height: 18px;
+  color: #fff;
+  background-color: #5f605b;
   display: block;
+  padding-right: 6px; /* LTR */
+  cursor: pointer;
+  -moz-border-radius: 5px; /* LTR */
+  -webkit-border-radius: 5px; /* LTR */
+  border-radius: 5px; /* LTR */
 }

This is too much theming for a module CSS file. It should go in Seven if anywhere. Additionally the div selector is uneccessary, not sure why it needs font size (and in pixels), padding, float declarations - why not just a different colour?

@catch the output looks good in simplytest.me. Want to review the CSS? Thanks for catching that!

There is some matching css in the RTL file too (see http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/theme...)
The float declaration can go from there and the padding will need updating too.

@pameeela do you want to reroll the patch with a modified core/themes/seven/style-rtl.css file that modifies div.add-or-remove-shortcuts?

Should be easy to do/test. Thanks @larowlan.

More code review on #10 patch:

- The last 4 property: value; lines don't need a /* LTR */ comment as they have no left/right specific values.

- padding: 0 6px 0 6px; can be shortened to padding: 0 6px;

seven/style-rtl.css just needs the declaration removed (done correctly), but none of the next added declaration needed, as it gets that all correctly from seven/style.css, there is no left / right specific values.

padding: 0 6px; means top + bottom are 0, and left + right are 6px, so does not get a /* LTR */ comment.

I did not test the patch, this is only another code review.

Great. Sorry I didn't mention this before, but the 3 blank lines need the whitespace removed. It's convenient to configure your text editor to automatically do that. Someone else please actually apply the patch for rtbc. Thanks for sticking with this pameeela!

The 3 blank lines in the patch begin with a *space*, they are not empty. Just place your cursor there and you'll see it, then hit delete.

I can't see any whitespace issues in the latest patch. Am I missing something?

Unless me and BBEdit are crazy, each one of the 3 blank lines begins with a 1 character whitespace. You can't "see" it until you place your cursor there. You can select it.

Ah, the whitespace is in the patch file, but not with a + so it does not add it to the patched files. You can disregard my whitespace rantings, although it's still a good idea to configure your text editor to strip trailing whitespace.

From CSS formatting guidelines, "There MUST NOT be any whitespace (spaces or tabs) at the end of lines. This means blank lines should also not contain any spaces or tabs."

So @echoz any reason not to mark this RTBC?

I have no more objections for someone else to finish testing, nice work pameeela :-)

Patch tested on the Mac with FF, Safari, Chrome & Opera. I guess we need IE testing though.

Thanks for the screenshots - I think this is back to RTBC

Oh, SO AWESOME to see this fixed! This has been drivin' me nuts. :)

Committed and pushed to 8.x. Thanks!

Ok, sorry to rain on the parade here because it looks like quite a bit of work has been done, but this is out of sync with the new Seven style guide:http://groups.drupal.org/node/283223.

Maybe from now on if an issue is taged with UX the first step in the process should be to check the style guide to see if there is a design. On the upside, at least we uncovered some cross-browser and accessibility issues we will know when implementing the new design :).

Yes, it is. But this fixes a bug, and the Seven styling isn't in core yet. :)