"Skip to main content" link doesn't work in Overlay for Bartik or Stark

Subscribe.

There are 2 issues here.

1. Bartik is declaring that .overlay #skip-link be display: none; which hides it from tabbing and screen readers.
2. Stark has no color information for the link and thus renders it "blue" on the dark modal background

This patch removes the selector that sets the skip link to display: none; in Bartik and adds a default color of white to overlay-child.css for the skip link.

Leaving in the display: none; in Bartik is an oversight that was missed when the override that set it's display was removed in a recently committed patch. Also, I think that it is perfectly acceptable to use white for the default coloring of the link as it is easily overridden in a theme and follows the coloring pattern of the overlay title being white as well.

This looks great to me. There should be a period at the end of the sentence in the code comment though. (Nitpicky, I know, but if I don't point it out someone else will!)

Another issue that has come to my attention is that with the standardization of handling the skip-link in core themes the link does not align with the bottom of the toolbar in the overlay.

This patch addresses that as well as ksenzee's note on the period at the end of the sentence.

Tagging

Test plan for this patch:

* Set your admin theme to Bartik [and later test again with Stark]
* Leave the overlay enabled
* Click any admin link that opens in overlay
* Tab a few times
* Confirm that only one "skip to main content" is present; AND confirm that it is visible only when tabbed into; AND confirm that following that link takes you into the Overlay.

Test with a variety of browsers (FF, Safari, IE7+, etc) as well as screenreaders. Sounds like we should also test with a variety of (non-admin) themes too.

Is this test plan right?

You also need to test it with Stark set as your admin theme. However, there will be two "skip to main content" links in the HTML. One in the parent window and one in the overlay window. The first one you reach should be the one for the overlay and the other should not be tab accessible.

This seems to be able to still get into D7?

The Drupal 7 release party in Stockholm, Sweden will update, test and review this patch so we hopefully can make some progress on it.

By assigning this to me, I'm by no means stealing this issue. Just stating that I'll coordinate some work on this tonight.

Applied the patch and tested with both Bartik and Stark in Webkit (Chrome 9 and Safari 5), FF 3.6, Opera 10 (all on OS X) and IE 7 (Windows), though no screenreaders. It works as expected (#6) in all those browsers, except for one small issue in both the Webkit browsers. After hitting Enter on the "Skip to main content" link, it skips to the beginning of the overlay as expected, but if you tab once more it will move to the following item in the markup, being the "If you have problems accessing administrative pages…" link. In all other browsers, the topmost item inside the overlay will be selected. Not sure whether this is something we can work around, might be "by design" in Webkit.

Curiously though, the same thing happens for Bartik in IE 7 too (Seven works fine).

If this minor problem I described is worth delaying this patch is beyond me to decide. Other than that, the patch works great. Now we just need someone to test the patch with screenreaders.

Cheers from the Drupal 7 Release Party in Gothenburg!

Thanks for the testing!

If this minor problem I described is worth delaying this patch is beyond me to decide.

Does the patch introduce a regression for when Seven is used as the admin theme? If the same bug doesn't occur in HEAD, but occurs after applying the patch, then we must fix it before committing. Otherwise, I think it's okay to commit this, and work on the remaining problem separately.

No regression, the same bug applies to unpatched HEAD.

The patch looks ready to commit to me. The small CSS changes shouldn't affect screen readers, don't think we need to test in those. One thing though, I didn't test it in IE8/9, not sure if that's something that must be done?

I agree that the minor "tab order" bug found in WebKit and IE7 can be worked on separately.

+1 from me. Adding the markup tag to get this in front of people who might also want to chime in.

The release party has ended here in Gothenburg, so I'm de-assigning my self again, to not confuse people :)

This patch does not change strings or APIs. So my understanding is that we would commit it to both 8.x and 7.x.

Patch looks fine, has reviews from ksenzee and effulgentsia, good enough for me.

#4: overlay-skip-link-visibility.patch queued for re-testing.

+++ modules/overlay/overlay-child.css	3 Nov 2010 22:45:57 -0000
@@ -48,6 +48,13 @@ html.js body {
+.overlay #skip-link {
+  margin-top: -20px;
+}
+#skip-link a {
+  color: #fff; /* This is white to contrast with the dark background behind it. */
+}

The first rule targets the skip link in the overlay, the second targets it always, not limited to overlay. I don't think that this is the intention?

Powered by Dreditor.

@sun, funny note, I was talking to a guy the other day who thought with all of your posts your contributions were actually those from Sun Microsystems. You've certainly contributed more personally to Drupal than IBM, Microsoft or Oracle, so it's not that surprising in some ways.

So, it probably should be:

+++ modules/overlay/overlay-child.css	3 Nov 2010 22:45:57 -0000
@@ -48,6 +48,13 @@ html.js body {
+.overlay #skip-link {
+  margin-top: -20px;
+}
+.overlay #skip-link a {
+  color: #fff; /* This is white to contrast with the dark background behind it. */
+}

1) Thanks, clarified that in my profile.

2) Yup.

So like this.

Thanks. Didn't test myself, but previous patch was RTBC already, this new variant merely prevents follow-up bugs.

I tested it and it works as expected.

I didn't notice any visual regression in a quick test, so this looks good to go. Thanks for all the work on this, folks!

Committed to 8.x and 7.x.

That's great, thanks webchick!