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

            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: