Details

    • Testing Instructions:
      Hide
      1. Flush your cache
      2. Create a new quiz
      3. Make sure the divs containing the Save and Cancel buttons do not contain an empty label, and that the buttons look nice in the page
      4. Navigate to 'Settings > Site administration > Users > Add a user'
      5. Make sure the divs containing the 'Update profile' button does not contain an empty label, and that the button look nice in the page
      6. Navigate to 'Settings > Course administration > Edit settings'
      7. Make sure the divs containing the Cancel and Save button do not contain an empty label, and that the buttons look nice in the page
      8. Repeat with a few other themes.
      Show
      Flush your cache Create a new quiz Make sure the divs containing the Save and Cancel buttons do not contain an empty label, and that the buttons look nice in the page Navigate to 'Settings > Site administration > Users > Add a user' Make sure the divs containing the 'Update profile' button does not contain an empty label, and that the button look nice in the page Navigate to 'Settings > Course administration > Edit settings' Make sure the divs containing the Cancel and Save button do not contain an empty label, and that the buttons look nice in the page Repeat with a few other themes.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30844-master

      Description

      Frequently near the bottoms of pages with input forms there is an orphaned form label with no content and not attached to an input element. This is probably from a loop iterating one time too many when creating the page. Most Web page accessibility tools will easily identify the pages where this problem exists. Depending on if the problem is in a common library or if it exists in lots of different places it might be necessary to break this into sub-tasks.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Greg Kraus added a comment -

            I'll provide some example pages after I enter the rest of the bugs from the report.

            Show
            Greg Kraus added a comment - I'll provide some example pages after I enter the rest of the bugs from the report.
            Hide
            Greg Kraus added a comment -

            None of the assistive technologies I tested actually had problems with this, but it would be nice to clean up the code here.

            Show
            Greg Kraus added a comment - None of the assistive technologies I tested actually had problems with this, but it would be nice to clean up the code here.
            Hide
            Michael de Raadt added a comment -

            Hi, Greg.

            Yes, it would be good to see an example of this.

            Show
            Michael de Raadt added a comment - Hi, Greg. Yes, it would be good to see an example of this.
            Hide
            Greg Kraus added a comment -

            When adding a new file resource, near the bottom of the page you will find the following code.

            <div class="fitem"><div class="fitemtitle"><div class="fgrouplabel"><label> </label></div></div><fieldset class="felement fgroup"><input type="submit" id="id_submitbutton2" value="Save and return to course" name="submitbutton2"> <input type="submit" id="id_submitbutton" value="Save and display" name="submitbutton"> <input type="submit" id="id_cancel" onclick="skipClientValidation = true; return true;" value="Cancel" name="cancel"></fieldset></div>

            Note the "<label> </label>" about a quarter of the way into the line of code. This same pattern seems to exist at the bottom of all of the "add a resource/activity" pages.

            Show
            Greg Kraus added a comment - When adding a new file resource, near the bottom of the page you will find the following code. <div class="fitem"><div class="fitemtitle"><div class="fgrouplabel"><label> </label></div></div><fieldset class="felement fgroup"><input type="submit" id="id_submitbutton2" value="Save and return to course" name="submitbutton2"> <input type="submit" id="id_submitbutton" value="Save and display" name="submitbutton"> <input type="submit" id="id_cancel" onclick="skipClientValidation = true; return true;" value="Cancel" name="cancel"></fieldset></div> Note the "<label> </label>" about a quarter of the way into the line of code. This same pattern seems to exist at the bottom of all of the "add a resource/activity" pages.
            Hide
            Frédéric Massart added a comment -

            Note to reviewer: I noticed that the 'template' used within the form helper did not allow me to remove the label. I first tried to set a label and hide it with CSS, but that did not work really smoothly and did not make much sense. I then decided to add a new template for the action buttons, which would be used when a group only contains submit buttons. I made the assumption that we would not need a label when a group only contains submit elements. Please tell me what you think about this. I will create the 2.1 and 2.2 branches once the code is validated. Thanks!

            Show
            Frédéric Massart added a comment - Note to reviewer: I noticed that the 'template' used within the form helper did not allow me to remove the label. I first tried to set a label and hide it with CSS, but that did not work really smoothly and did not make much sense. I then decided to add a new template for the action buttons, which would be used when a group only contains submit buttons. I made the assumption that we would not need a label when a group only contains submit elements. Please tell me what you think about this. I will create the 2.1 and 2.2 branches once the code is validated. Thanks!
            Hide
            Dan Poltawski added a comment -

            Adding Tim as watcher.

            Show
            Dan Poltawski added a comment - Adding Tim as watcher.
            Hide
            Rajesh Taneja added a comment -

            Thanks Fred,

            Code looks good, but I am not sure how it's going to affect themes/JS (if it's used in some way). I think Tim is the right person to review this.

            Show
            Rajesh Taneja added a comment - Thanks Fred, Code looks good, but I am not sure how it's going to affect themes/JS (if it's used in some way). I think Tim is the right person to review this.
            Hide
            Tim Hunt added a comment -

            The theme point is a good one. How do themes achieve their layout of forms? If the plan is float-left the label, fixed width, with the actual form field float-left beside it, then this change would alter the layout.

            Except ...

            What I say about how it is supposed to work is true. However, when the label is empty, it gets a hight of 0px, so actually it does not push the buttons to the right, at least not in the theme I am looking at. So, your change of code will not actually change the layout of the forms, although it will change how I assume they are supposed to be laid out.

            I will let you decide what to do about this.

            I will just say, well done for spotting my evil hack in comment.php, and fixing that too.

            Show
            Tim Hunt added a comment - The theme point is a good one. How do themes achieve their layout of forms? If the plan is float-left the label, fixed width, with the actual form field float-left beside it, then this change would alter the layout. Except ... What I say about how it is supposed to work is true. However, when the label is empty, it gets a hight of 0px, so actually it does not push the buttons to the right, at least not in the theme I am looking at. So, your change of code will not actually change the layout of the forms, although it will change how I assume they are supposed to be laid out. I will let you decide what to do about this. I will just say, well done for spotting my evil hack in comment.php, and fixing that too.
            Hide
            Frédéric Massart added a comment -

            Raj, what do you think?

            Show
            Frédéric Massart added a comment - Raj, what do you think?
            Hide
            Rajesh Taneja added a comment -

            Fred,
            I think before you push it for integration, please make sure button alignment is not disturbed because of this change. Try check this on different themes and feel free to push it for integration if it looks fine.

            Show
            Rajesh Taneja added a comment - Fred, I think before you push it for integration, please make sure button alignment is not disturbed because of this change. Try check this on different themes and feel free to push it for integration if it looks fine.
            Hide
            Frédéric Massart added a comment -

            I have tested over a bunch of different themes and everything looks fine. I have worked on this issue a few weeks ago, and now remember that the label does not push the form elements to the right, there is a margin set on the fields containers to create the alignment. So removing this empty label should not change anything, except eventually for themes which work totally differently.

            I rebased this, and am pushing it for integration. No branching for 2.x as it contains HTML changes.

            Show
            Frédéric Massart added a comment - I have tested over a bunch of different themes and everything looks fine. I have worked on this issue a few weeks ago, and now remember that the label does not push the form elements to the right, there is a margin set on the fields containers to create the alignment. So removing this empty label should not change anything, except eventually for themes which work totally differently. I rebased this, and am pushing it for integration. No branching for 2.x as it contains HTML changes.
            Hide
            Sam Hemelryk added a comment -

            I've just added the integration_held label and removed this from the current integration run sorry.
            It appears this is a master only change and as we are keeping 23 and master in sync has been delayed until after the minor release this Friday.

            My apologies for any inconvenience caused.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - I've just added the integration_held label and removed this from the current integration run sorry. It appears this is a master only change and as we are keeping 23 and master in sync has been delayed until after the minor release this Friday. My apologies for any inconvenience caused. Cheers Sam
            Hide
            Dan Poltawski added a comment -

            Integrated, thanks

            Show
            Dan Poltawski added a comment - Integrated, thanks
            Hide
            Adrian Greeve added a comment -

            I tested this in master first to get an understanding of the issue, then I tested the integration version. Orphaned form labels are no longer present and the buttons do not look out of place.
            Test passed

            Show
            Adrian Greeve added a comment - I tested this in master first to get an understanding of the issue, then I tested the integration version. Orphaned form labels are no longer present and the buttons do not look out of place. Test passed
            Hide
            Dan Poltawski added a comment -

            Congratulations!

            You've made it into the weekly release!

            Thanks for your contribution - here are some random drummers to keep you inspired for the next week!
            http://www.youtube.com/watch?v=_QhpHUmVCmY

            Show
            Dan Poltawski added a comment - Congratulations! You've made it into the weekly release! Thanks for your contribution - here are some random drummers to keep you inspired for the next week! http://www.youtube.com/watch?v=_QhpHUmVCmY
            Hide
            Tim Hunt added a comment -

            This bug dealt with bogus labels on submits. It seems that the same problem exists for 'static' form elements (e.g. MDL-35968). Does anyone know if any work has been done on that? Thanks.

            Show
            Tim Hunt added a comment - This bug dealt with bogus labels on submits. It seems that the same problem exists for 'static' form elements (e.g. MDL-35968 ). Does anyone know if any work has been done on that? Thanks.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: