Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35934

File manager selecting file does not announce that the file modal opened

    Details

    • Testing Instructions:
      Hide
      1. Login as a student
      2. Navigate to a course
      3. Click the assignment
      4. Within the file manager area, select a file (to edit), if there's no file available within the filemanager, you need to add it by selecting 'add..' button.
      5. Notice it announce 'edit' for the model window.

      Note: during development, I tested this with jaws screenreader and NVDA speech viewer.

      Show
      Login as a student Navigate to a course Click the assignment Within the file manager area, select a file (to edit), if there's no file available within the filemanager, you need to add it by selecting 'add..' button. Notice it announce 'edit' for the model window. Note: during development, I tested this with jaws screenreader and NVDA speech viewer.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Sprint:
      FRONTEND Sprint 3
    • Story Points (Obsolete):
      8
    • Sprint:
      FRONTEND Sprint 3

      Description

      Issue
      Context changes - When the user selects a file in the filepicker the user is not informed that the file edit modal has opened.

      Standard Level*
      WCAG 2 3.3.2 (A) http://www.w3.org/WAI/WCAG20/quickref/#qr-minimize-error-cues

      Impact
      Serious

      Example Link
      http://demo.moodle.net/mod/assign/view.php?id=1778&action=editsubmission

      Test Steps

      1. Login as a student
      2. Navigate to a course
      3. Click the assignment
      4. Click add submissions
      5. Click Upload a file
      6. Click on the file
      7. Notice nothing is announced to the screen reader when the file edit modal appears

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            jerome Jérôme Mouneyrac added a comment - - edited

            Thanks Rosie. It's a similar fix than Jason (Folwer) in https://tracker.moodle.org/browse/MDL-35925. I was wondering how bad was the missing aria-labelledby attribut. Apparently it's not required, Rosie demoed it to me and the screenreader says 'edit'. I guess the best way would be to add an id to the dialog header (but it requires more coding) and reference this id by aria-labelledby. We could also add a hidden label. Or finally you could send it straight to integration (it works and we can have feed back later if ever it was really useful).

            Anyone with an opinion on it? Jason (Hardin), do you know about that?

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Thanks Rosie. It's a similar fix than Jason (Folwer) in https://tracker.moodle.org/browse/MDL-35925 . I was wondering how bad was the missing aria-labelledby attribut. Apparently it's not required, Rosie demoed it to me and the screenreader says 'edit'. I guess the best way would be to add an id to the dialog header (but it requires more coding) and reference this id by aria-labelledby. We could also add a hidden label. Or finally you could send it straight to integration (it works and we can have feed back later if ever it was really useful). Anyone with an opinion on it? Jason (Hardin), do you know about that?
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jerome,

            Thank you for the comment.

            While waiting for a comment from JasonH, I will try to find a better solution to add the reference id for aria-labelledby.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jerome, Thank you for the comment. While waiting for a comment from JasonH, I will try to find a better solution to add the reference id for aria-labelledby.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jerome,

            I just updated the patch by assigning an id to the edit string and used it as reference for aria-labelledby. I'm not 100% sure that this is the correct way to do it. When you have a chance, could you take a look the patch again and provide some feedback?

            Thanks.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jerome, I just updated the patch by assigning an id to the edit string and used it as reference for aria-labelledby. I'm not 100% sure that this is the correct way to do it. When you have a chance, could you take a look the patch again and provide some feedback? Thanks.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Rosie. I think it should work most of the time. A problem happens if there are multiple filemanager, then all labels would have the same id. In your solution I'm not sure how good it is to add some html tag in the headercontent.
            I look at http://yuilibrary.com/yui/docs/panel/panel-form.html demo example, the header seems to already have a YUI3 generated id. But I don't know how you can get the id from fm_js_template_fileselectlayout(). If you can't find a solution, I'd go for sending it to integration without the aria-labelledby reference as it seems to work anyway.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Rosie. I think it should work most of the time. A problem happens if there are multiple filemanager, then all labels would have the same id. In your solution I'm not sure how good it is to add some html tag in the headercontent. I look at http://yuilibrary.com/yui/docs/panel/panel-form.html demo example, the header seems to already have a YUI3 generated id. But I don't know how you can get the id from fm_js_template_fileselectlayout(). If you can't find a solution, I'd go for sending it to integration without the aria-labelledby reference as it seems to work anyway.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jerome,

            Thanks for catching that.

            I updated the patch to use unique id for each filemanagers.

            When you have a chance please take a look the patch again.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jerome, Thanks for catching that. I updated the patch to use unique id for each filemanagers. When you have a chance please take a look the patch again.
            Hide
            jerome Jérôme Mouneyrac added a comment - - edited

            Can you check/confirm/disapprove it: instead of creating a new span tag you can reference the id of the header div itself? (I suppose it's possible) Otherwise all good to me. You can send it to integration.

            Show
            jerome Jérôme Mouneyrac added a comment - - edited Can you check/confirm/disapprove it: instead of creating a new span tag you can reference the id of the header div itself? (I suppose it's possible) Otherwise all good to me. You can send it to integration.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Adding Jason to watcher list.

            Hi Jason (Fowler),

            When you have a chance could you test this issue with your version of screen reader?

            I tested mine with NVDA in windows 7 and received the feedback announcement from the screen reader:

            • IE8 and firefox: 'edit dialog'
            • Chrome: 'edit section'

            I'm not sure if it is acceptable to have the feedback to announce 'section' instead of 'dialog'.

            Will bring up this issue for discussion during scrum meeting.

            Show
            rwijaya Rossiani Wijaya added a comment - Adding Jason to watcher list. Hi Jason (Fowler), When you have a chance could you test this issue with your version of screen reader? I tested mine with NVDA in windows 7 and received the feedback announcement from the screen reader: IE8 and firefox: 'edit dialog' Chrome: 'edit section' I'm not sure if it is acceptable to have the feedback to announce 'section' instead of 'dialog'. Will bring up this issue for discussion during scrum meeting.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Requesting peer review from Jerome/Jason.

            Thanks

            Show
            rwijaya Rossiani Wijaya added a comment - Requesting peer review from Jerome/Jason. Thanks
            Hide
            jerome Jérôme Mouneyrac added a comment -

            It seems good to me. You may want to limit the size of the filename and so the header (https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R1021) For example with three little dots at the end.

            Show
            jerome Jérôme Mouneyrac added a comment - It seems good to me. You may want to limit the size of the filename and so the header ( https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R1021 ) For example with three little dots at the end.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Jerome,

            Thanks for reviewing and suggestion

            I add a limitation to display the filename on the header, instead of on the filename itself.

            Patch updated.

            Sending for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Jerome, Thanks for reviewing and suggestion I add a limitation to display the filename on the header, instead of on the filename itself. Patch updated. Sending for integration review.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Rosie,

            I really like this change there is only one thing I noticed that I think should be cleaned up.
            https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R107 and https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R108 should be using setAttribute instead of just set.
            Using set works at the moment but if YUI ever introduce a role attribute for instance this would break. Better to be strictly correct and use setAttribute instead.

            I've leave this in integration review.

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Rosie, I really like this change there is only one thing I noticed that I think should be cleaned up. https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R107 and https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R108 should be using setAttribute instead of just set. Using set works at the moment but if YUI ever introduce a role attribute for instance this would break. Better to be strictly correct and use setAttribute instead. I've leave this in integration review.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Sam,

            Thank you for reviewing.

            Note: I didn't use this.selecnode.setAttribute for https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R122, because screenreader can't read it as dialog. Changing it to use Y.one().setAttribute() reads the dialog's title correctly.

            Patch updated.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Sam, Thank you for reviewing. Note: I didn't use this.selecnode.setAttribute for https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35934#L1R122 , because screenreader can't read it as dialog. Changing it to use Y.one().setAttribute() reads the dialog's title correctly. Patch updated.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Rosie - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Rosie - this has been integrated now.
            Hide
            markn Mark Nelson added a comment -

            Testing this when I get home on my mac. My work machine refuses to run a Windows VM.

            Show
            markn Mark Nelson added a comment - Testing this when I get home on my mac. My work machine refuses to run a Windows VM.
            Hide
            marina Marina Glancy added a comment - - edited

            I have some weird looking file select dialogue now. I reverted Rosie's commits (on my test site) but it is still weird, so I assume this is related to MDL-39851:

            https://tracker.moodle.org/browse/MDL-39851?focusedCommentId=238468&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-238468

            Show
            marina Marina Glancy added a comment - - edited I have some weird looking file select dialogue now. I reverted Rosie's commits (on my test site) but it is still weird, so I assume this is related to MDL-39851 : https://tracker.moodle.org/browse/MDL-39851?focusedCommentId=238468&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-238468
            Hide
            marina Marina Glancy added a comment -

            well actually related to this one as well. Why does it say "Edit" when I'm not editing but picking the file in filepicker?

            Show
            marina Marina Glancy added a comment - well actually related to this one as well. Why does it say "Edit" when I'm not editing but picking the file in filepicker?
            Hide
            marina Marina Glancy added a comment -

            marking as failing test. Patch affects filepicker select window as well as filemanager. On 2.5 I don't see the screwed layout (it is a but of another issue) but I can still see "Edit" in the header of filepicker

            Show
            marina Marina Glancy added a comment - marking as failing test. Patch affects filepicker select window as well as filemanager. On 2.5 I don't see the screwed layout (it is a but of another issue) but I can still see "Edit" in the header of filepicker
            Hide
            markn Mark Nelson added a comment -

            Ok, guess I will leave the testing until this has been resolved.

            Show
            markn Mark Nelson added a comment - Ok, guess I will leave the testing until this has been resolved.
            Hide
            rwijaya Rossiani Wijaya added a comment - - edited

            Hi Marina,

            The 'edit' string for the filepicker is not part of this issue. This is a fix for filemanager only.

            The filepicker issue is more relevant to MDL-41033. However, since MDL-41033 has passed the test, I created MDL-41200(with patch ready) to address the string changes.

            Show
            rwijaya Rossiani Wijaya added a comment - - edited Hi Marina, The 'edit' string for the filepicker is not part of this issue. This is a fix for filemanager only. The filepicker issue is more relevant to MDL-41033 . However, since MDL-41033 has passed the test, I created MDL-41200 (with patch ready) to address the string changes.
            Hide
            marina Marina Glancy added a comment -

            all right, so many issues in filepicker/filemanager area in this cycle that I got confused. Thanks for a quick patch. Btw was it 1am for you?

            Show
            marina Marina Glancy added a comment - all right, so many issues in filepicker/filemanager area in this cycle that I got confused. Thanks for a quick patch. Btw was it 1am for you?
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Yes it was 1am when I worked on it

            Thank you for reviewing.

            Show
            rwijaya Rossiani Wijaya added a comment - Yes it was 1am when I worked on it Thank you for reviewing.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Hi Rosie,

            After chatting with Marina this morning we've decided to integrated the changes on MDL-41200 today and everything tested together here.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Hi Rosie, After chatting with Marina this morning we've decided to integrated the changes on MDL-41200 today and everything tested together here. Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            OK this is ready for testing once more guys

            Show
            samhemelryk Sam Hemelryk added a comment - OK this is ready for testing once more guys
            Hide
            markn Mark Nelson added a comment -

            Ok, so the title now displays "Select" and "Edit" in the correct scenario and when using NVDA it does announce "Edit <nameofimage>.jpg". However, there is still an issue with the display. I will attach a screenshot. Since this has nothing to do with NVDA announcing the title it should be addressed in a separate issue imo.

            Show
            markn Mark Nelson added a comment - Ok, so the title now displays "Select" and "Edit" in the correct scenario and when using NVDA it does announce "Edit <nameofimage>.jpg". However, there is still an issue with the display. I will attach a screenshot. Since this has nothing to do with NVDA announcing the title it should be addressed in a separate issue imo.
            Hide
            markn Mark Nelson added a comment -

            This is in FF:

            When clicking the image I get a dotted line around the new window until I click on the browser again.

            Also, there is no space between the label and the input fields. You can also see that the ':' is placed on a new line.

            Show
            markn Mark Nelson added a comment - This is in FF: When clicking the image I get a dotted line around the new window until I click on the browser again. Also, there is no space between the label and the input fields. You can also see that the ':' is placed on a new line.
            Hide
            marina Marina Glancy added a comment -

            Mark, dotted line is because of MDL-39851

            Show
            marina Marina Glancy added a comment - Mark, dotted line is because of MDL-39851
            Hide
            markn Mark Nelson added a comment -

            Thanks Marina. I have created MDL-41213 to address these issues.

            Show
            markn Mark Nelson added a comment - Thanks Marina. I have created MDL-41213 to address these issues.
            Hide
            poltawski Dan Poltawski added a comment -

            Cảm ơn!

            Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly!

            Một hai ba, yo

            Show
            poltawski Dan Poltawski added a comment - Cảm ơn! Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly! Một hai ba, yo

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Sep/13

                  Agile