Moodle
  1. Moodle
  2. MDL-34671

Pressing Escape in modal dialogues causes full screen to exit in some browsers

    Details

    • Testing Instructions:
      Hide

      Using both of the following browsers (and any other browsers you think may be affected):

      • Opera under Windows
      • Safari under MacOS
      • Make your browser full screen
      • Open a course
      • Turn editing on
      • Open the Module Chooser
      • Press escape
        • Confirm that the module chooser closes but the window does not change out of full screen
      • On an existing resource, use the quick rename function in the toolbox
      • Press escape
        • Confirm that the quick rename cancels but the window does not change out of full screen
      Show
      Using both of the following browsers (and any other browsers you think may be affected): Opera under Windows Safari under MacOS Make your browser full screen Open a course Turn editing on Open the Module Chooser Press escape Confirm that the module chooser closes but the window does not change out of full screen On an existing resource, use the quick rename function in the toolbox Press escape Confirm that the quick rename cancels but the window does not change out of full screen
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-34671-m24
    • Pull Master Branch:
    • Rank:
      43126

      Description

      If you're using some browsers and are in full screen with a modal dialogue open, pressing escape should close the modal dialogue but not exit full screen.
      I've seen this with:

      • Opera under Windows
      • Safari under Chrome

      and in the following dialogues (and other locations):

      • modchooser (moodle-core-chooserdialogue)
      • filepicker
      • resource toolbox rename tool

      In both the modchooser and toolbox it's simply a case of changing the onkeyup event to an onkeydown event (keyup is too late to prevent the default browser action).

      I haven't managed to figure out what's triggering the filepicker being closed on escape yet.

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          The filepicker is caused by an issue with Y.Panel not doing the right thing - see http://yuilibrary.com/projects/yui3/ticket/2532616

          We can possibly work around this in Moodle by unsetting the mainui.hideon and adding an event listener instead, but I don't know whether we should or not.

          Show
          Andrew Nicols added a comment - The filepicker is caused by an issue with Y.Panel not doing the right thing - see http://yuilibrary.com/projects/yui3/ticket/2532616 We can possibly work around this in Moodle by unsetting the mainui.hideon and adding an event listener instead, but I don't know whether we should or not.
          Hide
          Andrew Nicols added a comment -

          Petr, Adding you as a watcher to this as maintainer of the YUI3 integration.
          I don't know what your thoughts are on overriding the hideOn value - I guess it depends on how quickly the upstream bug is worked.

          Show
          Andrew Nicols added a comment - Petr, Adding you as a watcher to this as maintainer of the YUI3 integration. I don't know what your thoughts are on overriding the hideOn value - I guess it depends on how quickly the upstream bug is worked.
          Hide
          Petr Škoda added a comment -

          Let's see how they resolve it in upstream and then we could try to monkey patch it for us in a similar way. Thanks for the report.

          Show
          Petr Škoda added a comment - Let's see how they resolve it in upstream and then we could try to monkey patch it for us in a similar way. Thanks for the report.
          Hide
          Andrew Nicols added a comment -

          Okay - seems appropriate. Most elegant fix I have so far is to override the default hideOn with the same node and keyCode, but against keydown instead of key:

          diff --git a/repository/filepicker.js b/repository/filepicker.js
          index 7c4e1f4..51b0c35 100644
          --- a/repository/filepicker.js
          +++ b/repository/filepicker.js
          @@ -1869,6 +1869,15 @@ M.core_filepicker.init = function(Y, options) {
                   },
                   launch: function() {
                       this.render();
          +            // Override the default hideOn to ensure that pressing escape
          +            // does not exit browser fullscreen
          +            this.mainui.set('hideOn', [
          +                {
          +                    node: Y.one('document'),
          +                    eventName: 'keydown',
          +                    keyCode: 27
          +                }
          +            ]);
                   },
                   show_recent_repository: function() {
                       this.hide_header();
          
          Show
          Andrew Nicols added a comment - Okay - seems appropriate. Most elegant fix I have so far is to override the default hideOn with the same node and keyCode, but against keydown instead of key: diff --git a/repository/filepicker.js b/repository/filepicker.js index 7c4e1f4..51b0c35 100644 --- a/repository/filepicker.js +++ b/repository/filepicker.js @@ -1869,6 +1869,15 @@ M.core_filepicker.init = function(Y, options) { }, launch: function() { this .render(); + // Override the default hideOn to ensure that pressing escape + // does not exit browser fullscreen + this .mainui.set('hideOn', [ + { + node: Y.one('document'), + eventName: 'keydown', + keyCode: 27 + } + ]); }, show_recent_repository: function() { this .hide_header();
          Hide
          Andrew Nicols added a comment -

          Have moved the Y.Panel issue to a separate issue so it can be assessed later when we know what's happening upstream.

          Show
          Andrew Nicols added a comment - Have moved the Y.Panel issue to a separate issue so it can be assessed later when we know what's happening upstream.
          Hide
          Frédéric Massart added a comment -

          Patch looks good Andrew. Feel free to push it for integration. Cheers!

          Show
          Frédéric Massart added a comment - Patch looks good Andrew. Feel free to push it for integration. Cheers!
          Hide
          Andrew Nicols added a comment -

          I'm not going to address the filepicker at this stage as it's not as simple as I first suspected. Although we can make the hideon listen to the escape key at keydown, it doesn't call preventDefault on the event so full screen still exits.

          Rebased on latest branches and submitting for integration.

          Show
          Andrew Nicols added a comment - I'm not going to address the filepicker at this stage as it's not as simple as I first suspected. Although we can make the hideon listen to the escape key at keydown, it doesn't call preventDefault on the event so full screen still exits. Rebased on latest branches and submitting for integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now.

          Could you please update the testing instructions to reflect the areas that have been fixed by this patch. I see filepicker is still listed there.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now. Could you please update the testing instructions to reflect the areas that have been fixed by this patch. I see filepicker is still listed there. Many thanks Sam
          Hide
          Andrew Nicols added a comment -

          Can't see anything in the testing instructions related to filepicker, only in the description.

          Show
          Andrew Nicols added a comment - Can't see anything in the testing instructions related to filepicker, only in the description.
          Hide
          Sam Hemelryk added a comment -

          Haha sorry I must've got my wires mixed up, good thing this wasn't a bomb

          Show
          Sam Hemelryk added a comment - Haha sorry I must've got my wires mixed up, good thing this wasn't a bomb
          Hide
          Aparup Banerjee added a comment -

          took this from Jason. His workstation can't handle anything mac.

          Show
          Aparup Banerjee added a comment - took this from Jason. His workstation can't handle anything mac.
          Hide
          Aparup Banerjee added a comment -

          tested on chrome, ff and safari. wfm, passed

          Show
          Aparup Banerjee added a comment - tested on chrome, ff and safari. wfm, passed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: