Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.1
    • Component/s: Accessibility
    • Testing Instructions:
      Hide

      This patch can be tested as follows:

      Test that navigating with a keyboard is possible.
      Keys used in this test include : Tab, Space, Enter, Left Arrow (LA), Right Arrow (RA).
      1) Tab through till the navigation block. Test that you can tab through every single branch node and leaf node.
      2) Test that every non-link branch node toggle expansion on keys[Enter,Space].
      2a)Test [RA] only expands and [LA] only collapses the branch.
      3) Test that every link node toggles expansion on a [space] keypress but follows the link upon an [enter] keypress.
      3a)Test [RA] only expands and [LA] only collapses the branch.
      4) repeat steps 1-3 on the settings block.

      5) collapse the navigation and settings blocks into the dock.
      6) Test that you can Tab through the dock items.
      7) Tab to the 'remove all' button at the end of the dock. Test that Enter removes all blocks into the page and removes the dock.
      8) repeat (5).
      9) Tab to a docked block.
      10) Test that docked blocks can be viewed with [RA], a long single [RA] keypress shouldn't affect viewing.
      11) Tab the the close panel icon in the block being viewed. Test that [Enter] hides the block into the dock again.
      12) Test toggling viewing a docked block with [enter,space] keys.
      13) Test collapsing the block with [LA], a long single [LA] keypress shouldn't affect docked state.

      14) (not part of patch test as it was already working for me but for fun) View a docked block. Tab to undock button within block. Test that you can undock a single block with [enter].

      Show
      This patch can be tested as follows: Test that navigating with a keyboard is possible. Keys used in this test include : Tab, Space, Enter, Left Arrow (LA), Right Arrow (RA). 1) Tab through till the navigation block. Test that you can tab through every single branch node and leaf node. 2) Test that every non-link branch node toggle expansion on keys [Enter,Space] . 2a)Test [RA] only expands and [LA] only collapses the branch. 3) Test that every link node toggles expansion on a [space] keypress but follows the link upon an [enter] keypress. 3a)Test [RA] only expands and [LA] only collapses the branch. 4) repeat steps 1-3 on the settings block. 5) collapse the navigation and settings blocks into the dock. 6) Test that you can Tab through the dock items. 7) Tab to the 'remove all' button at the end of the dock. Test that Enter removes all blocks into the page and removes the dock. 8) repeat (5). 9) Tab to a docked block. 10) Test that docked blocks can be viewed with [RA] , a long single [RA] keypress shouldn't affect viewing. 11) Tab the the close panel icon in the block being viewed. Test that [Enter] hides the block into the dock again. 12) Test toggling viewing a docked block with [enter,space] keys. 13) Test collapsing the block with [LA] , a long single [LA] keypress shouldn't affect docked state. 14) (not part of patch test as it was already working for me but for fun) View a docked block. Tab to undock button within block. Test that you can undock a single block with [enter] .
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      16862

      Description

      Items within the navigation block do not have full keyboard support, so keyboard/screenreader/voice input users cannot access navigation.

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          a few things have been done to improve accessibility regarding navigation.

          additional to test:

          • keyboard navigation of dock
          • hide/show docked block panel
          • undock blocks
          Show
          Aparup Banerjee added a comment - a few things have been done to improve accessibility regarding navigation. additional to test: keyboard navigation of dock hide/show docked block panel undock blocks
          Hide
          Aparup Banerjee added a comment -

          note: most spacebar was used as the action on the menus. Arrows weren't used as they seem useful in scrolling the page (left/right). Maybe this sort of choice would make for a nice accessibility configuration or perhaps theres a standard?

          Show
          Aparup Banerjee added a comment - note: most spacebar was used as the action on the menus. Arrows weren't used as they seem useful in scrolling the page (left/right). Maybe this sort of choice would make for a nice accessibility configuration or perhaps theres a standard?
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          I've just been looking at this now and made the following notes:

          In regards to blocks/dock.js I think the code here could be further improved although you are certainly heading in the correct direction.

          1. Minor point calling e.stopPropogation then e.preventDefault is the same as calling e.halt. But read on before you go changing them
          2. I've tried to keep all event callbacks as properly defined functions as such I think that the stopPropogration/preventDefault/halt calls should be moved into the callback functions and then wrapped to check the event type within the callback function so that they get called is required (if that check is in fact required).
          3. The reason that the dock isn't removed when you remove the last item is because of the stopPropogration call you've added. The dock is an event driven component which defines and makes use of its own events, when an item is removed it fires and event to signal the removal of an item, there is another part of the dock listening to this event that then checks if there are any docks left and if there arn't removes the dock. Calling stopPropogation of course prevents this event from being fired.
          4. Check out http://developer.yahoo.com/yui/3/event/#keylistener which shows how to attach a key event with the desired key(s) in the definition. Doing this will allow you to remove the anonymous callbacks functions altogether.
          5. I also think it would be wise to watch for the enter key for the three cases you have there.

          blocks/navigation/yui/navigation/navigation.js

          1. Same deal as above really, not sure if you can properly delegate a key event with the included keycode however, YUI docs are pretty sketchy in that area you may need to either read the YUI code or just test it, it is doesn't look possible I'd put my +1 towards creating a dedicated function to mediate key events.
          2. I would also consider listening for enter to toggle as well as right arrow for expand, and left arrow for collapse... possibly a rtl usability issue there but would be more useful for ltr language users.

          blocks/navigation/renderer.php

          1. No need to create $spanattr - the attributes array is reset on interation just add the tabindex to that at the right time.

          One final note - make sure you test this on a number of browsers - very particually IE7 and Safari which tend to be ... unique

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, I've just been looking at this now and made the following notes: In regards to blocks/dock.js I think the code here could be further improved although you are certainly heading in the correct direction. Minor point calling e.stopPropogation then e.preventDefault is the same as calling e.halt. But read on before you go changing them I've tried to keep all event callbacks as properly defined functions as such I think that the stopPropogration/preventDefault/halt calls should be moved into the callback functions and then wrapped to check the event type within the callback function so that they get called is required (if that check is in fact required). The reason that the dock isn't removed when you remove the last item is because of the stopPropogration call you've added. The dock is an event driven component which defines and makes use of its own events, when an item is removed it fires and event to signal the removal of an item, there is another part of the dock listening to this event that then checks if there are any docks left and if there arn't removes the dock. Calling stopPropogation of course prevents this event from being fired. Check out http://developer.yahoo.com/yui/3/event/#keylistener which shows how to attach a key event with the desired key(s) in the definition. Doing this will allow you to remove the anonymous callbacks functions altogether. I also think it would be wise to watch for the enter key for the three cases you have there. blocks/navigation/yui/navigation/navigation.js Same deal as above really, not sure if you can properly delegate a key event with the included keycode however, YUI docs are pretty sketchy in that area you may need to either read the YUI code or just test it, it is doesn't look possible I'd put my +1 towards creating a dedicated function to mediate key events. I would also consider listening for enter to toggle as well as right arrow for expand, and left arrow for collapse... possibly a rtl usability issue there but would be more useful for ltr language users. blocks/navigation/renderer.php No need to create $spanattr - the attributes array is reset on interation just add the tabindex to that at the right time. One final note - make sure you test this on a number of browsers - very particually IE7 and Safari which tend to be ... unique Cheers Sam
          Hide
          Sam Marshall added a comment -

          Sounds like great progress on this, well done.

          I believe the standard action key is Enter, not Space - same as triggering links. It's probably okay if both work.

          Show
          Sam Marshall added a comment - Sounds like great progress on this, well done. I believe the standard action key is Enter, not Space - same as triggering links. It's probably okay if both work.
          Hide
          Aparup Banerjee added a comment - - edited

          Hey yall ,
          i've updated the repo in the issue with changes, have a look and see how it goes. i've tested on chrom, ie8, safari for windows 5.0.5.

          SamM, i've used Enter as an activation sort of meaning, and Space as a toggle-ish meaning but in any case they should be easily modifiable from this point. Meanings can be modified in the event definition, the rest of the code operates on the mapped meanings.
          The difference at this point is: Enter follows the link in a menu branch while Space (and respective arrows) would operate the branch.

          Show
          Aparup Banerjee added a comment - - edited Hey yall , i've updated the repo in the issue with changes, have a look and see how it goes. i've tested on chrom, ie8, safari for windows 5.0.5. SamM, i've used Enter as an activation sort of meaning, and Space as a toggle-ish meaning but in any case they should be easily modifiable from this point. Meanings can be modified in the event definition, the rest of the code operates on the mapped meanings. The difference at this point is: Enter follows the link in a menu branch while Space (and respective arrows) would operate the branch.
          Hide
          Sam Marshall added a comment -

          Aha - thanks.

          I agree that may be the best easy option in this case - it is not ideal as this probably needs to be documented so that keyboard users understand it (esp. screenreader users) but obviously if you have only one link and it can do two things, there's not much option.

          Imo it would be slightly preferable to have different links but I can see this could be difficult to arrange...

          Show
          Sam Marshall added a comment - Aha - thanks. I agree that may be the best easy option in this case - it is not ideal as this probably needs to be documented so that keyboard users understand it (esp. screenreader users) but obviously if you have only one link and it can do two things, there's not much option. Imo it would be slightly preferable to have different links but I can see this could be difficult to arrange...
          Hide
          Aparup Banerjee added a comment -

          SamM,
          i agree, making another separate focus to separate branches and their link content does make accessibility easier. (might make clicking more susceptible to clicking within non-eventful gaps but i'm sure that can be handled somehow.)
          Perhaps another issue to think about it?

          This patch now works for all keyboard naviation. one annoyance left is that the XHR branch loading doesn't trigger. working on this..

          Show
          Aparup Banerjee added a comment - SamM, i agree, making another separate focus to separate branches and their link content does make accessibility easier. (might make clicking more susceptible to clicking within non-eventful gaps but i'm sure that can be handled somehow.) Perhaps another issue to think about it? This patch now works for all keyboard naviation. one annoyance left is that the XHR branch loading doesn't trigger. working on this..
          Hide
          Aparup Banerjee added a comment -

          new to upcoming 2.1 ! improved keyboard accessibility to navigation menus! :-D

          SamH, this should be ready for integration.

          Show
          Aparup Banerjee added a comment - new to upcoming 2.1 ! improved keyboard accessibility to navigation menus! :-D SamH, this should be ready for integration.
          Hide
          Sam Hemelryk added a comment -

          Hi Apu,

          I've been reviwing this and thinking it over for a while now, the following things I think need consideration:

          blocks/dock.js

          1. 640: The remove_all event shouldn't be triggered by the right arrow, in fact I would lean towards only triggering it with the event key.
          2. You note the actionkey is required to help with Y.Delegate however you don't delegate any events with it. Need to clarify the docs

          blocks/navigation/yui/navigation/navigation.js

          1. 261, 263: Define branchp without tabindex and then is !link setAttribute('tabIndex', '0')
          2. 345, 350: e.type = 'actionkey' ... you are missing an equals.
          3. 353: Should return true for consistency.

          One more thing - I don't like the if statements where you are checking the event isn't of a certain type.
          I think all of these statements need to be clarified. I think the best way through is to check that the event is of the desired type only. This way if in the future someone adds more keys to this they don't need to review all of the previous uses of the actionkey event.
          I can see two ways of doing this:

          1. Write it into the if statements
          2. Require a forth argument on actionkey attach events where you specify the desired keys/actions and then cross reference it within the keyHandler

          I think this really needs to be cleaned up and clarified.

          The rest of the patch looks fine.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Apu, I've been reviwing this and thinking it over for a while now, the following things I think need consideration: blocks/dock.js 640: The remove_all event shouldn't be triggered by the right arrow, in fact I would lean towards only triggering it with the event key. You note the actionkey is required to help with Y.Delegate however you don't delegate any events with it. Need to clarify the docs blocks/navigation/yui/navigation/navigation.js 261, 263: Define branchp without tabindex and then is !link setAttribute('tabIndex', '0') 345, 350: e.type = 'actionkey' ... you are missing an equals. 353: Should return true for consistency. One more thing - I don't like the if statements where you are checking the event isn't of a certain type. I think all of these statements need to be clarified. I think the best way through is to check that the event is of the desired type only. This way if in the future someone adds more keys to this they don't need to review all of the previous uses of the actionkey event. I can see two ways of doing this: Write it into the if statements Require a forth argument on actionkey attach events where you specify the desired keys/actions and then cross reference it within the keyHandler I think this really needs to be cleaned up and clarified. The rest of the patch looks fine. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Ahh one more thing, in regards to the point Sam was raising about having multiple operations on one link I also agree.
          I think this should be tied in with changes to add informative titles to the navigation structures so that the likes of screenreaders can be aware of the options.
          Presently with one link this would have been a hard task, however if we split the branch from the link it should be much easier.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ahh one more thing, in regards to the point Sam was raising about having multiple operations on one link I also agree. I think this should be tied in with changes to add informative titles to the navigation structures so that the likes of screenreaders can be aware of the options. Presently with one link this would have been a hard task, however if we split the branch from the link it should be much easier. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks SamH,
          I think
          'Require a forth argument on actionkey attach events where you specify the desired keys/actions and then cross reference it within the keyHandler'
          is an awesome suggestion, working on that now.

          I think i've commented somewhere in patch the area to split up the branch from the link, this is by using a tabindex for the branch and a separate one for the link too. I can add in the title field to a branch, any suggestions on what it should contain ? ( 'branch'? node text ? )

          I've a nagging doubt about screenreaders and how they would operate on that though.

          Hm, are there any recommended screenreaders to test with?
          Cheers,
          Apu

          Show
          Aparup Banerjee added a comment - Thanks SamH, I think 'Require a forth argument on actionkey attach events where you specify the desired keys/actions and then cross reference it within the keyHandler' is an awesome suggestion, working on that now. I think i've commented somewhere in patch the area to split up the branch from the link, this is by using a tabindex for the branch and a separate one for the link too. I can add in the title field to a branch, any suggestions on what it should contain ? ( 'branch'? node text ? ) I've a nagging doubt about screenreaders and how they would operate on that though. Hm, are there any recommended screenreaders to test with? Cheers, Apu
          Hide
          Sam Marshall added a comment -

          1) For info, at the OU we test with JAWS screenreader and Internet Explorer, which is basically the industry standard, but (a) it's very expensive - I think there is a limited demo but last time I installed that it did something horrible to my PC, and (b) I leave that to somebody else, see above.

          2) My understanding is that screenreaders generally do not read the 'title' tag by default. If you want screenreaders to read some extra information for a link that isn't visible on screen, you can either add the text using <span class="accesshide"> - or if there's an icon in the link or something like that, you can put the text in the 'alt' tag.

          3) I am happy to request that our expert accessibility tester retests the standard Moodle navigation block using a screen reader once you think it's working (i,e. if there's anything we are unsure about how it ought to be done), but this is likely to take several weeks at least until she has time - it might be best if it doesn't hold up integration. Once the code is ready, should I ask her?

          Show
          Sam Marshall added a comment - 1) For info, at the OU we test with JAWS screenreader and Internet Explorer, which is basically the industry standard, but (a) it's very expensive - I think there is a limited demo but last time I installed that it did something horrible to my PC, and (b) I leave that to somebody else, see above. 2) My understanding is that screenreaders generally do not read the 'title' tag by default. If you want screenreaders to read some extra information for a link that isn't visible on screen, you can either add the text using <span class="accesshide"> - or if there's an icon in the link or something like that, you can put the text in the 'alt' tag. 3) I am happy to request that our expert accessibility tester retests the standard Moodle navigation block using a screen reader once you think it's working (i,e. if there's anything we are unsure about how it ought to be done), but this is likely to take several weeks at least until she has time - it might be best if it doesn't hold up integration. Once the code is ready, should I ask her?
          Hide
          Sam Hemelryk added a comment -

          Failing it for this week so that the last few things can be cleaned up.

          Show
          Sam Hemelryk added a comment - Failing it for this week so that the last few things can be cleaned up.
          Hide
          Aparup Banerjee added a comment -

          updated my repo details to https://github.com/nebgor/moodle/compare/mistress...MDL-27428_2

          • added the action-ables as an argument
          • clarified bit more.

          Mr Hemelryk, could you review that?
          Mr Marshall, when the time is right, please do!

          Show
          Aparup Banerjee added a comment - updated my repo details to https://github.com/nebgor/moodle/compare/mistress...MDL-27428_2 added the action-ables as an argument clarified bit more. Mr Hemelryk, could you review that? Mr Marshall, when the time is right, please do!
          Hide
          Sam Hemelryk added a comment -

          Thanks Apu this has been rebased now.
          I did interactively rebase your work before integration in order to tidy up the commit messages, your last commit message started with a / and all of your commit messages differed slightly.

          Could you please improve the testing instructions as well, the tests need to be more specific (keys, places to test) and I think testers will likely need to clear caches in order to see the changes.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Apu this has been rebased now. I did interactively rebase your work before integration in order to tidy up the commit messages, your last commit message started with a / and all of your commit messages differed slightly. Could you please improve the testing instructions as well, the tests need to be more specific (keys, places to test) and I think testers will likely need to clear caches in order to see the changes. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          Thanks Sam, the test has been updated to test the patch.

          Show
          Aparup Banerjee added a comment - Thanks Sam, the test has been updated to test the patch.
          Hide
          Andrew Davis added a comment - - edited

          Looks good although there was a minor annoyance that Ill open a separate low priority MDL for.

          update: MDL-27604 (using the calendar with the keyboard kind of sucks)

          Show
          Andrew Davis added a comment - - edited Looks good although there was a minor annoyance that Ill open a separate low priority MDL for. update: MDL-27604 (using the calendar with the keyboard kind of sucks)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this is now part of Moodle upstream, many thanks!

          Q: This is exclusively for 2.1? No interesting to backport to 20_STABLE?

          Show
          Eloy Lafuente (stronk7) added a comment - And this is now part of Moodle upstream, many thanks! Q: This is exclusively for 2.1? No interesting to backport to 20_STABLE?

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: