Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41050

Progress bar size parameter does not work

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            quen Sam Marshall added a comment -

            PHP file for use with test script

            Show
            quen Sam Marshall added a comment - PHP file for use with test script
            Hide
            quen 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
            quen 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
            andyjdavis 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
            andyjdavis 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
            quen 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
            quen 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
            quen 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
            quen 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
            samhemelryk Sam Hemelryk added a comment -

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

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Sam - this has been integrated. I cherry-picked to stable branches. Many thanks Sam
            Hide
            salvetore 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
            salvetore 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
            quen Sam Marshall added a comment -

            Thanks Michael! Sorry for the broken test script.

            Show
            quen Sam Marshall added a comment - Thanks Michael! Sorry for the broken test script.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  9/Sep/13