Moodle
  1. Moodle
  2. MDL-35840

A typo prevents to correctly impose the focus

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 2.2.5, 2.3.2, 2.4
    • Fix Version/s: STABLE backlog
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide
      1. (Opt) Create an empty course
      2. Add a new SCORM/AICC activity using the attached SCORM 1.2 package, MDL-35840.zip
      3. Launch the Activity: at the very first attempt item "1.1" will be selected and highlighted
      4. Move the focus on another already opened program windows (e.g.: in Windows, ALT+TAB) and get back: the current item must be highlighted
      5. (Opt) Navigate other items changing the focus back and forth
        Note: you can click on other components of the SCORM activity, e.g. the content: to get the focus back you need to click within the scorm_toc component area.

      General note: module.js is injected into the window by the Moodle framework. Such an injection is not linked to the $version coming from version.php so it is always required to clean first your browser cache to be sure that you'll run the current (new) version of that file.

      Show
      (Opt) Create an empty course Add a new SCORM/AICC activity using the attached SCORM 1.2 package, MDL-35840 .zip Launch the Activity: at the very first attempt item "1.1" will be selected and highlighted Move the focus on another already opened program windows (e.g.: in Windows, ALT+TAB) and get back: the current item must be highlighted (Opt) Navigate other items changing the focus back and forth Note: you can click on other components of the SCORM activity, e.g. the content: to get the focus back you need to click within the scorm_toc component area. General note: module.js is injected into the window by the Moodle framework. Such an injection is not linked to the $version coming from version.php so it is always required to clean first your browser cache to be sure that you'll run the current (new) version of that file.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m24_MDL-35840_SCORM_TOC_focus_fix
    • Rank:
      44603

      Description

      mod/scorm/module.js: left.expanded should be left.expand.
      Besides if the user moves away from the browser instance and then get back, the focus on the TOC item is lost.

      Found on working for MDL-33106.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          Starting from the work already done for MDL-33106, the code below will be proposed for 2.3 (maybe 2.2 too) and 2.4 (MASTER):

          1. https://github.com/scara/moodle/commit/ee56ee9329ce114a27535e96515827c8a4f11a9e#L0R154
          2. https://github.com/scara/moodle/commit/ee56ee9329ce114a27535e96515827c8a4f11a9e#L0R366

          Since 2.4 is near to code freeze and hoping that MDL-35418 will land there too, the patch proposal for 2.4 will be done against the branch master_MDL-35418 from git://github.com/danmarsden/moodle.git.

          Show
          Matteo Scaramuccia added a comment - Starting from the work already done for MDL-33106 , the code below will be proposed for 2.3 (maybe 2.2 too) and 2.4 (MASTER): https://github.com/scara/moodle/commit/ee56ee9329ce114a27535e96515827c8a4f11a9e#L0R154 https://github.com/scara/moodle/commit/ee56ee9329ce114a27535e96515827c8a4f11a9e#L0R366 Since 2.4 is near to code freeze and hoping that MDL-35418 will land there too, the patch proposal for 2.4 will be done against the branch master_ MDL-35418 from git://github.com/danmarsden/moodle.git .
          Hide
          Matteo Scaramuccia added a comment -

          Patch for master has been provided as an attached patch file built against Dan's master_MDL-35418 branch: m24_MDL-35840SCORM_TOC_focus_fix_DONE_OVER_masterMDL-35418.diff.

          commit 024ad28e0d9eecf35417b7a3682e7be2e5f74486
          Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com>
          Date:   Sun Oct 7 23:35:31 2012 +0200
          
              MDL-35840 SCORM TOC: fixed a typo, improvements on getting the focus
          
          diff --git a/mod/scorm/module.js b/mod/scorm/module.js
          index 16ca7f5..e9a5a07 100644
          --- a/mod/scorm/module.js
          +++ b/mod/scorm/module.js
          @@ -152,7 +152,7 @@ M.mod_scorm.init = function(Y, hide_nav, hide_toc, toc_title, window_name, launc
                       scorm_resize_frame();
          
                       var left = scorm_layout_widget.getUnitByPosition('left');
          -            if (left.expanded) {
          +            if (left.expand) {
                           scorm_current_node.focus();
                       }
                       if (scorm_hide_nav == false) {
          @@ -493,6 +493,15 @@ M.mod_scorm.init = function(Y, hide_nav, hide_toc, toc_title, window_name, launc
                   tree.expandAll();
                   tree.render();
          
          +        // On getting the window, always set the focus on the current item
          +        YAHOO.util.Event.on(window, 'focus', function (e) {
          +            var current = scorm_tree_node.getHighlightedNode();
          +            var left = scorm_layout_widget.getUnitByPosition('left');
          +            if (current && left.expand) {
          +                current.focus();
          +            }
          +        });
          +
                   // navigation
                   if (scorm_hide_nav == false) {
                       scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible:true, draggable:true, close:false, xy: [250, 450],
          
          Show
          Matteo Scaramuccia added a comment - Patch for master has been provided as an attached patch file built against Dan's master_ MDL-35418 branch: m24_ MDL-35840 SCORM_TOC_focus_fix_DONE_OVER_master MDL-35418 .diff. commit 024ad28e0d9eecf35417b7a3682e7be2e5f74486 Author: Matteo Scaramuccia <moodle@matteoscaramuccia.com> Date: Sun Oct 7 23:35:31 2012 +0200 MDL-35840 SCORM TOC: fixed a typo, improvements on getting the focus diff --git a/mod/scorm/module.js b/mod/scorm/module.js index 16ca7f5..e9a5a07 100644 --- a/mod/scorm/module.js +++ b/mod/scorm/module.js @@ -152,7 +152,7 @@ M.mod_scorm.init = function(Y, hide_nav, hide_toc, toc_title, window_name, launc scorm_resize_frame(); var left = scorm_layout_widget.getUnitByPosition('left'); - if (left.expanded) { + if (left.expand) { scorm_current_node.focus(); } if (scorm_hide_nav == false ) { @@ -493,6 +493,15 @@ M.mod_scorm.init = function(Y, hide_nav, hide_toc, toc_title, window_name, launc tree.expandAll(); tree.render(); + // On getting the window, always set the focus on the current item + YAHOO.util.Event.on(window, 'focus', function (e) { + var current = scorm_tree_node.getHighlightedNode(); + var left = scorm_layout_widget.getUnitByPosition('left'); + if (current && left.expand) { + current.focus(); + } + }); + // navigation if (scorm_hide_nav == false ) { scorm_nav_panel = new Y.YUI2.widget.Panel('scorm_navpanel', { visible: true , draggable: true , close: false , xy: [250, 450],
          Hide
          Dan Marsden added a comment -

          thanks Matteo

          Show
          Dan Marsden added a comment - thanks Matteo
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          if you want I'll create a new pull branch for 2.4: as you've already read, MDL-35418 has finally landed into the main stream

          Show
          Matteo Scaramuccia added a comment - Hi Dan, if you want I'll create a new pull branch for 2.4: as you've already read, MDL-35418 has finally landed into the main stream
          Hide
          Dan Marsden added a comment -

          sure - I'll push this up for integration as soon as that's done (although I don't expect any integration to happen next week as all the HQ devs are at the hackfest)

          Show
          Dan Marsden added a comment - sure - I'll push this up for integration as soon as that's done (although I don't expect any integration to happen next week as all the HQ devs are at the hackfest)
          Hide
          Matteo Scaramuccia added a comment -

          My git patch attached for 2.4 conflicts with new '2in3' logic: see MDL-34741 for details.

          Show
          Matteo Scaramuccia added a comment - My git patch attached for 2.4 conflicts with new '2in3' logic: see MDL-34741 for details.
          Hide
          Matteo Scaramuccia added a comment -

          I've added another commit into the Pull Master Branch: the improvement done at the time of proposing a fix for MDL-33106 - the one to search the candidate tree node serving the launch_sco, at that time just a code cleanup - is now required for 2.4 to avoid the user to see the very first tree node highlighted while serving another item.

          Show
          Matteo Scaramuccia added a comment - I've added another commit into the Pull Master Branch : the improvement done at the time of proposing a fix for MDL-33106 - the one to search the candidate tree node serving the launch_sco , at that time just a code cleanup - is now required for 2.4 to avoid the user to see the very first tree node highlighted while serving another item.
          Hide
          Matteo Scaramuccia added a comment - - edited

          In the Pull Master branch I've made some more fixes and improvements to MDL-35418 too, here because of MDL-35840 has highligthed the issues I've fixed.
          @Dan: feel free to make a selection from my commits (required: the first 3 commits). I know I should open a separate issue for some of them.

          Show
          Matteo Scaramuccia added a comment - - edited In the Pull Master branch I've made some more fixes and improvements to MDL-35418 too, here because of MDL-35840 has highligthed the issues I've fixed. @Dan: feel free to make a selection from my commits (required: the first 3 commits). I know I should open a separate issue for some of them.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          I'm just reviewing my own work and probably https://github.com/scara/moodle/commit/2171a6f5df451c52d1da9b4503587feb0799c8e6 is too much since it could potentially create regressions since activities (SCO) are actually pushed through the current node by changing its title (the URL, regardless if it represents the parent node and not the real node pointing to the launched resource) i.e. we should review the whole of the new code before preventing things to act as expected from my POV (no API for an item w/o resource as well as no load attempt + activate the 'real' node, easy to do by moving the candidate nodes discovery within scorm_activate_item()): my goal yesterday was to avoid the browser to do things not useful but I think it should be better to remove that part from my proposal for this issue, 35840.

          I'm available to review the proposal by:
          a. adding a new commit which reverts the behaviour back to the original one
          b. dropping the branch to re-create a new one excluding that commit
          c. squashing everything in just one commit, after applying (a)
          d. whatever you think to be better, even dropping my attempt to review the indentation

          Let me know,
          Matteo

          Show
          Matteo Scaramuccia added a comment - Hi Dan, I'm just reviewing my own work and probably https://github.com/scara/moodle/commit/2171a6f5df451c52d1da9b4503587feb0799c8e6 is too much since it could potentially create regressions since activities (SCO) are actually pushed through the current node by changing its title (the URL, regardless if it represents the parent node and not the real node pointing to the launched resource) i.e. we should review the whole of the new code before preventing things to act as expected from my POV (no API for an item w/o resource as well as no load attempt + activate the 'real' node, easy to do by moving the candidate nodes discovery within scorm_activate_item() ): my goal yesterday was to avoid the browser to do things not useful but I think it should be better to remove that part from my proposal for this issue, 35840. I'm available to review the proposal by: a. adding a new commit which reverts the behaviour back to the original one b. dropping the branch to re-create a new one excluding that commit c. squashing everything in just one commit, after applying (a) d. whatever you think to be better, even dropping my attempt to review the indentation Let me know, Matteo
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note that MDL-37844 has "picked" some of the changes proposed here (for 24_STABLE and master), so surely this will need rebase/amending for those branches.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Note that MDL-37844 has "picked" some of the changes proposed here (for 24_STABLE and master), so surely this will need rebase/amending for those branches. Thanks!
          Hide
          Matteo Scaramuccia added a comment -

          TNX Eloy for the reminder: I'm just closing this issue (another -1 for the techy debt of SCORM ) since my proposal has been outdated by the changes in 2.4 and Dan has already applied the fix for the focus.
          I've somehow applied the experience made here now in MDL-37529.

          Show
          Matteo Scaramuccia added a comment - TNX Eloy for the reminder: I'm just closing this issue (another -1 for the techy debt of SCORM ) since my proposal has been outdated by the changes in 2.4 and Dan has already applied the fix for the focus. I've somehow applied the experience made here now in MDL-37529 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: