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

      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.

        Gliffy Diagrams

          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: