Moodle
  1. Moodle
  2. MDL-27180

File Picker does not bring focus to currently active repository and repositories not labeled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Deferred
    • Affects Version/s: 2.0.2, 2.0.3, 2.1
    • Fix Version/s: None
    • Labels:
    • Environment:
      Tested on Linux, Mac, Windows using Firefox, Chrome, Safari and IE with JAWS and NVDA screen readers
    • Rank:
      16841

      Description

      When in File picker:

      • select a repository from left pane

      Result: Focus remains on selected item in left pane

      Expected Result: Focus is moved to right pane, which is properly labeled to reflect the currently active repository

      Justification: Currently, it is very confusing for blind users to use the File picker. Changes taking place on the screen (in the right pane) are not announced and focus is not moved to them.

        Issue Links

          Activity

          Mark Sadecki created issue -
          Hide
          Michael Blake added a comment -

          Changed to blocker as this is an accessibility issue.

          Show
          Michael Blake added a comment - Changed to blocker as this is an accessibility issue.
          Michael Blake made changes -
          Field Original Value New Value
          Priority Minor [ 4 ] Blocker [ 1 ]
          Martin Dougiamas made changes -
          Workflow MDL Workflow [ 69269 ] MDL Full Workflow [ 76358 ]
          Michael de Raadt made changes -
          Fix Version/s STABLE backlog [ 10463 ]
          Labels accessibility activity filepicker keyboard usability yui accessibility filepicker keyboard triaged usability yui
          Assignee moodle.com [ moodle.com ] Dongsheng Cai [ dongsheng ]
          Affects Version/s 2.0 [ 10122 ]
          Affects Version/s 2.0.1 [ 10420 ]
          moodle.com made changes -
          Fix Version/s STABLE Sprint 12 [ 10850 ]
          Fix Version/s STABLE backlog [ 10463 ]
          moodle.com made changes -
          Assignee Dongsheng Cai [ dongsheng ] moodle.com [ moodle.com ]
          Rossiani Wijaya made changes -
          Assignee moodle.com [ moodle.com ] Rossiani Wijaya [ rwijaya ]
          Rossiani Wijaya made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Filter Manager made changes -
          Labels accessibility filepicker keyboard triaged usability yui triaged
          moodle.com made changes -
          Fix Version/s STABLE Sprint 13 [ 10952 ]
          Fix Version/s STABLE Sprint 12 [ 10850 ]
          Rossiani Wijaya made changes -
          Status Development in progress [ 3 ] Open [ 1 ]
          Rossiani Wijaya made changes -
          Assignee Rossiani Wijaya [ rwijaya ] Dongsheng Cai [ dongsheng ]
          Ankit Agarwal made changes -
          Assignee Dongsheng Cai [ dongsheng ] Ankit Agarwal [ ankit_frenz ]
          Ankit Agarwal made changes -
          Status Open [ 1 ] Development in progress [ 3 ]
          Ankit Agarwal made changes -
          Pull Master Diff URL https://github.com/ankitagarwal/moodle/compare/master...MDL-27180-master
          Pull Master Branch https://github.com/ankitagarwal/moodle/tree/MDL-27180-master
          Testing Instructions Open File picker
          Select a Repository from left panel
          Make sure focus is on the right panel
          Pull from Repository https://ankitagarwal@github.com/ankitagarwal/moodle.git
          Affects Version/s 2.1.1 [ 10750 ]
          Affects Version/s 2.0.4 [ 10652 ]
          Affects Version/s 2.2 [ 10656 ]
          Affects Version/s 2.0.2 [ 10421 ]
          Ankit Agarwal made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          moodle.com made changes -
          Peer reviewer rajeshtaneja
          Rajesh Taneja made changes -
          Rajesh Taneja made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Hide
          Rajesh Taneja added a comment - - edited

          Good work Ankit
          There are couple of things you might want to consider before pushing it for integration review:
          1. Spaces and tabs are used in code, it will be nice to replace them with spaces

          git diff origin/master
          

          look for red lines which indicate tabs.
          2. You don't have to look for type of Y.one. Check can go like

          if (firstelement === 'undefined' || !firstelement) 
          

          3. Focus is moved to right pane, but is selecting "refresh" text, even when files/folder are there in repository. I am not sure, but it might be nice to have focus on first file/folder if they are preset. I think Michael can provide some feedback on this.

          Show
          Rajesh Taneja added a comment - - edited Good work Ankit There are couple of things you might want to consider before pushing it for integration review: 1. Spaces and tabs are used in code, it will be nice to replace them with spaces git diff origin/master look for red lines which indicate tabs. 2. You don't have to look for type of Y.one. Check can go like if (firstelement === 'undefined' || !firstelement) 3. Focus is moved to right pane, but is selecting "refresh" text, even when files/folder are there in repository. I am not sure, but it might be nice to have focus on first file/folder if they are preset. I think Michael can provide some feedback on this.
          Rajesh Taneja made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Ankit Agarwal made changes -
          Status Development in progress [ 3 ] Waiting for peer review [ 10012 ]
          Rajesh Taneja made changes -
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Hide
          Rajesh Taneja added a comment -

          Thanks Ankit
          Code looks Good to me.

          Show
          Rajesh Taneja added a comment - Thanks Ankit Code looks Good to me.
          Rajesh Taneja made changes -
          Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
          Ankit Agarwal made changes -
          Status Development in progress [ 3 ] Open [ 1 ]
          Ankit Agarwal made changes -
          Affects Version/s 2.1 [ 10370 ]
          Affects Version/s 2.0.3 [ 10537 ]
          Affects Version/s 2.0.2 [ 10421 ]
          Affects Version/s 2.0.4 [ 10652 ]
          Affects Version/s 2.2 [ 10656 ]
          Affects Version/s 2.1.1 [ 10750 ]
          Ankit Agarwal made changes -
          Status Open [ 1 ] Waiting for integration review [ 10010 ]
          Testing Instructions Open File picker
          Select a Repository from left panel
          Make sure focus is on the right panel
          # Open File picker
          # Select a Repository from left panel
          # Make sure focus is on the right panel
          Hide
          Ankit Agarwal added a comment -

          Thanks Rajesh for the review

          Show
          Ankit Agarwal added a comment - Thanks Rajesh for the review
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Ankit Agarwal added a comment -

          Hi Eloy,
          Rebase done.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Eloy, Rebase done. Thanks
          Sam Hemelryk made changes -
          Currently in integration Yes
          Sam Hemelryk made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator samhemelryk
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I'm sending this back this week I think the solution to this problem needs more thought, and a couple of minor things with the patch as well sorry Ankit.
          In regards to the patch first up:

          1. ln:431 The panel id has already been stored in a local variable and it would be much better to use that panel_id defined on ln:416 - In fact now that the panel node is being used more than once I think it would be better to store it within a local var rather than retrieve it through YUI twice.
          2. The result of a Y.one call is either a node, or a null object. You only need to check if (something) and if you really want check it is not null. If we for the above create a local var for the panel we can check this in one place as presently the other use of that panel node assumes it is there and does not check.

          However I think there are a couple of things we should discuss before implementing this patch.
          Was the file picker ever designed with accessibility in mind? when playing with it I noticed that tabbing doesn't work within the filepicker for me (both before and after this patch) so even though we set the focus here it doesn't appear to be useable. I'm not entirely sure what is going wrong here I haven't looked into it perhaps it is something minor.
          In regards to shifting focus it is normally seen as a negative to move focus in regards to the likes of screen readers as it throws the user into the middle of a structure and they have no clue where they actually are in the greater scheme of things. Perhaps this file picker popup is an exception as it is like opening a new window.

          I think there are two things we need to do for this issue:

          1. Bring this to Dongshengs attention (I've added him as a watcher). He was the original author of the filepicker and can probably shed some light on decisions made about accessibility.
          2. Find an accessibility expert to have a really good look at the filepicker and its usability in whole.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I'm sending this back this week I think the solution to this problem needs more thought, and a couple of minor things with the patch as well sorry Ankit. In regards to the patch first up: ln:431 The panel id has already been stored in a local variable and it would be much better to use that panel_id defined on ln:416 - In fact now that the panel node is being used more than once I think it would be better to store it within a local var rather than retrieve it through YUI twice. The result of a Y.one call is either a node, or a null object. You only need to check if (something) and if you really want check it is not null. If we for the above create a local var for the panel we can check this in one place as presently the other use of that panel node assumes it is there and does not check. However I think there are a couple of things we should discuss before implementing this patch. Was the file picker ever designed with accessibility in mind? when playing with it I noticed that tabbing doesn't work within the filepicker for me (both before and after this patch) so even though we set the focus here it doesn't appear to be useable. I'm not entirely sure what is going wrong here I haven't looked into it perhaps it is something minor. In regards to shifting focus it is normally seen as a negative to move focus in regards to the likes of screen readers as it throws the user into the middle of a structure and they have no clue where they actually are in the greater scheme of things. Perhaps this file picker popup is an exception as it is like opening a new window. I think there are two things we need to do for this issue: Bring this to Dongshengs attention (I've added him as a watcher). He was the original author of the filepicker and can probably shed some light on decisions made about accessibility. Find an accessibility expert to have a really good look at the filepicker and its usability in whole. Cheers Sam
          Sam Hemelryk made changes -
          Status Integration review in progress [ 10004 ] Reopened [ 4 ]
          Ankit Agarwal made changes -
          Fix Version/s STABLE Sprint 14 [ 11050 ]
          Fix Version/s STABLE Sprint 13 [ 10952 ]
          Hide
          Michael de Raadt added a comment -

          We have reached an point in this work where a decision needs to be made, and in order to make this decision, it would be good to get feedback from someone who uses a screen reader or knows how they behave and what is expected. Perhaps that is Mark, the person who originally launched this issue.

          We can shift the focus from the repository to the right pane using the solution Ankit has created, however, we are not sure this is desirable, and for some repositories, it there is nothing that the focus can be shifted to.

          I propose we do two things:

          1. Leave the current solution out of integration until we get authoritative feedback on the issue, and
          2. Create a second issue that address potential for things to be targeted in the right pane (even if we don't shift focus automatically, it is important that focus can be shifted manually, without a mouse-click).
          Show
          Michael de Raadt added a comment - We have reached an point in this work where a decision needs to be made, and in order to make this decision, it would be good to get feedback from someone who uses a screen reader or knows how they behave and what is expected. Perhaps that is Mark, the person who originally launched this issue. We can shift the focus from the repository to the right pane using the solution Ankit has created, however, we are not sure this is desirable, and for some repositories, it there is nothing that the focus can be shifted to. I propose we do two things: Leave the current solution out of integration until we get authoritative feedback on the issue, and Create a second issue that address potential for things to be targeted in the right pane (even if we don't shift focus automatically, it is important that focus can be shifted manually, without a mouse-click).
          Ankit Agarwal made changes -
          Link This issue will be resolved by MDL-29418 [ MDL-29418 ]
          Eloy Lafuente (stronk7) made changes -
          Currently in integration Yes
          Hide
          Mark Sadecki added a comment -

          Thanks to everyone for their work on this issue to date. I'm sorry I have not yet had time to install the patch and test the solution with a screenreader. I will make this a priority and post my feedback here.

          Show
          Mark Sadecki added a comment - Thanks to everyone for their work on this issue to date. I'm sorry I have not yet had time to install the patch and test the solution with a screenreader. I will make this a priority and post my feedback here.
          Hide
          Michael de Raadt added a comment -

          Hi, Mark.

          It would be good to get your feedback on this.

          I suspect this will not solve the accessibility problem you raised, but we have raised the other related issue to work on in future.

          Show
          Michael de Raadt added a comment - Hi, Mark. It would be good to get your feedback on this. I suspect this will not solve the accessibility problem you raised, but we have raised the other related issue to work on in future.
          Hide
          Michael de Raadt added a comment -

          I'm brining this into the current sprint as we are still waiting for external feedback. We will look at this again at the end of the current sprint (next week).

          Show
          Michael de Raadt added a comment - I'm brining this into the current sprint as we are still waiting for external feedback. We will look at this again at the end of the current sprint (next week).
          Michael de Raadt made changes -
          Fix Version/s STABLE Sprint 15 [ 11158 ]
          Fix Version/s STABLE Sprint 14 [ 11050 ]
          Hide
          Michael de Raadt added a comment -

          I'm closing this as deferred as I think we have explored what we can in this issue and launched a new issue to deal with an extension of the problem.

          Show
          Michael de Raadt added a comment - I'm closing this as deferred as I think we have explored what we can in this issue and launched a new issue to deal with an extension of the problem.
          Michael de Raadt made changes -
          Status Reopened [ 4 ] Closed [ 6 ]
          Fix Version/s STABLE Sprint 15 [ 11158 ]
          Resolution Deferred [ 6 ]

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: