Moodle
  1. Moodle
  2. MDL-34446

The activity completion table does not display well in IE and Safari.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5
    • Fix Version/s: 2.3.2
    • Component/s: Activity completion
    • Labels:
    • Testing Instructions:
      Hide

      1. Set up a course or use existing one:
      a. Activity completion enabled
      b. At least 2 activities that have completion turned on
      c. At least one enrolled user

      2. Go to Reports/Activity completion in the following browsers:
      a. MSIE
      b. Firefox (2+)
      c. Chrome
      d. Safari
      e. Opera
      f. Safari on iOS

      In all cases, using the latest browser version, the activity names should now display sideways.

      Show
      1. Set up a course or use existing one: a. Activity completion enabled b. At least 2 activities that have completion turned on c. At least one enrolled user 2. Go to Reports/Activity completion in the following browsers: a. MSIE b. Firefox (2+) c. Chrome d. Safari e. Opera f. Safari on iOS In all cases, using the latest browser version, the activity names should now display sideways.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      MDL-34446-master
    • Rank:
      42849

      Description

      The activity completion table does not display well in IE and Safari. The activities are show horizontally rather than vertically and there is no scroll bar on the top of each page for teachers to scroll to the last item of all activities. A scroll bar on top is useful when they are large number of students (in the 100s).

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Felicia.

          Are you requesting a scroll bar at the top?

          Can you provide a screenshot of what you are seeing and provide some more details about why you think it should change?

          Show
          Michael de Raadt added a comment - Hi, Felicia. Are you requesting a scroll bar at the top? Can you provide a screenshot of what you are seeing and provide some more details about why you think it should change?
          Hide
          Michael de Raadt added a comment -

          I assume that this affects currently supported versions (2.2+), but it would be good to confirm this.

          Show
          Michael de Raadt added a comment - I assume that this affects currently supported versions (2.2+), but it would be good to confirm this.
          Hide
          Sam Marshall added a comment -

          This code was originally written when Firefox was on version 2 and SVG support was cool and new. Now, current versions of most browsers support SVG. I have changed the can_use_rotated_text function so that it checks browser version corresponding to all browsers I tested in (which was a lot, see test instructions).

          Note that I don't think the way this is implemented is very good. This is a minimal bugfix to correct the problem; I don't think that function should really exist, and perhaps it would also be better if it didn't use SVG for this purpose, but I don't have time to do a significant rewrite.

          Show
          Sam Marshall added a comment - This code was originally written when Firefox was on version 2 and SVG support was cool and new. Now, current versions of most browsers support SVG. I have changed the can_use_rotated_text function so that it checks browser version corresponding to all browsers I tested in (which was a lot, see test instructions). Note that I don't think the way this is implemented is very good. This is a minimal bugfix to correct the problem; I don't think that function should really exist, and perhaps it would also be better if it didn't use SVG for this purpose, but I don't have time to do a significant rewrite.
          Hide
          Sam Marshall added a comment -

          Submitting for integration review.

          Show
          Sam Marshall added a comment - Submitting for integration review.
          Hide
          Dan Poltawski added a comment -

          Well, I also don't like this approach. But I have taken this comment on board:

          This is a minimal bugfix to correct the problem; I don't think that function should really exist, and perhaps it would also be better if it didn't use SVG for this purpose, but I don't have time to do a significant rewrite.

          So we have a minor improvement here. Really I think we should be eliminating all browser sniffing we can from moodle and doing feature detection instead.

          Show
          Dan Poltawski added a comment - Well, I also don't like this approach. But I have taken this comment on board: This is a minimal bugfix to correct the problem; I don't think that function should really exist, and perhaps it would also be better if it didn't use SVG for this purpose, but I don't have time to do a significant rewrite. So we have a minor improvement here. Really I think we should be eliminating all browser sniffing we can from moodle and doing feature detection instead.
          Hide
          Dan Poltawski added a comment -

          I've integrated this to 2.3 and master. Thanks Sam.

          Show
          Dan Poltawski added a comment - I've integrated this to 2.3 and master. Thanks Sam.
          Hide
          Tim Barker added a comment -

          I tested this and there is an overall improvement. FF, Chrome and IE9, all display vertically as required. Sadly, Opera and Safari, latest versions don't, they still display horizontally. I also tested IE8 and it also displays horizontally. I'm failing it because the victory conditions have not been met.

          Show
          Tim Barker added a comment - I tested this and there is an overall improvement. FF, Chrome and IE9, all display vertically as required. Sadly, Opera and Safari, latest versions don't, they still display horizontally. I also tested IE8 and it also displays horizontally. I'm failing it because the victory conditions have not been met.
          Hide
          Dan Poltawski added a comment -

          So I tried this in Safari and got false:

          <?php
          
          require_once('config.php');
          
          var_dump(check_browser_version('Safari', 536.26));
          
          Show
          Dan Poltawski added a comment - So I tried this in Safari and got false: <?php require_once('config.php'); var_dump(check_browser_version('Safari', 536.26));
          Hide
          Sam Marshall added a comment -

          I'll look into this later today.

          Show
          Sam Marshall added a comment - I'll look into this later today.
          Hide
          Sam Marshall added a comment -

          Okay, I have a new version that resolves both the problems by fixing:

          1. check_browser_version doesn't work for Opera version 10+
          2. WebKit versions always have a decimal point in, but check_browser_version doesn't work if you specify the required version with a point in.
          3. I screwed up and made a typo in the Safari version (it's 536.25).

          So far, I have made a new commit just for this and pushed it to my branch (note: I also rebased during testing):

          https://github.com/sammarshallou/moodle/commit/9dd5c999c114d2c29bc3d32328bf90feea6d18d4

          Not sure what the procedure is, should I rebase it back into a single commit and resubmit or something?

          Show
          Sam Marshall added a comment - Okay, I have a new version that resolves both the problems by fixing: 1. check_browser_version doesn't work for Opera version 10+ 2. WebKit versions always have a decimal point in, but check_browser_version doesn't work if you specify the required version with a point in. 3. I screwed up and made a typo in the Safari version (it's 536.25). So far, I have made a new commit just for this and pushed it to my branch (note: I also rebased during testing): https://github.com/sammarshallou/moodle/commit/9dd5c999c114d2c29bc3d32328bf90feea6d18d4 Not sure what the procedure is, should I rebase it back into a single commit and resubmit or something?
          Hide
          Dan Poltawski added a comment -

          Hmm, just noticed we have unit tests for the brwoser versions check. Ideally we'd update this for the new user agents.

          Show
          Dan Poltawski added a comment - Hmm, just noticed we have unit tests for the brwoser versions check. Ideally we'd update this for the new user agents.
          Hide
          Dan Poltawski added a comment -

          I've integrated this. We didn't break anything, ideally we'd add tests for this fix, but i'm leaving that for now.

          (btw I seem to have screwed up the fix version before, its only in master. sorry about that)

          Show
          Dan Poltawski added a comment - I've integrated this. We didn't break anything, ideally we'd add tests for this fix, but i'm leaving that for now. (btw I seem to have screwed up the fix version before, its only in master. sorry about that)
          Hide
          Dan Poltawski added a comment -

          Oh no I didn't. I had the fix version right, doh!

          Show
          Dan Poltawski added a comment - Oh no I didn't. I had the fix version right, doh!
          Hide
          Dan Poltawski added a comment -

          I've just tested on opera and safari and all good now.

          Show
          Dan Poltawski added a comment - I've just tested on opera and safari and all good now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility!

          Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - For the good and the bad... this is now part of Moodle and people around the world will start using it immediately, what a responsibility! Many thanks for your collaboration, yay! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: