Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.1
    • Fix Version/s: None
    • Labels:
      None
    • Testing Instructions:
      Hide

      This changes effected the entire select option and input text which created through html_writer, single_select() or html tag.

      The best way to test this issue is by viewing the html source (eg: firebug) for select option and input text.

      For the following attempts, make sure there's no error display and select/input text is properly labeled.

      #. create and attempt a quiz within the course. Access the stats reports for the quiz and make sure there is no error display.
      #. on page site admin>reports>log
      #. on page site admin>plugins>filters>manage filters
      #. grader report for the course, somecourse>grade
      #. any other page that use select option or input text.

      Show
      This changes effected the entire select option and input text which created through html_writer, single_select() or html tag. The best way to test this issue is by viewing the html source (eg: firebug) for select option and input text. For the following attempts, make sure there's no error display and select/input text is properly labeled. #. create and attempt a quiz within the course. Access the stats reports for the quiz and make sure there is no error display. #. on page site admin>reports>log #. on page site admin>plugins>filters>manage filters #. grader report for the course, somecourse>grade #. any other page that use select option or input text.
    • Affected Branches:
      MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30843_accessibility
    • Rank:
      33843

      Description

      There are several select input fields that do not have labels explicitly tied to them. Often this is because there is a visual cue as to what information is being asked for but the visual cue is not explicitly linked to the input element.

      Because this problem is sporadic it might be necessary to break this task out into smaller sub-tasks for each instance of the problem.

      Here is a tutorial for methods of labeling text input elements. http://oit.ncsu.edu/itaccess/forms#select

      1. mdl-30843-review.txt
        12 kB
        Glenn Ansley
      1. label_accessibility.png
        146 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Greg.

          Unfortunately we cannot have sub-tasks of sub-tasks. Adding the individual instances as sub-tasks of the greater meta-issue will be fine.

          Show
          Michael de Raadt added a comment - Hi, Greg. Unfortunately we cannot have sub-tasks of sub-tasks. Adding the individual instances as sub-tasks of the greater meta-issue will be fine.
          Hide
          Michael Penney added a comment -

          This issue is marked as affects version 2.1.1 - does it also affect 2.2.x?

          Show
          Michael Penney added a comment - This issue is marked as affects version 2.1.1 - does it also affect 2.2.x?
          Hide
          Rossiani Wijaya added a comment -

          Side note: in some files, the input text and select are not accompanied by label directly because soemtimes the caller function provide the label tag and call other function just to retrieve the input text or select.

          Submitting this for peer review.

          Show
          Rossiani Wijaya added a comment - Side note: in some files, the input text and select are not accompanied by label directly because soemtimes the caller function provide the label tag and call other function just to retrieve the input text or select. Submitting this for peer review.
          Hide
          Rossiani Wijaya added a comment -

          Hi Glenn,

          I'm assigning you as peer-review. Feel free to re-assign this issue to other.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Glenn, I'm assigning you as peer-review. Feel free to re-assign this issue to other. Thanks Rosie
          Hide
          Glenn Ansley added a comment -

          Wow. That is a huge diff. I guess future peer reviews shouldn't intimidate me if I can get through this as my first one.

          I'm not sure how all in you guys go when it comes to coding guidelines. So, at the risk of being way to thorough, I'm attaching my review of the code. There's a lot of information in there. Much of it has to do with the length of the strings but there are also some whitespace issues and a couple lingering strings that look like trace code.

          In addition to the file, I also created a script that performed a PHP litmus test on every file and it passed. I though this was a good idea since there were so many files and I could see myself committing at least one syntax in there. You came through clean though.

          Again, this was my first ever peer review so I probably went into too much detail in some spots and missed something obvious elsewhere. If you or anyone else at HQ has feedback, I'd more than welcome it.

          Show
          Glenn Ansley added a comment - Wow. That is a huge diff. I guess future peer reviews shouldn't intimidate me if I can get through this as my first one. I'm not sure how all in you guys go when it comes to coding guidelines. So, at the risk of being way to thorough, I'm attaching my review of the code. There's a lot of information in there. Much of it has to do with the length of the strings but there are also some whitespace issues and a couple lingering strings that look like trace code. In addition to the file, I also created a script that performed a PHP litmus test on every file and it passed. I though this was a good idea since there were so many files and I could see myself committing at least one syntax in there. You came through clean though. Again, this was my first ever peer review so I probably went into too much detail in some spots and missed something obvious elsewhere. If you or anyone else at HQ has feedback, I'd more than welcome it.
          Hide
          Glenn Ansley added a comment -

          attached the file i completed.

          Show
          Glenn Ansley added a comment - attached the file i completed.
          Hide
          Greg Kraus added a comment -

          I'm confused about what is actually going on here. When I look at the code the current diff generates I see something like this.

          (from enrol/users.php?id=2)

          <label for="single_select4f47de1e3f6465">Enrolment methods</label>
          <label class="accesshide" for="menuifilter">0</label>
          <select id="single_select4f47de1e3f6465" class="select menuifilter" name="ifilter">
          <option value="0" selected="selected">All</option>
          <option value="1">Manual enrolments</option>
          </select>

          In this case, the select element was actually labelled correctly before the diff was ever applied, and now there is a new <label class="accesshide" for="menuifilter">0</label> that has numerous errors with it. This is a case where nothing actually needed to change.

          Also, I don't see any labels for any of the select menus on the main course pages for "add a resource" or "add an activity".

          Show
          Greg Kraus added a comment - I'm confused about what is actually going on here. When I look at the code the current diff generates I see something like this. (from enrol/users.php?id=2) <label for="single_select4f47de1e3f6465">Enrolment methods</label> <label class="accesshide" for="menuifilter">0</label> <select id="single_select4f47de1e3f6465" class="select menuifilter" name="ifilter"> <option value="0" selected="selected">All</option> <option value="1">Manual enrolments</option> </select> In this case, the select element was actually labelled correctly before the diff was ever applied, and now there is a new <label class="accesshide" for="menuifilter">0</label> that has numerous errors with it. This is a case where nothing actually needed to change. Also, I don't see any labels for any of the select menus on the main course pages for "add a resource" or "add an activity".
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Thank you for noticing double label for the enrol/users. I created double label within the output renderer for single select input.

          I checked the label for add resource and activity select and it is display above the helplink span tag (attaching screenshot of the page source).

          Updating patch for enrol/users page.

          Show
          Rossiani Wijaya added a comment - Hi Greg, Thank you for noticing double label for the enrol/users. I created double label within the output renderer for single select input. I checked the label for add resource and activity select and it is display above the helplink span tag (attaching screenshot of the page source). Updating patch for enrol/users page.
          Hide
          Adrian Greeve added a comment -

          Hi Rossie. I noticed that in the Grader report the jump menu doesn't have a label. If you could include that into this fix, that would be wonderful.
          Many thanks.

          Show
          Adrian Greeve added a comment - Hi Rossie. I noticed that in the Grader report the jump menu doesn't have a label. If you could include that into this fix, that would be wonderful. Many thanks.
          Hide
          Rossiani Wijaya added a comment -

          Hi Adrian,

          Thank you for reporting.

          Patch updated.

          Show
          Rossiani Wijaya added a comment - Hi Adrian, Thank you for reporting. Patch updated.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          When you have a chance, could you please peer review the patch again?

          Thanks.

          Show
          Rossiani Wijaya added a comment - Hi Greg, When you have a chance, could you please peer review the patch again? Thanks.
          Hide
          Michael de Raadt added a comment -

          Carrying over to the new sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the new sprint.
          Hide
          Greg Kraus added a comment -

          The input elements are labelled now for the grader report and also for the enrollment method. The "add a resource" and "add an activity" inputs are labelled too, so this looks good. I'm not sure if there are other spots to check, but the formula you are using is working.

          The only issue I noticed, and it's fairly minor, is on the main course page the label for each of the "add a resource" and "add an activity" inputs is the same, saying simply "add a resource ..." or "add an activity ...". It would be a bit more descriptive to have the label as "add a resource to [topic/weekly block title goes here]". I'm not sure how difficult that is to pull off with the current code changes.

          Show
          Greg Kraus added a comment - The input elements are labelled now for the grader report and also for the enrollment method. The "add a resource" and "add an activity" inputs are labelled too, so this looks good. I'm not sure if there are other spots to check, but the formula you are using is working. The only issue I noticed, and it's fairly minor, is on the main course page the label for each of the "add a resource" and "add an activity" inputs is the same, saying simply "add a resource ..." or "add an activity ...". It would be a bit more descriptive to have the label as "add a resource to [topic/weekly block title goes here] ". I'm not sure how difficult that is to pull off with the current code changes.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Thank you for reviewing this.

          As your request to change the label for "add a resource" and "add an activity", I will create new issue to address that.

          Patch updated and sending this for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Greg, Thank you for reviewing this. As your request to change the label for "add a resource" and "add an activity", I will create new issue to address that. Patch updated and sending this for integration review.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated (yesterday) 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
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated (yesterday) 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 Rossi, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Rossi, this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Rossi, can you please add testing instructions

          Show
          Sam Hemelryk added a comment - Rossi, can you please add testing instructions
          Hide
          Dan Poltawski added a comment -

          Hi All,

          This looks broken!

          https://github.com/rwijaya/moodle/compare/master...MDL-30843_b#L44R357

          Its breaking HTML editors..

          Show
          Dan Poltawski added a comment - Hi All, This looks broken! https://github.com/rwijaya/moodle/compare/master...MDL-30843_b#L44R357 Its breaking HTML editors..
          Hide
          Sam Hemelryk added a comment -

          Re-opening as per Dan's observation. Good catch thanks Dan.
          Rossi please add an additional commit to your branch and let me know when do so that I can cherry-pick it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Re-opening as per Dan's observation. Good catch thanks Dan. Rossi please add an additional commit to your branch and let me know when do so that I can cherry-pick it. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          OK I've pushed up a fix for this now and its ready for testing again.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - OK I've pushed up a fix for this now and its ready for testing again. Cheers Sam
          Hide
          Ankit Agarwal added a comment -

          This issue is generating the following error on integration master:-

          Invalid get_string() identifier: 'log' or component 'report_log'. Perhaps you are missing $string['log'] = ''; in /var/www/int/master/moodle/report/log/lang/en/report_log.php?
          on page admin>site admin>reports>log

          stack

              line 6487 of /lib/moodlelib.php: call to debugging()
              line 7111 of /lib/moodlelib.php: call to core_string_manager->get_string()
              line 620 of /report/log/locallib.php: call to get_string()
              line 198 of /report/log/index.php: call to report_log_print_selector_form()
          

          code causing this issue is in report/log/locallib.php

          echo html_writer::label(get_string('log', 'report_log'). ' '. strtolower(get_string('format')), 'menulogformat', false, array('class' => 'accesshide'));
          
          Show
          Ankit Agarwal added a comment - This issue is generating the following error on integration master:- Invalid get_string() identifier: 'log' or component 'report_log'. Perhaps you are missing $string ['log'] = ''; in /var/www/int/master/moodle/report/log/lang/en/report_log.php? on page admin>site admin>reports>log stack line 6487 of /lib/moodlelib.php: call to debugging() line 7111 of /lib/moodlelib.php: call to core_string_manager->get_string() line 620 of /report/log/locallib.php: call to get_string() line 198 of /report/log/index.php: call to report_log_print_selector_form() code causing this issue is in report/log/locallib.php echo html_writer::label(get_string('log', 'report_log'). ' '. strtolower(get_string('format')), 'menulogformat', false , array('class' => 'accesshide'));
          Hide
          Dan Poltawski added a comment -

          A reported issue from Andrew in MDL-32300:

          As a teacher when I went to Results > Statistics (/mod/quiz/report.php?id=6&mode=statistics) I get the following. It appears below the "recalculate now" button.

          Fatal error: Call to undefined function html_writer() in /home/andrew/Desktop/code/moodle/int/master/mod/quiz/report/statistics/report.php on line 977

          Show
          Dan Poltawski added a comment - A reported issue from Andrew in MDL-32300 : As a teacher when I went to Results > Statistics (/mod/quiz/report.php?id=6&mode=statistics) I get the following. It appears below the "recalculate now" button. Fatal error: Call to undefined function html_writer() in /home/andrew/Desktop/code/moodle/int/master/mod/quiz/report/statistics/report.php on line 977
          Hide
          Dan Poltawski added a comment -

          I'm failing this issue as its not looking good at all.

          Show
          Dan Poltawski added a comment - I'm failing this issue as its not looking good at all.
          Hide
          Sam Hemelryk added a comment -

          Hi Rosie,

          I'm reverting this and reopening it now.
          There have been three regressions that have been found in these changes now.
          Please go back over the changes and ensure that every change gets tested.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, I'm reverting this and reopening it now. There have been three regressions that have been found in these changes now. Please go back over the changes and ensure that every change gets tested. Cheers Sam
          Hide
          Tim Hunt added a comment -

          I am wondering about this change: https://github.com/rwijaya/moodle/compare/master...MDL-30843_b#L81R974

          Doesn't this change the set of choices in the drop-down from $downloadoptions to $this->table->defaultdownloadformat? That looks wrong to me.

          Show
          Tim Hunt added a comment - I am wondering about this change: https://github.com/rwijaya/moodle/compare/master...MDL-30843_b#L81R974 Doesn't this change the set of choices in the drop-down from $downloadoptions to $this->table->defaultdownloadformat? That looks wrong to me.
          Hide
          Rossiani Wijaya added a comment -

          Thank you Sam, Dan and Tim for the feedback.

          Make same changes to the patch and uploaded.

          Sending this for integration review.

          Show
          Rossiani Wijaya added a comment - Thank you Sam, Dan and Tim for the feedback. Make same changes to the patch and uploaded. Sending this for integration review.
          Hide
          Sam Hemelryk added a comment -

          Rosie, could you please get someone at HQ to peer-review this if you havn't already?

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Rosie, could you please get someone at HQ to peer-review this if you havn't already? Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          Sure. Will try to find someone to review it.

          Sam, could you set the status for peer-review?

          Show
          Rossiani Wijaya added a comment - Sure. Will try to find someone to review it. Sam, could you set the status for peer-review?
          Hide
          Dan Poltawski added a comment -

          Reopening as Rosie Asked

          Show
          Dan Poltawski added a comment - Reopening as Rosie Asked
          Hide
          Ankit Agarwal added a comment -

          Hi Rosie,

          • Changes to admin/roles.php are not needed
          • Found some unrelated errors in admin/timezone.php (just mentioning so that it is not forgotten )
            (Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result)
          • Just to discuss will it be better if we support label from html_writer::select api itself?

          Will do rest of the review tomorrow
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Rosie, Changes to admin/roles.php are not needed Found some unrelated errors in admin/timezone.php (just mentioning so that it is not forgotten ) (Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result) Just to discuss will it be better if we support label from html_writer::select api itself? Will do rest of the review tomorrow Thanks
          Hide
          Ankit Agarwal added a comment - - edited

          Hi,
          Here is the rest of the review:-

          • following changes in backup_detail_input()
                    if ($type == 'text') {
                        if (empty($attributes['id'])) {
                            $attributes['id'] = $name;
                        }
                        $label = html_writer::label($name, $attibutes['id'], false, array('class' => 'accesshide'));
                    } else {
                        $label = '';
                    }
            
          1. Is completely messing up the UI , as you can see in the restore page(during a course restore). since you are setting label to "". All labels are removed when the input element is not a text field.
          2. core_backup_renderer::backup_detail_pair is adding a label already so am not sure what is the use of the above modification? Also in core if you do a grep , you will see that backup_detail_input is used only to generate radio buttons at the moment in core.
          3. May be this needs to be fixed in another issue but the input field during restore are still missing labels. For example you can see this in course_selector(), which doesnt have any label (it is the same way for other input text fields)
            $html .= $this->backup_detail_pair('', html_writer::empty_tag('input', array('type'=>'submit', 'value'=>get_string('continue'))));
            
          • in grade/report/grader/lib.php
            +                            $itemcell->text .= html_writer::label(get_string('typescale', 'grades'), 'grade_'.$userid.'_'.$item->id, false, array('class' => 'accesshide'));
            

            shouldnt 'grade_'.$userid.''.$item->id be 'menugrade'.$userid.'_'.$item->id ?

          • I am not sure how $select->nothing works,
            but isset($select->nothing should be changed to empty , so as to make sure the array is not empty.
          • the space deletion in mod/feedback/analysis_course.php (L 178) is not required
          • select_time() takes care of labeling, so because of the changes in mod/forum/search.php now we have two labels for those select fields
          • wow!! finally reached the end of the diff

          Rest of the patch looks good.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi, Here is the rest of the review:- following changes in backup_detail_input() if ($type == 'text') { if (empty($attributes['id'])) { $attributes['id'] = $name; } $label = html_writer::label($name, $attibutes['id'], false , array('class' => 'accesshide')); } else { $label = ''; } Is completely messing up the UI , as you can see in the restore page(during a course restore). since you are setting label to "". All labels are removed when the input element is not a text field. core_backup_renderer::backup_detail_pair is adding a label already so am not sure what is the use of the above modification? Also in core if you do a grep , you will see that backup_detail_input is used only to generate radio buttons at the moment in core. May be this needs to be fixed in another issue but the input field during restore are still missing labels. For example you can see this in course_selector(), which doesnt have any label (it is the same way for other input text fields) $html .= $ this ->backup_detail_pair('', html_writer::empty_tag('input', array('type'=>'submit', 'value'=>get_string(' continue ')))); in grade/report/grader/lib.php + $itemcell->text .= html_writer::label(get_string('typescale', 'grades'), 'grade_'.$userid.'_'.$item->id, false , array('class' => 'accesshide')); shouldnt 'grade_'.$userid.' '.$item->id be 'menugrade '.$userid.'_'.$item->id ? I am not sure how $select->nothing works, but isset($select->nothing should be changed to empty , so as to make sure the array is not empty. the space deletion in mod/feedback/analysis_course.php (L 178) is not required select_time() takes care of labeling, so because of the changes in mod/forum/search.php now we have two labels for those select fields wow!! finally reached the end of the diff Rest of the patch looks good. Thanks
          Hide
          Rossiani Wijaya added a comment - - edited

          Thanks Ankit for reviewing.

          1. I will create new issue to address the timezone error. (MDL-32564)
          2. It would be a great improvement for html_writer::select api to support labeling. currently, most files that use html_writer::select set the label manually. This patch just cover the missing label for the html_writer::select.
          3. in grade/report/grader/lib.php, the select id is set through the attributes array, which is set to "grade_'.$userid.'_'.$item->id". Therefore, the 'menu' prefix is not needed.
          4. Made changes to the patch according to your suggestion.

          Thanks again for taking the time to review this patch Ankit.

          Patches updated and push to github.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - - edited Thanks Ankit for reviewing. 1. I will create new issue to address the timezone error. ( MDL-32564 ) 2. It would be a great improvement for html_writer::select api to support labeling. currently, most files that use html_writer::select set the label manually. This patch just cover the missing label for the html_writer::select. 3. in grade/report/grader/lib.php, the select id is set through the attributes array, which is set to "grade_'.$userid.'_'.$item->id". Therefore, the 'menu' prefix is not needed. 4. Made changes to the patch according to your suggestion. Thanks again for taking the time to review this patch Ankit. Patches updated and push to github. Submitting for integration review.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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 -

          Hi Rosie,

          Wow, sorry everyone about the last review I must've had my eye's closed while doing it.
          This time through I've picked up quite a bit that needs tiding up (sorry Rosie).

          1. "get_string('select') . ' ' . get_string('options')" is not a good solution, thats not translatable.
            • theme/mymobile/renderers.php
            • lib/outputrenderers.php
          2. report/log/locallib.php: Also "get_string('logs'). ' '. strtolower(get_string('format'))" for the same reason
          3. report/courseoverview/index.php: missing id attribute for numcourses input for new label.
          4. I wonder whether "single_select::set_label(get_accesshide(get_string('course')));" is the best approach, it is going to lead to a label containing a div that only introduces an accesshide class. I truthfully thing a better solution would be to add an attributes arg to single_select::set_label, store it and have the renderer use it when producing output.
            • calendar/renderer.php:715
            • mod/forum/lib.php:3808
            • mod/forum/lib.php:3812
            • mod/glossary/lib.php:1777
            • lib/adminlib.php:6531
            • lib/adminlib.php:6568
            • course/index.php:372
            • grade/lib.php:382
            • admin/portfolio.php:200
            • admin/portfolio.php:222
            • admin/filters.php:245
            • admin/filters.php:267
            • admin/repository.php:308
            • admin/repository.php:345
            • report/stats/locallib.php:49
          5. question/type/multianswer/renderer.php:277 Typo $selecg
          6. Something very weird and not right about these lines
            • question/type/match/renderer.php:83
            • question/type/essay/renderer.php:226
          7. Does the changes on the following lines require accesshide class? Please test and check, it will also turn up any other issues I've missed in the question types.
            • question/type/shortanswer/renderer.php:74
            • question/type/numerical/renderer.php:79&102
            • question/type/multianswer/renderer.php:277
          8. OK there appears to be lots of places where you are joining two strings to get the label, I won't mention it again in this review but they all need changing I think.
          9. question/type/calculated/questiontype.php
            • 752 & 757: Surely setting the labels text to the default selected value is not good for usability. Check with Tim and see what he thinkgs those labels should be, you can always introduce new strings if need be.
            • 1198 & 1207: Maybe I'm wrong here but that bit of script breaks HTML strict because there will be several select inputs all with the same id. You didn't do that, its already there, but adding labels is just going to compond and further increase the issue. If thats correct I think those changes should be reverted and a separate issue opened for Tim to fix XHTML strict and add labels.
          10. question/format/xhtml/format.php There are several inputs in that file that do not have labels, it appears you've only added it for the MATCH qtype. Also I'm not sure whether the string "options" really adds anything to that field?
          11. question/behaviour/rendererbase.php:97 $commentformat is an int, certainly thats not a useful label?
          12. lib/form/editor.php:362 id's can't contain [ or ]. The for attribute usef in the label call has to be wrong.
          13. Once move a default value used as the label (lib/tablelib.php:932), I can't imagine that will be right in any situation, please review all of them, I will stop mentioning it now. (I just saw an instance where the default value can be '' and is used for a label).
          14. lib/portfoliolib.php::portfolio_instance_select $selectname is used as the id, please either check that it does not use [] anywhere, or strip them for the id if they are present.
          15. lib/outputrenderers.php I don't like how single|url_select is forcing an default label of select options. There's no telling where that function has been used and it presents a VERY real chance of causing regressions. I don't think we need to force a meaningless label, I think reviewing our uses making sure where practical they have a meaningful label is a much better approach.
          16. I don't think anything in deprecated lib needs to be updated, all of them sound be converted, and again a chance of causing regressions there for thhose that have no.
          17. Please test advranced grading rubrics again to be tripple sure there are no problems with the change in source. In fact I don't know I really like the solution of adding a label inside a span that has the class label. That seems lazy, I think either we don't add the label there, or we convert it properly.

          Alright, I've run out of time sorry Rosie.
          Many of these issues pop up over and over again, please review the whole lot looking for things noted above.
          In summary:

          1. Concatenating two strings to make one is bad, translations will likely be wrong and normally its not leading to a useful label (otherwise the string would already exist).
          2. Default values don't make good labels, not useful ones at least, they can be int, they can be empty etc.
          3. Make sure if you are using the items name for the label name that has been passed in by a calling function that it is a valid id, [] are not valid in an id.
          4. Test, Test, Test. Create an absolutely crazy test case as you work through it involves testing as many areas as you can before you go nuts. Especially areas where JS is being used, and/or the select is being included in an export.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, Wow, sorry everyone about the last review I must've had my eye's closed while doing it. This time through I've picked up quite a bit that needs tiding up (sorry Rosie). "get_string('select') . ' ' . get_string('options')" is not a good solution, thats not translatable. theme/mymobile/renderers.php lib/outputrenderers.php report/log/locallib.php: Also "get_string('logs'). ' '. strtolower(get_string('format'))" for the same reason report/courseoverview/index.php: missing id attribute for numcourses input for new label. I wonder whether "single_select::set_label(get_accesshide(get_string('course')));" is the best approach, it is going to lead to a label containing a div that only introduces an accesshide class. I truthfully thing a better solution would be to add an attributes arg to single_select::set_label, store it and have the renderer use it when producing output. calendar/renderer.php:715 mod/forum/lib.php:3808 mod/forum/lib.php:3812 mod/glossary/lib.php:1777 lib/adminlib.php:6531 lib/adminlib.php:6568 course/index.php:372 grade/lib.php:382 admin/portfolio.php:200 admin/portfolio.php:222 admin/filters.php:245 admin/filters.php:267 admin/repository.php:308 admin/repository.php:345 report/stats/locallib.php:49 question/type/multianswer/renderer.php:277 Typo $selecg Something very weird and not right about these lines question/type/match/renderer.php:83 question/type/essay/renderer.php:226 Does the changes on the following lines require accesshide class? Please test and check, it will also turn up any other issues I've missed in the question types. question/type/shortanswer/renderer.php:74 question/type/numerical/renderer.php:79&102 question/type/multianswer/renderer.php:277 OK there appears to be lots of places where you are joining two strings to get the label, I won't mention it again in this review but they all need changing I think. question/type/calculated/questiontype.php 752 & 757: Surely setting the labels text to the default selected value is not good for usability. Check with Tim and see what he thinkgs those labels should be, you can always introduce new strings if need be. 1198 & 1207: Maybe I'm wrong here but that bit of script breaks HTML strict because there will be several select inputs all with the same id. You didn't do that, its already there, but adding labels is just going to compond and further increase the issue. If thats correct I think those changes should be reverted and a separate issue opened for Tim to fix XHTML strict and add labels. question/format/xhtml/format.php There are several inputs in that file that do not have labels, it appears you've only added it for the MATCH qtype. Also I'm not sure whether the string "options" really adds anything to that field? question/behaviour/rendererbase.php:97 $commentformat is an int, certainly thats not a useful label? lib/form/editor.php:362 id's can't contain [ or ]. The for attribute usef in the label call has to be wrong. Once move a default value used as the label (lib/tablelib.php:932), I can't imagine that will be right in any situation, please review all of them, I will stop mentioning it now. (I just saw an instance where the default value can be '' and is used for a label). lib/portfoliolib.php::portfolio_instance_select $selectname is used as the id, please either check that it does not use [] anywhere, or strip them for the id if they are present. lib/outputrenderers.php I don't like how single|url_select is forcing an default label of select options. There's no telling where that function has been used and it presents a VERY real chance of causing regressions. I don't think we need to force a meaningless label, I think reviewing our uses making sure where practical they have a meaningful label is a much better approach. I don't think anything in deprecated lib needs to be updated, all of them sound be converted, and again a chance of causing regressions there for thhose that have no. Please test advranced grading rubrics again to be tripple sure there are no problems with the change in source. In fact I don't know I really like the solution of adding a label inside a span that has the class label. That seems lazy, I think either we don't add the label there, or we convert it properly. Alright, I've run out of time sorry Rosie. Many of these issues pop up over and over again, please review the whole lot looking for things noted above. In summary: Concatenating two strings to make one is bad, translations will likely be wrong and normally its not leading to a useful label (otherwise the string would already exist). Default values don't make good labels, not useful ones at least, they can be int, they can be empty etc. Make sure if you are using the items name for the label name that has been passed in by a calling function that it is a valid id, [] are not valid in an id. Test, Test, Test. Create an absolutely crazy test case as you work through it involves testing as many areas as you can before you go nuts. Especially areas where JS is being used, and/or the select is being included in an export. Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam for the feedback.

          #9b (script breaks HTML strict): I revert the the patch and will create a new issue for that.

          I updated the patch for master only at this point. Once it pass the peer review stage, I will update the other version.

          Submitting for peer review.

          Show
          Rossiani Wijaya added a comment - Thanks Sam for the feedback. #9b (script breaks HTML strict): I revert the the patch and will create a new issue for that. I updated the patch for master only at this point. Once it pass the peer review stage, I will update the other version. Submitting for peer review.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Rosie,
          Here is my review:-

          1. echo html_writer::label(get_string('username'), 'menuusername', false, array('class' =>'accesshide'));
            should be
            echo html_writer::label(get_string('username'), 'menuusername', false, array('class' => 'accesshide')); (space)
          2. in access_control.php we are replacing echo " " . get_string('accesslevel', 'mnet') . ":\n"; with html writer. In the process we are loosing the ":\n"
          3. changes to backup/util/ui/renderer.php are not needed.
          4. in grade/edit/tree/lib.php id of the label should be 'menuaggregation_'.$category->id instead of 'aggregation_'.$category->id
          5. similarly "menu" is missing in grade/report/grader/lib.php
          6. in lib/outputcomponents.php the phpdoc for $labelattributes says it is a string but it is an array(twice in the same page)
          7. is class="" required in lib/portfolio.php
          8. 'viewing' should be 'menuviewing' in lib/message.php for the html label
          9. mod/assignment/lib.php 'menumenu'. $auser->id should be 'menumenu['. $auser->id .']' ?
          10. the labels in mod/forum/search.php are wrong menufromday should be menutoday (multiple similar instances in the same page)
          11. there are bunch of select_time() in mod/forum/search.php which are missing labels

          Rest looks good.
          Thanks!

          Show
          Ankit Agarwal added a comment - - edited Hi Rosie, Here is my review:- echo html_writer::label(get_string('username'), 'menuusername', false, array('class' =>'accesshide')); should be echo html_writer::label(get_string('username'), 'menuusername', false, array('class' => 'accesshide')); (space) in access_control.php we are replacing echo " " . get_string('accesslevel', 'mnet') . ":\n"; with html writer. In the process we are loosing the ":\n" changes to backup/util/ui/renderer.php are not needed. in grade/edit/tree/lib.php id of the label should be 'menuaggregation_'.$category->id instead of 'aggregation_'.$category->id similarly "menu" is missing in grade/report/grader/lib.php in lib/outputcomponents.php the phpdoc for $labelattributes says it is a string but it is an array(twice in the same page) is class="" required in lib/portfolio.php 'viewing' should be 'menuviewing' in lib/message.php for the html label mod/assignment/lib.php 'menumenu'. $auser->id should be 'menumenu ['. $auser->id .'] ' ? the labels in mod/forum/search.php are wrong menufromday should be menutoday (multiple similar instances in the same page) there are bunch of select_time() in mod/forum/search.php which are missing labels Rest looks good. Thanks!
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Rossiani Wijaya added a comment -

          Thank you Ankit for reviewing.

          4. The id for the select is set through $attributes with the value of 'aggregation_'.$category->id. So the 'menu' string is not needed.
          5. same case as #4.
          8. same case as #4.
          9. html_writer::select truncate the '[' and ']' sign for the id. Therefore when it renders it shows menumenu$user->id.

          Fixed the rest that you mentioned above.

          I will look at the patch 1 more time before submitting it for integration review.

          Show
          Rossiani Wijaya added a comment - Thank you Ankit for reviewing. 4. The id for the select is set through $attributes with the value of 'aggregation_'.$category->id. So the 'menu' string is not needed. 5. same case as #4. 8. same case as #4. 9. html_writer::select truncate the ' [' and '] ' sign for the id. Therefore when it renders it shows menumenu$user->id. Fixed the rest that you mentioned above. I will look at the patch 1 more time before submitting it for integration review.
          Hide
          Rossiani Wijaya added a comment -

          Update patch.

          Sending for integration review.

          Show
          Rossiani Wijaya added a comment - Update patch. Sending for integration review.
          Hide
          Sam Hemelryk added a comment -

          Hi Rosie,

          This is getting so very close, but the more I look at things the more I think this issue has become an unmangeable size and needs to be split.
          While reviewing this I noticed the following:

          1. course/scales.php nothing value has been changed.
          2. filter/algebra/algebradebug.php label contains an anchor which from memory is not great for usability. This area perhaps does not need to be worried about or a smarter+smaller label found and applied with accesshide.
          3. Needless whitespace removed from deprecatedlib.php
          4. weblib.php print_grade_menu I don't think this function should be changed, its been depreacted, and plus I don't think gradeitems would describe what is actually being selected.

          The following areas I tested because the labels didn't look like they would really be useful.

          1. lib/adminlib.php admin_setting_courselist_frontpage::output_html the same label is repeated four times, it is probably less accessible than it was previously.
          2. lib/adminlib.php admin_setting_emoticons::output_html the label contains the field value, surely thats ok no benefit?
          3. lib/adminlib.php admin_setting_configtime::output_html I think this needs to be looked at, perhaps improved. It prints three labels, the first is the setting string e.g. "Run at", then it prints a label for each select box so "hour" and "minutes". Do screenreaders still read out that first label? or should we have two labels "Run at hour" and "Run at minute".
          4. Anything using the new "selectanoptions". The text for that is "Select an options", as the purpose of a select box is to let the user select an option I don't think that needs to be said. I think either we apply a useful label, or we don't apply a label at all.
          5. messages/lib.php message_print_usergroup_selector I don't think "Go to messages" by itself is a great choice for that select box. Looking at it I see it has no labels or text at all at the moment. It may be an idea to have a chat to Andrew about this one and see what he thinks. Perhaps we could put a visible label there.
          6. mod/chat/gui_ajax/index.php uses "Messages" for the input where you type your chat messages. I think this could probably be improved. Perhaps "Enter your message" or something more edgy like "Say". To be truthful I wasn't sure what really suits here so I checked out Google chat. They do a private chat of course, person to person. They don't use labels but they do use aria. They have "Chat with Jen Hemelryk, currently offline" or similar.

          All in all 90% of the changes look spot on, I think we need to look at how we can get some of these changes into core so that we can separate out and simplify the area that still need work, or areas we're not settled upon yet.

          So two methods strike me:

          1. We create issues for each area (admin, course, blocks, assignment, forum, message etc) split the patch and put the ones we are happy with back up for integration review.
          2. Revert the changes that we arn't sure about from this patch and put it up for integration reivew, then open issues for the remaining areas.

          Personally I don't mind so much, but I think perhaps 1 will see things get in easier.

          Eitehr way a big MUST for this issue is to test every change.
          As part of this review I started looking at the label changes in the interface, and that has shown up more areas still requiring thought and work.
          I think in order to be happy with this issue we need to ensure each and every area gets looked at and where applicable because of JS functionality tested (i.e. quick grading in assignment).

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, This is getting so very close, but the more I look at things the more I think this issue has become an unmangeable size and needs to be split. While reviewing this I noticed the following: course/scales.php nothing value has been changed. filter/algebra/algebradebug.php label contains an anchor which from memory is not great for usability. This area perhaps does not need to be worried about or a smarter+smaller label found and applied with accesshide. Needless whitespace removed from deprecatedlib.php weblib.php print_grade_menu I don't think this function should be changed, its been depreacted, and plus I don't think gradeitems would describe what is actually being selected. The following areas I tested because the labels didn't look like they would really be useful. lib/adminlib.php admin_setting_courselist_frontpage::output_html the same label is repeated four times, it is probably less accessible than it was previously. lib/adminlib.php admin_setting_emoticons::output_html the label contains the field value, surely thats ok no benefit? lib/adminlib.php admin_setting_configtime::output_html I think this needs to be looked at, perhaps improved. It prints three labels, the first is the setting string e.g. "Run at", then it prints a label for each select box so "hour" and "minutes". Do screenreaders still read out that first label? or should we have two labels "Run at hour" and "Run at minute". Anything using the new "selectanoptions". The text for that is "Select an options", as the purpose of a select box is to let the user select an option I don't think that needs to be said. I think either we apply a useful label, or we don't apply a label at all. messages/lib.php message_print_usergroup_selector I don't think "Go to messages" by itself is a great choice for that select box. Looking at it I see it has no labels or text at all at the moment. It may be an idea to have a chat to Andrew about this one and see what he thinks. Perhaps we could put a visible label there. mod/chat/gui_ajax/index.php uses "Messages" for the input where you type your chat messages. I think this could probably be improved. Perhaps "Enter your message" or something more edgy like "Say". To be truthful I wasn't sure what really suits here so I checked out Google chat. They do a private chat of course, person to person. They don't use labels but they do use aria. They have "Chat with Jen Hemelryk, currently offline" or similar. All in all 90% of the changes look spot on, I think we need to look at how we can get some of these changes into core so that we can separate out and simplify the area that still need work, or areas we're not settled upon yet. So two methods strike me: 1. We create issues for each area (admin, course, blocks, assignment, forum, message etc) split the patch and put the ones we are happy with back up for integration review. 2. Revert the changes that we arn't sure about from this patch and put it up for integration reivew, then open issues for the remaining areas. Personally I don't mind so much, but I think perhaps 1 will see things get in easier. Eitehr way a big MUST for this issue is to test every change. As part of this review I started looking at the label changes in the interface, and that has shown up more areas still requiring thought and work. I think in order to be happy with this issue we need to ensure each and every area gets looked at and where applicable because of JS functionality tested (i.e. quick grading in assignment). Cheers Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          moodle.com added a comment -

          In order to integrate these changes we needed to split this issue. Because Jira does not permit sub-tasks to have sub-tasks, we have created a new META issue (MDL-34551) with a number of sub-tasks to manage the integration of the fix to this issue.

          Show
          moodle.com added a comment - In order to integrate these changes we needed to split this issue. Because Jira does not permit sub-tasks to have sub-tasks, we have created a new META issue ( MDL-34551 ) with a number of sub-tasks to manage the integration of the fix to this issue.
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Rossiani Wijaya added a comment -

          This issue has been fixed through MDL-34551.

          Closing.

          Show
          Rossiani Wijaya added a comment - This issue has been fixed through MDL-34551 . Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: