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

          Attachments

            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