Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.5
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Accessibility, Questions
    • Labels:
      None
    • Rank:
      809

      Description

      Layout tables used for many question types but not labeled as such. No headers.

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Hi Lori,

          Thank you for reporting accessibility issue.
          However, I'm not able to locate the issue. Could you provide more info regarding this issue(eg: web address)?

          Thank you
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Lori, Thank you for reporting accessibility issue. However, I'm not able to locate the issue. Could you provide more info regarding this issue(eg: web address)? Thank you Rosie
          Hide
          Rossiani Wijaya added a comment -

          Hi Lori,

          If I understand correctly, the issue is located at question bank page. Is this correct?

          steps to replicate:

          1. select the 'question bank' link under 'course admin' block
          2. create new question, if there's none
          3. using firebug, view the source code for the listed question and notice that the table is missing header for column and row.
          Show
          Rossiani Wijaya added a comment - Hi Lori, If I understand correctly, the issue is located at question bank page. Is this correct? steps to replicate: select the 'question bank' link under 'course admin' block create new question, if there's none using firebug, view the source code for the listed question and notice that the table is missing header for column and row.
          Hide
          David Monllaó added a comment -

          Hi Rossie,

          All seems ok with the scope attribute; with NVDA (default config) the non-focusable elements are skipped but it seems that other screen readers like JAWS reads tables based on the scope attr (http://www.usability.com.au/resources/tables.cfm#simple)

          The only thing I see is in the styles file, the td.hide don't seems to be used anywhere.

          Show
          David Monllaó added a comment - Hi Rossie, All seems ok with the scope attribute; with NVDA (default config) the non-focusable elements are skipped but it seems that other screen readers like JAWS reads tables based on the scope attr ( http://www.usability.com.au/resources/tables.cfm#simple ) The only thing I see is in the styles file, the td.hide don't seems to be used anywhere.
          Hide
          Rossiani Wijaya added a comment -

          Thanks David for reviewing.

          Update patch to remove td.hide from css file.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks David for reviewing. Update patch to remove td.hide from css file. Submitting for integration review.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 Rossi,

          Straight up I am a little lost about these changes.
          What I see is that you are adding a new column at that start that is hidden and contains the question type correct?
          However column 2 (or 3 with your patch applied) is already doing exactly that but using an icon rather than text, and is using the proper question type string as both the alt and the title.
          Perhaps I'm misunderstanding something but this change just appears to be introducing duplicate information.

          I'll leave this in integration review in order to let you reply.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rossi, Straight up I am a little lost about these changes. What I see is that you are adding a new column at that start that is hidden and contains the question type correct? However column 2 (or 3 with your patch applied) is already doing exactly that but using an icon rather than text, and is using the proper question type string as both the alt and the title. Perhaps I'm misunderstanding something but this change just appears to be introducing duplicate information. I'll leave this in integration review in order to let you reply. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          I'm reopening this Rossi, looks like this is just duplicating data there.

          Show
          Sam Hemelryk added a comment - I'm reopening this Rossi, looks like this is just duplicating data there.
          Hide
          Rossiani Wijaya added a comment -

          Hi Sam,

          Sorry for not replying sooner, I didn't get the notification for your comment.

          The additional column is meant for screenreader to describe the content for the table row. The accessibility for table is to have <th> for each row and I thought it would be good to know the question type earlier so they can immediately checked the option on the next column.

          Show
          Rossiani Wijaya added a comment - Hi Sam, Sorry for not replying sooner, I didn't get the notification for your comment. The additional column is meant for screenreader to describe the content for the table row. The accessibility for table is to have <th> for each row and I thought it would be good to know the question type earlier so they can immediately checked the option on the next column.
          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
          Rossiani Wijaya added a comment -

          Hi Tim and Lori,

          Could you provide some feedback on what is the best way to solve this issue?

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Tim and Lori, Could you provide some feedback on what is the best way to solve this issue? Thanks Rosie
          Hide
          Tim Hunt added a comment -

          "The accessibility for table is to have <th> for each row" I have never seen that advice. Do you have a reference?

          Even if that is correct advice, adding a duplicate questiontype column serves no purpose. The question type is just one property that a question has. It is not the natural 'heading' for the row.

          If any column should be changed from td to th, then it is probably the name column (but leaving it where it is in the middle of the row).

          It is particularly nasty the way you have just hacked echo '<th>' into a random place in the code. Did you not look at how the code was already structured. (A question bank view object, which is an aggregate of a number of different question bank column objects.) Please can you respect that existing structure in any changes you make.

          Note that there are actually two different displays of the question bank: One when you go to 'Question bank' in the settings navigation, and one when you go to 'Edit quiz' - and these have different columns. Also, in the 'Question bank' view, see what happens when you turn on the 'Display question text' option.

          I suggest the correct way to implement this is:

          1. New public method on question_bank_column_base, set_as_header_col, which sets a corresponding protected field.
          2. Change display_start and display_end so that if that field is set, then the output <th scope="row" instead of <td. You should probably take the opportunity to change those methods to use HTML writer.
          3. Change the code that sets up question_bank_view, and quiz_question_bank_view, to set the most appropriate existing column to be the header col.
          Show
          Tim Hunt added a comment - "The accessibility for table is to have <th> for each row" I have never seen that advice. Do you have a reference? Even if that is correct advice, adding a duplicate questiontype column serves no purpose. The question type is just one property that a question has. It is not the natural 'heading' for the row. If any column should be changed from td to th, then it is probably the name column (but leaving it where it is in the middle of the row). It is particularly nasty the way you have just hacked echo '<th>' into a random place in the code. Did you not look at how the code was already structured. (A question bank view object, which is an aggregate of a number of different question bank column objects.) Please can you respect that existing structure in any changes you make. Note that there are actually two different displays of the question bank: One when you go to 'Question bank' in the settings navigation, and one when you go to 'Edit quiz' - and these have different columns. Also, in the 'Question bank' view, see what happens when you turn on the 'Display question text' option. I suggest the correct way to implement this is: New public method on question_bank_column_base, set_as_header_col, which sets a corresponding protected field. Change display_start and display_end so that if that field is set, then the output <th scope="row" instead of <td. You should probably take the opportunity to change those methods to use HTML writer. Change the code that sets up question_bank_view, and quiz_question_bank_view, to set the most appropriate existing column to be the header col.
          Hide
          Rossiani Wijaya added a comment -

          Thanks Tim for the suggestion on how to fix the issue.

          Noting for display_start() and display_end(), the parameters are not used within the base class. It is being used for the sub-class, therefore it shouldn't be deleted.

          Sending it for peer-review.

          Show
          Rossiani Wijaya added a comment - Thanks Tim for the suggestion on how to fix the issue. Noting for display_start() and display_end(), the parameters are not used within the base class. It is being used for the sub-class, therefore it shouldn't be deleted. Sending it for peer-review.
          Hide
          Tim Hunt added a comment -

          Just changing the issue title so that it accurately reflects the change that was done.

          Show
          Tim Hunt added a comment - Just changing the issue title so that it accurately reflects the change that was done.
          Hide
          Tim Hunt added a comment -

          One peer reveiw comment. Minor whitespace error in the first PHPdoc comment: https://github.com/rwijaya/moodle/compare/master...MDL-21625_b#L0R1144

          Show
          Tim Hunt added a comment - One peer reveiw comment. Minor whitespace error in the first PHPdoc comment: https://github.com/rwijaya/moodle/compare/master...MDL-21625_b#L0R1144
          Hide
          Ankit Agarwal added a comment -

          Hi Rosie,
          This looks good. Also tested this out in master and works as expected.
          Feel free to submit this for integration after taking care of the white space issue Tim mentioned.
          Thanks

          Show
          Ankit Agarwal added a comment - Hi Rosie, This looks good. Also tested this out in master and works as expected. Feel free to submit this for integration after taking care of the white space issue Tim mentioned. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Thanks Ankit for reviewing.

          Updated the patch for whitespace.

          Submitting for integration review.

          Show
          Rossiani Wijaya added a comment - Thanks Ankit for reviewing. Updated the patch for whitespace. Submitting for integration review.
          Hide
          Dan Poltawski 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
          Dan Poltawski 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 -

          Thanks Rosie, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Rosie, this has been integrated now.
          Hide
          David Monllaó added a comment -

          It passes. Tested in 22, 23 and master, I can see the th with scope="row" in the question name column

          Show
          David Monllaó added a comment - It passes. Tested in 22, 23 and master, I can see the th with scope="row" in the question name column
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: