Moodle
  1. Moodle
  2. MDL-41050

Progress bar size parameter does not work

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      1. Place the attached testprogress.php file in the root of your Moodle installation.

      2. Run the file. The page should show 2 progress bars, one 500 pixels wide and one 700 pixels wide (each with border). Below each progress bar is a box that was manually created with matching width.

      EXPECTED:

      The two boxes are precisely aligned with the progress bars. Both progress bars look correct.

      Show
      1. Place the attached testprogress.php file in the root of your Moodle installation. 2. Run the file. The page should show 2 progress bars, one 500 pixels wide and one 700 pixels wide (each with border). Below each progress bar is a box that was manually created with matching width. EXPECTED: The two boxes are precisely aligned with the progress bars. Both progress bars look correct.
    • Affected Branches:
      MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-41050-master
    • Rank:
      51975

      Description

      The progress bar class has a size parameter which defaults to 500. There is nowhere in Moodle code that uses any value other than 500 for this parameter.

      There are two problems:

      1. The progress bar is centered slightly incorrectly because the size it uses does not take into account 2 pixels of border (i.e. something should be 502, not 500).

      2. The width value is ignored at the key point, meaning the width option doesn't actually change the width of the bar. This seems like rather a flaw in the width option.

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          PHP file for use with test script

          Show
          Sam Marshall added a comment - PHP file for use with test script
          Hide
          Sam Marshall added a comment -

          This is a quite trivial display change and the review should hopefully be pretty obvious, with the test script provided.

          Since this is a trivial fix I made minimal changes to the existing code, rather than attempting to improve it.

          Show
          Sam Marshall added a comment - This is a quite trivial display change and the review should hopefully be pretty obvious, with the test script provided. Since this is a trivial fix I made minimal changes to the existing code, rather than attempting to improve it.
          Hide
          Andrew Davis added a comment -

          Remove the description of the original behaviour from the testing instructions. The tester will only care about the behaviour after the fix.

          Is there anywhere within Moodle that the tester could verify this fix? It would be nice to be able to test this in the environment where it will run.

          Otherwise I think its fine to go for integration.

          Show
          Andrew Davis added a comment - Remove the description of the original behaviour from the testing instructions. The tester will only care about the behaviour after the fix. Is there anywhere within Moodle that the tester could verify this fix? It would be nice to be able to test this in the environment where it will run. Otherwise I think its fine to go for integration.
          Hide
          Sam Marshall added a comment -

          Thanks for review!

          I have modified testing instructions.

          There is nowhere in core Moodle that this can be tested because:

          1. The slightly-incorrect centering is too hard to see without a comparison, such as in my test script.

          2. Everywhere in Moodle uses the 500 pixel width (either by leaving it default, or explicitly specifying it) so it isn't possible to test the incorrect width behaviour.

          Since this meets the review comments I'm now going to submit for integration. Thanks!

          Show
          Sam Marshall added a comment - Thanks for review! I have modified testing instructions. There is nowhere in core Moodle that this can be tested because: 1. The slightly-incorrect centering is too hard to see without a comparison, such as in my test script. 2. Everywhere in Moodle uses the 500 pixel width (either by leaving it default, or explicitly specifying it) so it isn't possible to test the incorrect width behaviour. Since this meets the review comments I'm now going to submit for integration. Thanks!
          Hide
          Sam Marshall added a comment -

          Note: Although this is a bug, so it could theoretically be fixed in 2.4/2.5, it is very minor since I don't think anybody much cares about it being a couple pixels off... I haven't made extra branches but am happy to do so if desired, let me know.

          Show
          Sam Marshall added a comment - Note: Although this is a bug, so it could theoretically be fixed in 2.4/2.5, it is very minor since I don't think anybody much cares about it being a couple pixels off... I haven't made extra branches but am happy to do so if desired, let me know.
          Hide
          Sam Hemelryk added a comment -

          Thanks Sam - this has been integrated. I cherry-picked to stable branches.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Sam - this has been integrated. I cherry-picked to stable branches. Many thanks Sam
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          Tested in 2.4, 2.5 and master.

          I had to modify the test script in 2.4 as html_writer::div() was not defined there. I used a tag of type div instead. The progress bar worked as expected in all versions.

          Show
          Michael de Raadt added a comment - Test result: Success! Tested in 2.4, 2.5 and master. I had to modify the test script in 2.4 as html_writer::div() was not defined there. I used a tag of type div instead. The progress bar worked as expected in all versions.
          Hide
          Sam Marshall added a comment -

          Thanks Michael! Sorry for the broken test script.

          Show
          Sam Marshall added a comment - Thanks Michael! Sorry for the broken test script.
          Hide
          Dan Poltawski added a comment -

          Cảm ơn!

          Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly!

          Một hai ba, yo

          Show
          Dan Poltawski added a comment - Cảm ơn! Your changes have now been merged upstream in git and will be available on the Moodle download sites shortly! Một hai ba, yo

            People

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

              Dates

              • Created:
                Updated:
                Resolved: