AC-179 fixing skip links #9909

downzer0 · 2015-09-24T17:01:31Z

This piece of work addresses the skip links in the LMS and relates to AC-179.

Background

The skip link is a page anchor to the main content area. While the page would jump to the target container, focus wouldn't be sent correctly in all cases. The container wasn't set to receive focus (i.e., tabindex="-1") so focus wouldn't be sent. Firefox interpreted the skip link and sent focus, but this behavior is abnormal.

Solution

I did three things, one of which addresses the issue, the other two are extracurricular. I...

changed the section role="main" to a main element
added tabindex="-1" to this container, and
disabled visual focus on this container

Sandbox

https://clrux-ac-179.sandbox.edx.org

Reviewers

@dan-f (behavior)
@clytwynec (tests)

cc @cptvitamin @cahrens

dan-f · 2015-09-24T17:20:31Z

I think there should be a JS or bok_choy test to confirm that this behavior now works as expected, so that things won't break again in the future.

dan-f · 2015-09-24T17:38:58Z

@clytwynec what would be the best way to test this change? Could this be handled by the new a11y suite?

cptvitamin · 2015-09-24T19:29:59Z

@clrux @dan-f This behavior is most likely due to a known bug in Webkit (we've been talking about this in accessibility circles for years and it has to do with the way Apple interpreted the HTML5 spec. They recently acknowledged that its most likely not the correct way to interpret it and have a fix planned, but not implemented:
https://bugs.webkit.org/show_bug.cgi?id=116046

You can find out about a few elegant solutions here:
http://whatisdamon.com/blog/2012/01/why-your-skip-to-content-link-might-not-work/

cahrens · 2015-10-06T20:26:59Z

@cptvitamin and I were talking about this PR. Would the better, more general solution be to change line 139 of main.html to add tabindex="-1" to the div with id="content" (and I imagine main_django.html should also be updated)?

downzer0 · 2015-10-07T12:35:53Z

Thanks @cahrens. I've updated this branch as requested. Do you think it's wise to write tests in this branch, or work with @clytwynec to get something included in her overall accessibility test efforts?

cahrens · 2015-10-07T13:37:27Z

common/test/acceptance/pages/lms/course_page.py

+        skip_to = self.q(css=".nav-skip").attrs('href')[0]
+        self.q(css=".nav-skip").first().click()
+        return EmptyPromise(
+            lambda: self.q(skip_to).present, "Main content area received focus"


I don't think this is testing focus. Isn't it just checking that the CSS is present on the page?

I haven't tested this, but I think you should be able to just check for self.q("{}:focus".format(skip_to)).present instead?

You may also want to use self.wait_for instead of EmptyPromise. They both have the same result, but wait_for is a bit more clear.

cahrens · 2015-10-07T13:38:39Z

I think that tests should be part of this PR (and it looks like you have started them, though I'm not sure the test is doing what you think it is). I'd suggest you and @clytwynec decide together what the appropriate amount of testing should be.

downzer0 · 2015-10-07T13:43:23Z

lms/templates/main.html

@@ -107,13 +107,13 @@
 % if not disable_window_wrap:
  <div class="window-wrap" dir="${static.dir_rtl()}">
 % endif
-    <a class="nav-skip" href="<%block name="nav_skip">#content</%block>">${_("Skip to main content")}</a>
+    <a class="nav-skip" href="#content">${_("Skip to main content")}</a>


@cahrens I'm removing the dynamic construction of this, which guarantees it syncs up with the skip link. Is this okay?

I don't understand what was there before. You should check with the person who wrote that line of code, if possible.

It looks like Dave St.Germain was the original author of this line and he is no longer with us. My guess is that he was generating these href's dynamically, which was causing the skip links to only work on some pages. My change makes this reference static so it will nearly always match up with the content area.

downzer0 · 2015-10-26T15:15:46Z

Turns out it's not as simple as removing the Django dynamic stuff. It seems as if each different template defines the skip link and main content area. We'd need to do a more thorough overhaul to make them all the same, but we'd need to know what else this impacts. For now, I'm just renaming the main template from my-content to content and adding the role and tabindex. This will at least let folks skip to the main content area in the majority of the LMS.

downzer0 · 2015-11-25T18:38:17Z

@clytwynec After the break, mind if we connect on this? For some reason, focus isn't being sent to the container in the test, though it works outside of the test. IDs match and I'm using the same keystrokes. It's weird.

clytwynec · 2015-12-30T16:58:20Z

common/test/acceptance/tests/lms/test_account_settings.py

@@ -462,7 +462,6 @@ def test_account_settings_a11y(self):
        self.account_settings_page.a11y_audit.config.set_rules({
            'ignore': [
                'link-href',  # TODO: AC-233, AC-238
-                'skip-link',  # TODO: AC-179


@clrux Removing these 'skip-link' lines will result in a check for the skip link on the page during the test, so this should provide the test coverage needed.

downzer0 · 2016-02-04T19:12:36Z

@clytwynec There's a single failing a11y test. When stepping through, the skip link has a matching id, so I'm curious why this is failing. Let's review this next week.

clytwynec · 2016-02-10T16:23:23Z

@clrux Is the link target focusable on the failing page?

downzer0 · 2016-02-10T20:42:29Z

@clytwynec It is in the test browser that pops up. It has a matching id along with tabindex="-1".

clytwynec · 2016-02-10T21:29:12Z

@clrux Is the tabindex on the #content element when the test is run? Looking at this page on staging, it looks like it's being added by JavaScript at the time the link is clicked.

downzer0 · 2016-02-11T19:01:25Z

@clytwynec Whew, got it. Thanks for the tips. Can you verify this is okay?
@dan-f Can you verify skip links are working as expected?

dan-f · 2016-02-12T14:11:58Z

common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py

@@ -267,6 +267,7 @@ def test_cohorted_search_user_staff_all_content(self):
        self._goto_staff_page().set_staff_view_mode('Staff')
        self.courseware_search_page.search_for_term(self.visible_to_all_html)
        assert self.visible_to_all_html in self.courseware_search_page.search_results.html[0]
+        # from nose.tools import set_trace; set_trace()


Don't forget to remove this.

Ah, thanks!

dan-f · 2016-02-12T14:13:57Z

@clrux I can help verify if you have a sandbox for me.

Also, what's up with legacy_instructor_dashboard.html? Did it get moved from some place?

downzer0 · 2016-02-12T14:30:32Z

@dan-f Sandbox is up: https://clrux-ac-179.sandbox.edx.org/

As for the legacy dashboard, just realized that it was re-added in this work. Sarina removed it on 10 Dec. I'll remove it along with the commented test line.

dan-f · 2016-02-12T14:43:26Z

@clrux behaviorally, it's working for me in Chrome and Firefox. Safari is another story - I can't seem to focus on anything, though this has happened to me before and I think it may be safari's fault or my fault in not setting it up properly. Would you be able to verify Safari?

The skip link is getting rendered oddly when it has focus within the actual course. Here's a screenshot. Check out the upper left corner.

downzer0 · 2016-02-12T17:28:25Z

@dan-f I'm updating the sandbox. Safari bug should be fixed. Thanks for the catch!

dan-f · 2016-02-16T15:42:40Z

LGTM 👍 great work!

One minor nitpick is that I'm not seeing focus rendered on the "Skip to main content" element. Not sure if that's an issue or not, but I wanted to raise it.

Also - I figured out how to fix/enable tabbing in Safari. Here's the link: http://osxdaily.com/2011/08/14/enable-tab-key-navigation-safari/

downzer0 · 2016-02-16T17:40:56Z

@dan-f Thanks man. I noticed the lack of the outline too, but it's inconsistent, and I think it's related to styles. I wasn't going to worry about it since I originally thought that styles was out of scope. But, it does deal with skip links so I'm inclined to try and track down where the overrides are coming from.

downzer0 · 2016-02-16T19:01:53Z

@dan-f I've forced the outline. Please verify at your convenience.

dan-f · 2016-02-16T19:38:08Z

Nice. I'm not seeing the outline - did you update the sandbox?

I won't nitpick here - you have a 👍 from me, and I trust that you'll get the outline. Let me know if you want me to help verify though!

clytwynec · 2016-02-16T20:25:10Z

👍

downzer0 · 2016-02-16T20:51:45Z

@dan-f I'll squash and update the sandy if for nothing more than accountability.
@clytwynec Thanks!

downzer0 · 2016-02-17T14:09:17Z

@dan-f Sandbox is ready. I did a quick run-through and found one miss. The discussion forums focusable container didn't have a tabindex on it. I added it, squashed, and updated the sandy. Merging. Thank you both for the help!

AC-179 fixing skip links

downzer0 force-pushed the clrux/ac-179 branch from 6ccc5de to a8a07ce Compare September 24, 2015 19:31

downzer0 force-pushed the clrux/ac-179 branch from 1a1567a to 3fd0afd Compare October 7, 2015 12:31

cahrens reviewed Oct 7, 2015
View reviewed changes

downzer0 reviewed Oct 7, 2015
View reviewed changes

downzer0 force-pushed the clrux/ac-179 branch 4 times, most recently from cf1c901 to 44a9871 Compare October 26, 2015 15:14

downzer0 force-pushed the clrux/ac-179 branch 2 times, most recently from 814b787 to 15024a2 Compare October 30, 2015 19:40

downzer0 force-pushed the clrux/ac-179 branch 3 times, most recently from c61d570 to 3e9eee9 Compare November 18, 2015 19:49

downzer0 force-pushed the clrux/ac-179 branch from 7c7983c to 8b640cf Compare November 24, 2015 19:09

downzer0 force-pushed the clrux/ac-179 branch 2 times, most recently from 66469d1 to d435da3 Compare December 4, 2015 16:38

clytwynec reviewed Dec 30, 2015
View reviewed changes

downzer0 force-pushed the clrux/ac-179 branch 2 times, most recently from 0f0417d to 4baeba2 Compare February 4, 2016 17:34

downzer0 changed the title ~~LMS: fixing skip links~~ AC-179 fixing skip links Feb 9, 2016

dan-f reviewed Feb 12, 2016
View reviewed changes

downzer0 force-pushed the clrux/ac-179 branch from 3655775 to 41effd5 Compare February 16, 2016 18:50

downzer0 force-pushed the clrux/ac-179 branch from d1f91f9 to c2b1d03 Compare February 16, 2016 20:52

LMS: fixing skip links and adding a11y tests

2a4e141

downzer0 force-pushed the clrux/ac-179 branch from c2b1d03 to 2a4e141 Compare February 17, 2016 12:46

downzer0 pushed a commit that referenced this pull request Feb 17, 2016

Merge pull request #9909 from edx/clrux/ac-179

21aa7ed

AC-179 fixing skip links

downzer0 merged commit 21aa7ed into master Feb 17, 2016

downzer0 deleted the clrux/ac-179 branch February 17, 2016 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AC-179 fixing skip links #9909

AC-179 fixing skip links #9909

downzer0 commented Sep 24, 2015

dan-f commented Sep 24, 2015

dan-f commented Sep 24, 2015

cptvitamin commented Sep 24, 2015

cahrens commented Oct 6, 2015

downzer0 commented Oct 7, 2015

cahrens Oct 7, 2015

clytwynec Oct 7, 2015

clytwynec Oct 7, 2015

cahrens commented Oct 7, 2015

downzer0 Oct 7, 2015

cahrens Oct 7, 2015

downzer0 Oct 21, 2015

downzer0 commented Oct 26, 2015

downzer0 commented Nov 25, 2015

clytwynec Dec 30, 2015

downzer0 commented Feb 4, 2016

clytwynec commented Feb 10, 2016

downzer0 commented Feb 10, 2016

clytwynec commented Feb 10, 2016

downzer0 commented Feb 11, 2016

dan-f Feb 12, 2016

downzer0 Feb 12, 2016

dan-f commented Feb 12, 2016

downzer0 commented Feb 12, 2016

dan-f commented Feb 12, 2016

downzer0 commented Feb 12, 2016

dan-f commented Feb 16, 2016

downzer0 commented Feb 16, 2016

downzer0 commented Feb 16, 2016

dan-f commented Feb 16, 2016

clytwynec commented Feb 16, 2016

downzer0 commented Feb 16, 2016

downzer0 commented Feb 17, 2016

AC-179 fixing skip links #9909

AC-179 fixing skip links #9909

Conversation

downzer0 commented Sep 24, 2015

Background

Solution

Sandbox

Reviewers

dan-f commented Sep 24, 2015

dan-f commented Sep 24, 2015

cptvitamin commented Sep 24, 2015

cahrens commented Oct 6, 2015

downzer0 commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

downzer0 commented Oct 26, 2015

downzer0 commented Nov 25, 2015

Choose a reason for hiding this comment

downzer0 commented Feb 4, 2016

clytwynec commented Feb 10, 2016

downzer0 commented Feb 10, 2016

clytwynec commented Feb 10, 2016

downzer0 commented Feb 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-f commented Feb 12, 2016

downzer0 commented Feb 12, 2016

dan-f commented Feb 12, 2016

downzer0 commented Feb 12, 2016

dan-f commented Feb 16, 2016

downzer0 commented Feb 16, 2016

downzer0 commented Feb 16, 2016

dan-f commented Feb 16, 2016

clytwynec commented Feb 16, 2016

downzer0 commented Feb 16, 2016

downzer0 commented Feb 17, 2016