Moodle
  1. Moodle
  2. MDL-42104

Trap focus for open modal (backport of MDL-35926)

    Details

    • Testing Instructions:
      Hide
      1. Login as a student
      2. Navigate to a course
      3. Click the assignment
      4. Click add submissions
      5. Click the Add... button in the file picker
      • Tab through the page and make sure it only select the elements within the modal.

      Additional Test (optional):

      • Repeat the above steps with screenreader on and make sure the screen reader only read elements within the modal.

      Notes: This patch only affects the forward tab direct, shift tab will still get out of the loop. It also only affects a limited set of windows - which are just the ones from filepicker/filemanager (e.g. file properties, create folder, unzip etc).

      Show
      Login as a student Navigate to a course Click the assignment Click add submissions Click the Add... button in the file picker Tab through the page and make sure it only select the elements within the modal. Additional Test (optional): Repeat the above steps with screenreader on and make sure the screen reader only read elements within the modal. Notes: This patch only affects the forward tab direct, shift tab will still get out of the loop. It also only affects a limited set of windows - which are just the ones from filepicker/filemanager (e.g. file properties, create folder, unzip etc).
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Sprint:
      FRONTEND Sprint 8

      Description

      Issue
      Context change - When the user activates Add.. modal, the focus is not being put in the modal window but is being sent back to the top of the page. Screen reader and keyboard users now have to make their way back to the modal window, only to find out it's impossible to get back in it (and therefore, close it).

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

      Impact
      Critical

      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 the Add... button in the file picker with a screen reader one
      6. Notice the screen reader starts reading at the top of the page.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Eloy Lafuente (stronk7) added a comment -

            The integration team is considering this backport request right now. Stay tuned!

            Show
            Eloy Lafuente (stronk7) added a comment - The integration team is considering this backport request right now. Stay tuned!
            Hide
            Dan Poltawski added a comment -

            This backport request has been voted on by integrators, and we'd like to see it backported asap.

            Thanks!

            Show
            Dan Poltawski added a comment - This backport request has been voted on by integrators, and we'd like to see it backported asap. Thanks!
            Hide
            Rossiani Wijaya added a comment -

            Sending for peer-review to get some feedback.

            I will backport the patch to 2.5 once it passed peer-review.

            Show
            Rossiani Wijaya added a comment - Sending for peer-review to get some feedback. I will backport the patch to 2.5 once it passed peer-review.
            Hide
            Andrew Nicols added a comment -

            -1
            Is this the correct patch? It doesn't look like https://github.com/moodle/moodle/commit/586d393fd3d72f99dd16dfba42d1d2889fcd09f6 and I don't think it will do as expected.

            Show
            Andrew Nicols added a comment - -1 Is this the correct patch? It doesn't look like https://github.com/moodle/moodle/commit/586d393fd3d72f99dd16dfba42d1d2889fcd09f6 and I don't think it will do as expected.
            Hide
            Rossiani Wijaya added a comment -

            Hi Andrew,

            The patch is slightly different from the other issue because the code base for 2.6 is changed to use dialog.

            Show
            Rossiani Wijaya added a comment - Hi Andrew, The patch is slightly different from the other issue because the code base for 2.6 is changed to use dialog.
            Hide
            Sam Hemelryk added a comment -

            Rosie, can you please create a 25 branch for this as well. It is conflicting there.

            Show
            Sam Hemelryk added a comment - Rosie, can you please create a 25 branch for this as well. It is conflicting there.
            Hide
            Sam Hemelryk added a comment -

            Ah Rosie is on annual leave, I'll look to address those conflicts now.

            Show
            Sam Hemelryk added a comment - Ah Rosie is on annual leave, I'll look to address those conflicts now.
            Hide
            Sam Hemelryk added a comment -

            Thanks all, this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks all, this has been integrated now.
            Hide
            Dan Poltawski added a comment -

            Failing for now on 2.4 this seems to have broken the file picker.

            Show
            Dan Poltawski added a comment - Failing for now on 2.4 this seems to have broken the file picker.
            Hide
            Dan Poltawski added a comment -

            Yep, failing, reverting this issue and it magically starts working.

            Show
            Dan Poltawski added a comment - Yep, failing, reverting this issue and it magically starts working.
            Hide
            Sam Hemelryk added a comment -

            Aha thanks for picking that up Dan, perhaps you could add a bit of detail about what failed as this probably won't be looked at until the new year.

            In the mean time as Rosie is away I've reverted this fix.

            Show
            Sam Hemelryk added a comment - Aha thanks for picking that up Dan, perhaps you could add a bit of detail about what failed as this probably won't be looked at until the new year. In the mean time as Rosie is away I've reverted this fix.
            Hide
            CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Michael de Raadt added a comment -

            Carrying over to the next sprint. Rosie will return later this week.

            Show
            Michael de Raadt added a comment - Carrying over to the next sprint. Rosie will return later this week.
            Hide
            Rossiani Wijaya added a comment -

            Currently, bugs fixes for 2.4 is only accepted for security issue.

            Therefore I'm removing patch for 2.4 and provided patch for 2.5.

            Show
            Rossiani Wijaya added a comment - Currently, bugs fixes for 2.4 is only accepted for security issue. Therefore I'm removing patch for 2.4 and provided patch for 2.5.
            Hide
            Rossiani Wijaya added a comment -

            Submitting for integration review

            Show
            Rossiani Wijaya added a comment - Submitting for integration review
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi, changes here look ok (overall), but I just give it a try with Safari and really, following the testing instructions, cannot find a difference with the fix and without it. In fact, in both cases, I see the focus getting the whole modal view, and then each of its panels.

            Which should be the differences? Is this for all browsers?

            Show
            Eloy Lafuente (stronk7) added a comment - Hi, changes here look ok (overall), but I just give it a try with Safari and really, following the testing instructions, cannot find a difference with the fix and without it. In fact, in both cases, I see the focus getting the whole modal view, and then each of its panels. Which should be the differences? Is this for all browsers?
            Hide
            Rossiani Wijaya added a comment -

            Hi Eloy,

            I tested this issue again and it works in the following systems:

            Windows7: ie9, firefox and chrome
            Ubuntu: firefox and chrome

            However when I tested in Mac, it didn't work as it should be in all browsers and all branches (2.5, 2.6 and master): safari, chrome and firefox. I think the Mac issue should be investigate in different issue. Reported issue MDL-43778.

            Show
            Rossiani Wijaya added a comment - Hi Eloy, I tested this issue again and it works in the following systems: Windows7: ie9, firefox and chrome Ubuntu: firefox and chrome However when I tested in Mac, it didn't work as it should be in all browsers and all branches (2.5, 2.6 and master): safari, chrome and firefox. I think the Mac issue should be investigate in different issue. Reported issue MDL-43778 .
            Hide
            Damyon Wiese added a comment -

            Integrated to 25 only. This is only a subset of the original patch - so any watchers should manage their expectations. It only handles the case of forward tabbing (no shift tab), and only covers a handful of dialogues (filepicker/filemanager) because before 26 we had not unified on M.core.dialogue, so each dialog needs to be coded for separately.

            I tested on ie8 + firefox + chrome and it worked for me.

            Thanks Rosie!

            Show
            Damyon Wiese added a comment - Integrated to 25 only. This is only a subset of the original patch - so any watchers should manage their expectations. It only handles the case of forward tabbing (no shift tab), and only covers a handful of dialogues (filepicker/filemanager) because before 26 we had not unified on M.core.dialogue, so each dialog needs to be coded for separately. I tested on ie8 + firefox + chrome and it worked for me. Thanks Rosie!
            Hide
            Dan Poltawski added a comment -

            Sorry for the delay in testing, I didn't realise I was assigned this test - it looks good.

            I tested in chromevox.

            I discovered a problem with delete action: MDL-43827.

            Show
            Dan Poltawski added a comment - Sorry for the delay in testing, I didn't realise I was assigned this test - it looks good. I tested in chromevox. I discovered a problem with delete action: MDL-43827 .
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I won't be saying "Thanks!" this week. I'm tired of it.

            For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

            Just closing this as fixed, ciao

            PS: Just a bit of black/cruel humor, sorry, LOL!

            Show
            Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile