Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-36316 Eliminate user-agent sniffing where possible
  3. MDL-39964

Activity completion report should use feature detection rather than browser sniffing

    Details

    • Testing Instructions:
      Hide

      In as many browsers as possible (at least IE>9, IE8, firefox, chrome, mobilesafari)

      1. Enable completion tracking in advanced features
      2. In a course, enable completion tracking in the course settings
      3. An activity to the course
      4. Go to course -> reports > activity completion
      5. VERIFY: no JS errors occur
      6. VERIFY: the text is rotated if the browser supports SVG (IE9+ and all the rest)
      7. VERIFY: the text is not rotated if the browser doesn't support SVG (IE8).
      Show
      In as many browsers as possible (at least IE>9, IE8, firefox, chrome, mobilesafari) Enable completion tracking in advanced features In a course, enable completion tracking in the course settings An activity to the course Go to course -> reports > activity completion VERIFY: no JS errors occur VERIFY: the text is rotated if the browser supports SVG (IE9+ and all the rest) VERIFY: the text is not rotated if the browser doesn't support SVG (IE8).
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-39964-master

      Description

      Browser sniffing is problematic and should be avoiding. Since this feature uses JS to do its work, we should be able to use feature detection. Investigate nad implement.

        Gliffy Diagrams

          Activity

          Hide
          poltawski Dan Poltawski added a comment - - edited

          Yes, assuming its just SVG support, it looks like its as simple as:

          function supportsSvg() {
              return document.implementation.hasFeature("http://www.w3.org/TR/SVG11/feature#BasicStructure", "1.1");
          }
          

          (from http://stackoverflow.com/questions/654112/how-do-you-detect-support-for-vml-or-svg-in-a-browser)

          Show
          poltawski Dan Poltawski added a comment - - edited Yes, assuming its just SVG support, it looks like its as simple as: function supportsSvg() { return document.implementation.hasFeature("http://www.w3.org/TR/SVG11/feature#BasicStructure", "1.1"); } (from http://stackoverflow.com/questions/654112/how-do-you-detect-support-for-vml-or-svg-in-a-browser )
          Hide
          poltawski Dan Poltawski added a comment -

          OK, i've come up with a quick patch for this. I actually think this would be advantageous to backport (but not the function deprecation) because it'll automatically support the right browsers and avoid sniffing problems in the future.

          Ideally report/progress/textrotate.js should be converted to a modern yui module/function at the same time, but its beyond the scope of what I want to do for the time being and it also would not be back-portable.

          Show
          poltawski Dan Poltawski added a comment - OK, i've come up with a quick patch for this. I actually think this would be advantageous to backport (but not the function deprecation) because it'll automatically support the right browsers and avoid sniffing problems in the future. Ideally report/progress/textrotate.js should be converted to a modern yui module/function at the same time, but its beyond the scope of what I want to do for the time being and it also would not be back-portable.
          Hide
          poltawski Dan Poltawski added a comment -

          Sam Marshall if you are able to find some time to look at this it would be good, particularly my assumptions about the feature detection.

          (I've tested it in IE8 and 10 and seems to be right).

          Show
          poltawski Dan Poltawski added a comment - Sam Marshall if you are able to find some time to look at this it would be good, particularly my assumptions about the feature detection. (I've tested it in IE8 and 10 and seems to be right).
          Hide
          quen Sam Marshall added a comment -

          +1 from me for the feature detect if this works in IE8 - code seems fine.

          Note: At some point, this JS should really be revisited with two considerations:

          1. Is it possible to use CSS transforms instead of SVG? (It might not be because I'm not sure how CSS transforms combine with the 'taking up space' aspects).

          2. Remove YUI2 calls

          I'm not necessarily volunteering to do this (unless it becomes urgent), though

          Show
          quen Sam Marshall added a comment - +1 from me for the feature detect if this works in IE8 - code seems fine. Note: At some point, this JS should really be revisited with two considerations: 1. Is it possible to use CSS transforms instead of SVG? (It might not be because I'm not sure how CSS transforms combine with the 'taking up space' aspects). 2. Remove YUI2 calls I'm not necessarily volunteering to do this (unless it becomes urgent), though
          Hide
          quen Sam Marshall added a comment -

          note: by 'works in IE8' I obviously meant, 'correctly detects that the feature isn't available and doesn't use it, without giving a JS error'. Since that's included in test instructions I presume it has already been checked and does work, but just saying.

          Show
          quen Sam Marshall added a comment - note: by 'works in IE8' I obviously meant, 'correctly detects that the feature isn't available and doesn't use it, without giving a JS error'. Since that's included in test instructions I presume it has already been checked and does work, but just saying.
          Hide
          poltawski Dan Poltawski added a comment -

          Thanks Sam, I agree this should be properly converted to YUI3/CSS, but for the time being I think this is a back-portable solution (and will help prevent future bugs like MDL-34446).

          Note: i've only deprecated the function on master (though i'd prefer to be rid of it on all branches).

          Show
          poltawski Dan Poltawski added a comment - Thanks Sam, I agree this should be properly converted to YUI3/CSS, but for the time being I think this is a back-portable solution (and will help prevent future bugs like MDL-34446 ). Note: i've only deprecated the function on master (though i'd prefer to be rid of it on all branches).
          Hide
          poltawski Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Dan - this has been integrated now.
          It will be nice in the future to convert to YUI3 but not required now for sure.

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Dan - this has been integrated now. It will be nice in the future to convert to YUI3 but not required now for sure.
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks for fixing this Dan,

          I can see rotated text on all browsers (ie9, ie10, FF, chrome, opera and safari), except ie8.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Dan, I can see rotated text on all browsers (ie9, ie10, FF, chrome, opera and safari), except ie8.
          Hide
          marina Marina Glancy added a comment -

          Thanks for your awesome work! This has now become a part of Moodle.

          Closing as fixed!

          Show
          marina Marina Glancy added a comment - Thanks for your awesome work! This has now become a part of Moodle. Closing as fixed!
          Hide
          mannes Mannes Brak added a comment -

          This same fixed should be done in /report/completion/textrotate.js

          Show
          mannes Mannes Brak added a comment - This same fixed should be done in /report/completion/textrotate.js
          Hide
          sry_not4sale Aaron Barnes added a comment -

          Hi Mannes,

          That was fixed in MDL-42476

          Cheers,
          Aaron

          Show
          sry_not4sale Aaron Barnes added a comment - Hi Mannes, That was fixed in MDL-42476 Cheers, Aaron

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Jul/13