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

          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.
          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.
          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.
          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
          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
          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).
          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).
          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.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: