Moodle
  1. Moodle
  2. MDL-36002

Move action for activities and course sections are not keyboard accessible with javascript enabled

    Details

    • Testing Instructions:
      Hide
      • Login as a teacher to a course
      • Enable editing
      • Using the keyboard - tab to the move icon for an activity
      • Press Enter to initiate the "keyboard drag drop"
      • In the opened dialog - tab to the selected page element to "drop" the item on
      • Press Enter to drop the item
      • Verify the "keyboard drag drop" dialog closes, the item is moved in the page and the item is focused.
      • Repeat for moving a course section
      • Repeat for moving a block
      Show
      Login as a teacher to a course Enable editing Using the keyboard - tab to the move icon for an activity Press Enter to initiate the "keyboard drag drop" In the opened dialog - tab to the selected page element to "drop" the item on Press Enter to drop the item Verify the "keyboard drag drop" dialog closes, the item is moved in the page and the item is focused. Repeat for moving a course section Repeat for moving a block
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36002-master
    • Story Points:
      8
    • Rank:
      51635
    • Sprint:
      FRONTEND Sprint 3

      Description

      Issue
      Keyboard - the move activity and move section icons are not accessible by keyboard alone when javascript is enabled.

      Standard Level
      WCAG 2 2.1.1 (A) http://www.w3.org/TR/UNDERSTANDING-WCAG20/keyboard-operation-keyboard-operable.html

      Impact
      Serious

      Example Link
      http://demo.moodle.net/course/view.php?id=625&notifyeditingon=1

      Test Steps

      1. Login as teacher
      2. Navigate to the CF101 course
      3. Turn editing on
      4. Try to tab to the move icon for the activity or section.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I'm shifting this issue into a new Epic issue, where we are collecting together accessibility issues. 218225232

          Show
          Michael de Raadt added a comment - I'm shifting this issue into a new Epic issue, where we are collecting together accessibility issues. 218225232
          Hide
          Damyon Wiese added a comment -

          Comments on this after looking for a bit. The code is replacing one or 2 non-js buttons with a drag and drop move handle that is not accessible. We don't want to show both the non-js and js buttons - and the non-js buttons can lead to ugly pages.

          I think what might be a good option here is to make the drag and drop keyboard accessible somehow. I'll have a play with this and see if I can make it work nicely.

          My thoughts is that pressing "enter" on the drag / drop icon would "start a drag" and then the arrow keys would move the content around until "enter" triggered the "drop". (And Escape == cancel).

          Show
          Damyon Wiese added a comment - Comments on this after looking for a bit. The code is replacing one or 2 non-js buttons with a drag and drop move handle that is not accessible. We don't want to show both the non-js and js buttons - and the non-js buttons can lead to ugly pages. I think what might be a good option here is to make the drag and drop keyboard accessible somehow. I'll have a play with this and see if I can make it work nicely. My thoughts is that pressing "enter" on the drag / drop icon would "start a drag" and then the arrow keys would move the content around until "enter" triggered the "drop". (And Escape == cancel).
          Hide
          Jason Hardin added a comment -

          Damyon,
          When you say use the arrow keys to move the content. What does move mean? Concern here is my initial impression would require that I can see to move the block where I want. it is better for a visually impaired user to have a set of options to place the block. And idea here is instead of kicking off drag and drop have a keyboard command that kicks off something like the old regions option where I am now focused on navigating between all of the available regions where I can place the block. All the before block x in column 1 or after block y in column 2 etc.

          Show
          Jason Hardin added a comment - Damyon, When you say use the arrow keys to move the content. What does move mean? Concern here is my initial impression would require that I can see to move the block where I want. it is better for a visually impaired user to have a set of options to place the block. And idea here is instead of kicking off drag and drop have a keyboard command that kicks off something like the old regions option where I am now focused on navigating between all of the available regions where I can place the block. All the before block x in column 1 or after block y in column 2 etc.
          Hide
          Damyon Wiese added a comment -

          Jason - that was an early comment. After thinking more - I came to the same conclusion you did. The only sensible thing to do is open a menu of possible drop targets and let them choose one.

          See this page for more reference:

          http://dev.opera.com/articles/view/accessible-drag-and-drop/

          Show
          Damyon Wiese added a comment - Jason - that was an early comment. After thinking more - I came to the same conclusion you did. The only sensible thing to do is open a menu of possible drop targets and let them choose one. See this page for more reference: http://dev.opera.com/articles/view/accessible-drag-and-drop/
          Hide
          Damyon Wiese added a comment -

          Note: this change adds a grab handle to each block - which may disqualify it from backporting.

          Show
          Damyon Wiese added a comment - Note: this change adds a grab handle to each block - which may disqualify it from backporting.
          Hide
          Jérôme Mouneyrac added a comment -

          First, this is great to see it working for screen reader users. Thanks Damyon. I noticed few things:

          • the move is not saved (loading icon not displayed).
          • The aria-labelledby of the overlay reference an label id that doesn't exist.
          • when pressing escape we lose the location where we were. (Does the user expects to be focussing the cancelled hit draghandler? Is it a big deal to lose the previous focus?)
          • when moving items, how do you do a disctinction between section and items ? I don't know if it's real an annoyance thought... just mentioning.
          Show
          Jérôme Mouneyrac added a comment - First, this is great to see it working for screen reader users. Thanks Damyon. I noticed few things: the move is not saved (loading icon not displayed). The aria-labelledby of the overlay reference an label id that doesn't exist. when pressing escape we lose the location where we were. (Does the user expects to be focussing the cancelled hit draghandler? Is it a big deal to lose the previous focus?) when moving items, how do you do a disctinction between section and items ? I don't know if it's real an annoyance thought... just mentioning.
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome,

          I added a fix for point 1. The problem was yui was throwing an exception from the simulated drag drop event. I added a commit that creates a proper wrapper for the keyboard drag drop event to fix this.

          Point 2. Good spotting - this is a problem with M.core.dialogue though and I suspect it will be fixed by Jason + Rosies changes this week.

          Added a commit to fix point 3.

          Re point 4 - I really did not want to change the API for drag drop at all for this change - this is why I get the names of the drop regions from the DOM.

          Show
          Damyon Wiese added a comment - Thanks Jerome, I added a fix for point 1. The problem was yui was throwing an exception from the simulated drag drop event. I added a commit that creates a proper wrapper for the keyboard drag drop event to fix this. Point 2. Good spotting - this is a problem with M.core.dialogue though and I suspect it will be fixed by Jason + Rosies changes this week. Added a commit to fix point 3. Re point 4 - I really did not want to change the API for drag drop at all for this change - this is why I get the names of the drop regions from the DOM.
          Hide
          Damyon Wiese added a comment -

          No outstanding issues - sending for integration.

          Show
          Damyon Wiese added a comment - No outstanding issues - sending for integration.
          Hide
          Dan Poltawski added a comment -

          Thanks Damyon, I've integrated this to master.

          Show
          Dan Poltawski added a comment - Thanks Damyon, I've integrated this to master.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          There's no test instructions here - for sure they would help.

          I tried turning on editing, tabbing through to the move icon and hitting enter but all that happens is the focus shifts back to the start of the document.
          If I hit space instead of enter nothing at all happens, not even the browser default of scrolling down a page.

          Failing this as this definitely seems wrong.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, There's no test instructions here - for sure they would help. I tried turning on editing, tabbing through to the move icon and hitting enter but all that happens is the focus shifts back to the start of the document. If I hit space instead of enter nothing at all happens, not even the browser default of scrolling down a page. Failing this as this definitely seems wrong. Cheers Sam
          Hide
          Damyon Wiese added a comment - - edited

          Yep - there was a problem with this on the integration branch.

          A fix is ready:

          Repo: https://github.com/damyon/moodle.git
          Branch: MDL-36002-master-fix1
          Diff: https://github.com/damyon/moodle/commit/70b4027b521df2c1b6d500ed06187ccb5442a1d9 (edited work with space as well).
          Also sorry for the testing instructions - I thought I had done it but must have my issues confused.

          Show
          Damyon Wiese added a comment - - edited Yep - there was a problem with this on the integration branch. A fix is ready: Repo: https://github.com/damyon/moodle.git Branch: MDL-36002 -master-fix1 Diff: https://github.com/damyon/moodle/commit/70b4027b521df2c1b6d500ed06187ccb5442a1d9 (edited work with space as well). Also sorry for the testing instructions - I thought I had done it but must have my issues confused.
          Hide
          Dan Poltawski added a comment -

          Sent back to testing

          Show
          Dan Poltawski added a comment - Sent back to testing
          Hide
          Sam Hemelryk added a comment -

          Cool thanks for tidying that up Damyon.

          After playing with this a while there are a few things that I noticed that I think need improvement.

          1. The item being moved shouldn't have a "Move after blah" action, instead it should probably be titled "Don't move blah" or "Leave in place". Something that describes what would happen anyway (moving something after itself does nothing).
          2. When moving an activity the sections you can move to need to be labelled differently, as above "Move before Section 4" doesn't describe what is going to occur, perhaps "To the top of Section 1" or something. Talk to Helen she'll have better ideas than me. In fact thinking about how different course formats can work I suspect that you'd be better asking the course format how it would like to describe moving to a section.
          3. When moving a section or activity the first very section is labelled as "After Add file(s) here" - surely that can be improved as well, describing a section by just one activity that can be performed on it is pretty confusing.
          4. When moving a block the option to move after the "add a block" block is not a valid argument. You cant move a valid block after a fake block the end effect is nothing happens.
          5. There is bug if you try to move the first block on the right after the last block on the left within a theme that uses the right as the default region. In this situation the block is moved visually but not XHR is fired so when you refresh the block has reverted to its former position. If you move several blocks after this you get a mess. See steps below to reproduce.

          To reproduce the bug:

          1. Edit theme/base/config.php and change the default region for the course layout to side-post.
          2. Log in as admin.
          3. Enter a course and turn on editing.
          4. Tab to the move icon for the first block on the right and hit enter.
          5. Select the last block on the left and hit enter.
          6. Observe the block has moved.
          7. Refresh the page.
          8. Observe that the block in fact did not move.
            Somewhere along this process an error is generated as well:
            TypeError: this.dragsourceregion is null
            

          During testing I found an existing bug as well MDL-41204.

          So decisions - this seems like a real improvement but there are a few tweaks still to be made and a bug to be diagnosed.
          As this is master only I think its fine to let it in and open issues to be included in the next frontend sprint and fixed before the release of 2.6.
          The decision is yours and Dan's of course, for the time being I'll fail this pending decision.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Cool thanks for tidying that up Damyon. After playing with this a while there are a few things that I noticed that I think need improvement. The item being moved shouldn't have a "Move after blah" action, instead it should probably be titled "Don't move blah" or "Leave in place". Something that describes what would happen anyway (moving something after itself does nothing). When moving an activity the sections you can move to need to be labelled differently, as above "Move before Section 4" doesn't describe what is going to occur, perhaps "To the top of Section 1" or something. Talk to Helen she'll have better ideas than me. In fact thinking about how different course formats can work I suspect that you'd be better asking the course format how it would like to describe moving to a section. When moving a section or activity the first very section is labelled as "After Add file(s) here" - surely that can be improved as well, describing a section by just one activity that can be performed on it is pretty confusing. When moving a block the option to move after the "add a block" block is not a valid argument. You cant move a valid block after a fake block the end effect is nothing happens. There is bug if you try to move the first block on the right after the last block on the left within a theme that uses the right as the default region. In this situation the block is moved visually but not XHR is fired so when you refresh the block has reverted to its former position. If you move several blocks after this you get a mess. See steps below to reproduce. To reproduce the bug: Edit theme/base/config.php and change the default region for the course layout to side-post. Log in as admin. Enter a course and turn on editing. Tab to the move icon for the first block on the right and hit enter. Select the last block on the left and hit enter. Observe the block has moved. Refresh the page. Observe that the block in fact did not move. Somewhere along this process an error is generated as well: TypeError: this .dragsourceregion is null During testing I found an existing bug as well MDL-41204 . So decisions - this seems like a real improvement but there are a few tweaks still to be made and a bug to be diagnosed. As this is master only I think its fine to let it in and open issues to be included in the next frontend sprint and fixed before the release of 2.6. The decision is yours and Dan's of course, for the time being I'll fail this pending decision. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          After discussing with Damyon I've reverted this issue and am reopening it for further work.

          Show
          Sam Hemelryk added a comment - After discussing with Damyon I've reverted this issue and am reopening it for further work.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Damyon Wiese added a comment - - edited

          Re: chatting with Sam about this one.

          The best approach here might be to use the current JS for getting the drag and drop region - but if the regions specifies an "aria-labelledby" attribute - get it from there instead.

          Show
          Damyon Wiese added a comment - - edited Re: chatting with Sam about this one. The best approach here might be to use the current JS for getting the drag and drop region - but if the regions specifies an "aria-labelledby" attribute - get it from there instead.
          Hide
          Damyon Wiese added a comment -

          Pushed a new branch.

          Addressing Sams comments:
          1. The item being moved shouldn't have...

          It doesn't - the item being moved is not shown in the list. It would have been showing the section though and getting the name from the activity (which it now doesn't)

          2. When moving an activity the sections you can move to need to be labelled differently...

          I changed the text to say 'Move to "14 August - 20 August"' or 'Move to "News Forum"'. It is really hard to be more descriptive without requiring tons of changes in course formats etc.

          3. When moving a section or activity the first very section is labelled as "After Add file(s) here"...

          I added access hide text to the first section in a course format that it uses instead. It gets the name from the course format as usual so it will be "General" or something.

          4. When moving a block the option to move after the "add a block" block is not a valid argument...

          Well it is just more obvious this way. It is a valid drop region and I don't want to add special cases. It does actually function to move the block to the bottom of the list.

          5. There is bug...

          Fixed! (Had to simulate drag_start for the blocks dnd code).

          Thanks for the suggestions Sam - this looks better now. Sending for integration.

          Show
          Damyon Wiese added a comment - Pushed a new branch. Addressing Sams comments: 1. The item being moved shouldn't have... It doesn't - the item being moved is not shown in the list. It would have been showing the section though and getting the name from the activity (which it now doesn't) 2. When moving an activity the sections you can move to need to be labelled differently... I changed the text to say 'Move to "14 August - 20 August"' or 'Move to "News Forum"'. It is really hard to be more descriptive without requiring tons of changes in course formats etc. 3. When moving a section or activity the first very section is labelled as "After Add file(s) here"... I added access hide text to the first section in a course format that it uses instead. It gets the name from the course format as usual so it will be "General" or something. 4. When moving a block the option to move after the "add a block" block is not a valid argument... Well it is just more obvious this way. It is a valid drop region and I don't want to add special cases. It does actually function to move the block to the bottom of the list. 5. There is bug... Fixed! (Had to simulate drag_start for the blocks dnd code). Thanks for the suggestions Sam - this looks better now. Sending for integration.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Hi Damyon,

          There seems to be a weird bug where by 'move left' always appears as an action (sometimes uselessless) but never move right?

          Show
          Dan Poltawski added a comment - Hi Damyon, There seems to be a weird bug where by 'move left' always appears as an action (sometimes uselessless) but never move right?
          Hide
          Dan Poltawski added a comment -

          Debugging spotted:

          The option name numsections in course format topics is invalid because the field with the same name exists in {course} table
          	•	line 246 of /course/format/lib.php: call to debugging()
          	•	line 281 of /course/format/lib.php: call to format_base->get_course()
          	•	line 301 of /course/format/lib.php: call to format_base->get_sections()
          	•	line 56 of /course/format/topics/lib.php: call to format_base->get_section()
          	•	line 2080 of /course/lib.php: call to format_topics->get_section_name()
          	•	line 1889 of /lib/navigationlib.php: call to get_section_name()
          	•	line 420 of /course/format/lib.php: call to global_navigation->load_generic_course_sections()
          	•	line 147 of /course/format/topics/lib.php: call to format_base->extend_course_navigation()
          
          The option name hiddensections in course format topics is invalid because the field with the same name exists in {course} table
          	•	line 246 of /course/format/lib.php: call to debugging()
          	•	line 281 of /course/format/lib.php: call to format_base->get_course()
          	•	line 301 of /course/format/lib.php: call to format_base->get_sections()
          
          The option name coursedisplay in course format topics is invalid because the field with the same name exists in {course} table
          	•	line 246 of /course/format/lib.php: call to debugging()
          	•	line 281 of /course/format/lib.php: call to format_base->get_course()
          	•	line 301 of /course/format/lib.php: call to format_base->get_sections()
          	•	line 56 of /course/format/topics/lib.php: call to format_base->get_section()
          	•	line 2080 of /course/lib.php: call to format_topics->get_section_name()
          
          Show
          Dan Poltawski added a comment - Debugging spotted: The option name numsections in course format topics is invalid because the field with the same name exists in {course} table • line 246 of /course/format/lib.php: call to debugging() • line 281 of /course/format/lib.php: call to format_base->get_course() • line 301 of /course/format/lib.php: call to format_base->get_sections() • line 56 of /course/format/topics/lib.php: call to format_base->get_section() • line 2080 of /course/lib.php: call to format_topics->get_section_name() • line 1889 of /lib/navigationlib.php: call to get_section_name() • line 420 of /course/format/lib.php: call to global_navigation->load_generic_course_sections() • line 147 of /course/format/topics/lib.php: call to format_base->extend_course_navigation() The option name hiddensections in course format topics is invalid because the field with the same name exists in {course} table • line 246 of /course/format/lib.php: call to debugging() • line 281 of /course/format/lib.php: call to format_base->get_course() • line 301 of /course/format/lib.php: call to format_base->get_sections() The option name coursedisplay in course format topics is invalid because the field with the same name exists in {course} table • line 246 of /course/format/lib.php: call to debugging() • line 281 of /course/format/lib.php: call to format_base->get_course() • line 301 of /course/format/lib.php: call to format_base->get_sections() • line 56 of /course/format/topics/lib.php: call to format_base->get_section() • line 2080 of /course/lib.php: call to format_topics->get_section_name()
          Hide
          Damyon Wiese added a comment -

          Thanks Dan - Move anywhere should never appear in the list. Not sure what caused that - and the above will be related to the change that always writes a name for the section.

          I'll see if there is a quick fix.

          Show
          Damyon Wiese added a comment - Thanks Dan - Move anywhere should never appear in the list. Not sure what caused that - and the above will be related to the change that always writes a name for the section. I'll see if there is a quick fix.
          Hide
          Damyon Wiese added a comment -

          I pushed one extra commit to get the correct names for labels (and hidden activities) in the popup.

          Show
          Damyon Wiese added a comment - I pushed one extra commit to get the correct names for labels (and hidden activities) in the popup.
          Hide
          Damyon Wiese added a comment -

          Note: I could not reproduce the above debugging error.

          Show
          Damyon Wiese added a comment - Note: I could not reproduce the above debugging error.
          Hide
          Dan Poltawski added a comment -

          I've tried a few times to reproduce the debugging error, but can't.

          Show
          Dan Poltawski added a comment - I've tried a few times to reproduce the debugging error, but can't.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, thanks Damyon

          Show
          Dan Poltawski added a comment - Integrated to master, thanks Damyon
          Hide
          Dan Poltawski added a comment -

          Its all looking good, apart from block moving. For some reason the block movmeent icon isn't appearing on the clean theme.

          Show
          Dan Poltawski added a comment - Its all looking good, apart from block moving. For some reason the block movmeent icon isn't appearing on the clean theme.
          Hide
          Damyon Wiese added a comment -

          A fix is available for clean - it had a different drag/drop path for blocks that I missed.

          Repo: git://github.com/damyon/moodle.git
          branch: MDL-36002-master-fix1

          Show
          Damyon Wiese added a comment - A fix is available for clean - it had a different drag/drop path for blocks that I missed. Repo: git://github.com/damyon/moodle.git branch: MDL-36002 -master-fix1
          Hide
          Dan Poltawski added a comment -

          Yay. Passing.

          Show
          Dan Poltawski added a comment - Yay. Passing.
          Hide
          Damyon Wiese added a comment -

          Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

          Hurray!

          Show
          Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile