Details

    • Testing Instructions:
      Hide

      There is no visible changes on this issue. I simply add TH and SCOPE tags to identify which cells are row and column headers.
      Site reference about table accessibility: http://accessibility.psu.edu/tables

      As admin, set the following to choice activity:
      case 1:

      1. display mode: vertical
      2. publish result: show results to students after they answer
      3. privacy result: publish anonymous results, do not show student names
      4. as student, view the responses page (if you haven't select any response, do so).

      case 2:

      1. display mode: horizontal
      2. publish result: show results to students after they answer
      3. privacy result: publish anonymous results, do not show student names
      4. as student, view the responses page

      case 3:

      1. privacy result: publish full results, showing names and their choices
      2. as admin, view the responses page
      3. as student, view the responses page

      Make sure the above cases are working without error.

      Show
      There is no visible changes on this issue. I simply add TH and SCOPE tags to identify which cells are row and column headers. Site reference about table accessibility: http://accessibility.psu.edu/tables As admin, set the following to choice activity: case 1: display mode: vertical publish result: show results to students after they answer privacy result: publish anonymous results, do not show student names as student, view the responses page (if you haven't select any response, do so). case 2: display mode: horizontal publish result: show results to students after they answer privacy result: publish anonymous results, do not show student names as student, view the responses page case 3: privacy result: publish full results, showing names and their choices as admin, view the responses page as student, view the responses page Make sure the above cases are working without error.
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33757

      Description

      The choice responses should be summarized into a simple table with proper headers such as

      <table border="1">
        <tr>
          <th scope="col">Choice</th>
          <th scope="col">Percentage</th>
          <th scope="col">Responses</th>
        </tr>
        <tr>
          <th scope="row">Green</th>
          <td>23%</td>
          <td>8</td>
        </tr>
        <tr>
          <th scope="row">Red</th>
          <td>14%</td>
          <td>5</td>
        </tr>
        <tr>
          <th scope="row">Blue</th>
          <td>34%</td>
          <td>12</td>
        </tr>
        <tr>
          <th scope="row">Yellow</th>
          <td>29%</td>
          <td>10</td>
        </tr>
      </table>
      

      If you wanted to add a bar graph to it as well, you can do that with the technique that is currently employed of stretching an image to the correct number of pixels. If that technique is used, just make the alt="" instead of putting all of the "skip result graph" links.

      The key here is you are presenting a data table, and all data tables must denote their row and/or column headers.

      1. choice-diff-results.html
        16 kB
        Glenn Ansley
      2. choice-results-greg-kraus.html
        12 kB
        Greg Kraus
      3. choices_withpatch.html
        12 kB
        Rossiani Wijaya

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Thank you for addressing the accessibility issue on choice module.

          I made a patch to address this. It would be great if you could provide some feedback for the patch.

          Please note that this is a rough patch, I will polish it up once I get some feedback from you or any reviewer.

          Thanks.
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Greg, Thank you for addressing the accessibility issue on choice module. I made a patch to address this. It would be great if you could provide some feedback for the patch. Please note that this is a rough patch, I will polish it up once I get some feedback from you or any reviewer. Thanks. Rosie
          Hide
          Andrew Davis added a comment -

          Hi Rossie. Could you write some test instructions for this

          Show
          Andrew Davis added a comment - Hi Rossie. Could you write some test instructions for this
          Hide
          Rossiani Wijaya added a comment -

          Hi Andrew,

          I just added testing instruction. Please note that there is no visible changes to the page.

          Thanks

          Show
          Rossiani Wijaya added a comment - Hi Andrew, I just added testing instruction. Please note that there is no visible changes to the page. Thanks
          Hide
          Glenn Ansley added a comment -

          Hi Rossiani,
          Thanks for the updates. I'm attaching an HTML file that contains copy / paste from the source code for all three cases (before and after). I'm doing this because I can apply the changes and compare output but I don't know enough to say the changes are what Greg was hoping for or not.

          Greg - hopefully this html file will help you give some feedback.
          Glenn

          Show
          Glenn Ansley added a comment - Hi Rossiani, Thanks for the updates. I'm attaching an HTML file that contains copy / paste from the source code for all three cases (before and after). I'm doing this because I can apply the changes and compare output but I don't know enough to say the changes are what Greg was hoping for or not. Greg - hopefully this html file will help you give some feedback. Glenn
          Hide
          Rossiani Wijaya added a comment -

          Hi Glenn,

          Thank you for creating output comparison. It surely will help other user to test and provide some feedback.

          Rosie.

          Show
          Rossiani Wijaya added a comment - Hi Glenn, Thank you for creating output comparison. It surely will help other user to test and provide some feedback. Rosie.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Rosie,
          This looks good to me. (Ps:- I have limited knowledge of accessibility tough)
          just correct the spacing on line 392 before submitting for integration. ($accessiblecell->scope = 'col'; )

          Really appreciate your help Glenn with this.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Rosie, This looks good to me. (Ps:- I have limited knowledge of accessibility tough) just correct the spacing on line 392 before submitting for integration. ($accessiblecell->scope = 'col'; ) Really appreciate your help Glenn with this. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Thanks Ankit for reviewing the code.

          Patch updated.

          Show
          Rossiani Wijaya added a comment - Thanks Ankit for reviewing the code. Patch updated.
          Hide
          Michael de Raadt added a comment -

          Hi, Greg.

          Could you please let us know if this change will resolve the issue for you? Perhaps you could work with Glenn to see the effect of this change.

          The reason I ask is that this is the first of many issues we are going to resolve with this accessibility work and I want to be sure we are on the right track before we get stuck into the rest.

          Show
          Michael de Raadt added a comment - Hi, Greg. Could you please let us know if this change will resolve the issue for you? Perhaps you could work with Glenn to see the effect of this change. The reason I ask is that this is the first of many issues we are going to resolve with this accessibility work and I want to be sure we are on the right track before we get stuck into the rest.
          Hide
          Greg Kraus added a comment -

          I'll have some feedback for you tomorrow.

          Show
          Greg Kraus added a comment - I'll have some feedback for you tomorrow.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Greg.

          Could you also please provide some feedback for MDL-30843?

          Show
          Rossiani Wijaya added a comment - Thanks Greg. Could you also please provide some feedback for MDL-30843 ?
          Hide
          Greg Kraus added a comment -

          I attached a file (choice-results-greg-kraus.html) with the way I would code the page. Here are my notes for it.

          • Usually your table headers are actually repeated somehwere else in the page. You don't want to have the same information in teh header cell also present in the same place somewhere else in the table. All three cases had that. To a screen reader user they are in essence going to hear the header text twice - once when it reads the header and once when it reads the text in the cell.
          • I wouldn't put the text "number of users" as a title, because then it's only available to users when they mouse over it. I would make it its own table header.
          • Any time you have the text "Graph Display" you can leave that off and have a blank cell if you want or you can leave it on
          • You don't have to include the text "Choice Options" if you don't want to. You can just make it an empty cell or you can leave it in there.
          • I would not hide the table headers off the screen. I've never seen that technique used before.
          • You don't need any of the "skip result graph" links, especially since the graph is a single image with alt=""
          • For alt text for the image, you don't need to say "picture of". The user already knows it's a picture. Because the person's name immediately follows the image you could say alt="" because the context of the picture tells you what it is. (I realize this is a larger issue than the way the picture is generated for this one specific page.)
          • I would add a table summary like summary="Results for CHOICE NAME". This doesn't change the way the page disaplays or functions, but it gives screen reader users an idea of what is about to be presented to them in the table. You could get even more detailed with the summary and give a description of how the data is laid out. (for case 2) summary="Results for CHOICE NAME. Each result is displayed in a row with a graph in the last cell of each row." By explaining there is a graph there the screen reader user won't wonder why they aren't hearing anything there when they know there is a cell there.
          Show
          Greg Kraus added a comment - I attached a file (choice-results-greg-kraus.html) with the way I would code the page. Here are my notes for it. Usually your table headers are actually repeated somehwere else in the page. You don't want to have the same information in teh header cell also present in the same place somewhere else in the table. All three cases had that. To a screen reader user they are in essence going to hear the header text twice - once when it reads the header and once when it reads the text in the cell. I wouldn't put the text "number of users" as a title, because then it's only available to users when they mouse over it. I would make it its own table header. Any time you have the text "Graph Display" you can leave that off and have a blank cell if you want or you can leave it on You don't have to include the text "Choice Options" if you don't want to. You can just make it an empty cell or you can leave it in there. I would not hide the table headers off the screen. I've never seen that technique used before. You don't need any of the "skip result graph" links, especially since the graph is a single image with alt="" For alt text for the image, you don't need to say "picture of". The user already knows it's a picture. Because the person's name immediately follows the image you could say alt="" because the context of the picture tells you what it is. (I realize this is a larger issue than the way the picture is generated for this one specific page.) I would add a table summary like summary="Results for CHOICE NAME". This doesn't change the way the page disaplays or functions, but it gives screen reader users an idea of what is about to be presented to them in the table. You could get even more detailed with the summary and give a description of how the data is laid out. (for case 2) summary="Results for CHOICE NAME. Each result is displayed in a row with a graph in the last cell of each row." By explaining there is a graph there the screen reader user won't wonder why they aren't hearing anything there when they know there is a cell there.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Thank you for the feedback.

          I updated the patches according to your suggestions. Please let me know if it needs further changes.

          Show
          Rossiani Wijaya added a comment - Hi Greg, Thank you for the feedback. I updated the patches according to your suggestions. Please let me know if it needs further changes.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          When you have a chance, could you please peer review this issue again?

          Thanks.

          Show
          Rossiani Wijaya added a comment - Hi Greg, When you have a chance, could you please peer review this issue 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 -

          I'll take a look at it.

          Show
          Greg Kraus added a comment - I'll take a look at it.
          Hide
          Greg Kraus added a comment -

          I'm having a difficult time getting this patch applied. Can you just send me the html output for the results page? Thanks.

          Show
          Greg Kraus added a comment - I'm having a difficult time getting this patch applied. Can you just send me the html output for the results page? Thanks.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Just created the html output for the results page.

          Thank you for helping us.

          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Greg, Just created the html output for the results page. Thank you for helping us. Rosie
          Hide
          Rossiani Wijaya added a comment -

          Rebasing.

          Ping Greg.

          Show
          Rossiani Wijaya added a comment - Rebasing. Ping Greg.
          Hide
          Michael de Raadt added a comment -

          Hi, Greg.

          It would be good if you could make another attempt to review this. It would be good if we could get this issue integrated before the end of this sprint.

          Show
          Michael de Raadt added a comment - Hi, Greg. It would be good if you could make another attempt to review this. It would be good if we could get this issue integrated before the end of this sprint.
          Hide
          Greg Kraus added a comment -

          Sorry for the delay. All of the patch versions in the latest html file look good in terms of the table layout and headers. The only problem I see is actually with the checkbox input when the individual users are listed. They are unlabelled form elements. You could do something like the following where I added the id="user" in the input and for="user" in the new label element I inserted.

          <div class="user"><div class="attemptaction"><input type="checkbox" name="attemptid[]" value="3" id="user"></div><div class="image"><a href="http://local/moodle/user/view.php?id=3&course=2"><img width="35" height="35" class="userpicture defaultuserpic" title="Picture of donald duck" alt="Picture of donald duck" src="http://local/moodle/theme/image.php?theme=magazine&image=u%2Ff2&rev=935"></a></div><div class="fullname"><label for="user"><a class="username" href="http://local/moodle/user/view.php?id=3&course=2">donald duck</a></label></div><div class="clearfloat"></div></div>

          If this isn't easy to do for some reason there are some other implementation options.

          Show
          Greg Kraus added a comment - Sorry for the delay. All of the patch versions in the latest html file look good in terms of the table layout and headers. The only problem I see is actually with the checkbox input when the individual users are listed. They are unlabelled form elements. You could do something like the following where I added the id="user" in the input and for="user" in the new label element I inserted. <div class="user"><div class="attemptaction"><input type="checkbox" name="attemptid[]" value="3" id="user"></div><div class="image"><a href="http://local/moodle/user/view.php?id=3&course=2"><img width="35" height="35" class="userpicture defaultuserpic" title="Picture of donald duck" alt="Picture of donald duck" src="http://local/moodle/theme/image.php?theme=magazine&image=u%2Ff2&rev=935"></a></div><div class="fullname"><label for="user"><a class="username" href="http://local/moodle/user/view.php?id=3&course=2">donald duck</a></label></div><div class="clearfloat"></div></div> If this isn't easy to do for some reason there are some other implementation options.
          Hide
          Rossiani Wijaya added a comment -

          Hi Greg,

          Thank you for reviewing.

          As for the unlabeled element for the checkbox input, it will be fixed through MDL-33575.

          Patch has been rebased.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Greg, Thank you for reviewing. As for the unlabeled element for the checkbox input, it will be fixed through MDL-33575 . Patch has been rebased. Submitting for integration review.
          Hide
          Sam Hemelryk added a comment -

          Hi Rosie,

          I'm sending this back sorry.
          I have noted a couple of regressions while reviewing this.

          1. When using $choice->name you must pass it through format_string as the multilang filter may be used. When using it within table->summary you need to string tags as well probably because I don't imagine HTML within the summary attribute will bode well with XHTML.
          2. There's a bug when showing results and an option has been selected by more than one user. It can be fixed by changing "$cell->text =" to "$cell->text .=" when adding users to an option. However that is not a great solution, writing to a property should never be done by that as there is a high chance of regression in the future (property made private and exposed through get/set for example). I think it would be best to build up a $content or $html var and then set that to he cell text after iterating the users.

          The rest looks OK.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, I'm sending this back sorry. I have noted a couple of regressions while reviewing this. When using $choice->name you must pass it through format_string as the multilang filter may be used. When using it within table->summary you need to string tags as well probably because I don't imagine HTML within the summary attribute will bode well with XHTML. There's a bug when showing results and an option has been selected by more than one user. It can be fixed by changing "$cell->text =" to "$cell->text .=" when adding users to an option. However that is not a great solution, writing to a property should never be done by that as there is a high chance of regression in the future (property made private and exposed through get/set for example). I think it would be best to build up a $content or $html var and then set that to he cell text after iterating the users. The rest looks OK. Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam for the comment.

          Updating patch and re-sending for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Sam for the comment. Updating patch and re-sending for integration review.
          Hide
          Sam Hemelryk added a comment -

          Thanks Rosie, all looked spot on this time and has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Rosie, all looked spot on this time and has been integrated now
          Hide
          Adrian Greeve added a comment -

          Tested with 2.1, 2.2, 2.3 and master.
          No errors to report.
          Everything working well.
          Test passed

          Show
          Adrian Greeve added a comment - Tested with 2.1, 2.2, 2.3 and master. No errors to report. Everything working well. 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

            People

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

              Dates

              • Created:
                Updated:
                Resolved: