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

    • Rank:
      50704

      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.

        Activity

        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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 Glancy added a comment -

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

        Closing as fixed!

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

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

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

        Hi Mannes,

        That was fixed in MDL-42476

        Cheers,
        Aaron

        Show
        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: