Details

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

      Description

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

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              rwijaya 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
              rwijaya 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
              rwijaya 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
              rwijaya 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
              dmonllao 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
              dmonllao 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
              rwijaya Rossiani Wijaya added a comment -

              Thanks David for reviewing.

              Update patch to remove td.hide from css file.

              Submitting for integration review.

              Show
              rwijaya Rossiani Wijaya added a comment - Thanks David for reviewing. Update patch to remove td.hide from css file. Submitting for integration review.
              Hide
              poltawski 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
              poltawski 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
              samhemelryk 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
              samhemelryk 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
              samhemelryk Sam Hemelryk added a comment -

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

              Show
              samhemelryk Sam Hemelryk added a comment - I'm reopening this Rossi, looks like this is just duplicating data there.
              Hide
              rwijaya 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
              rwijaya 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              rwijaya 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
              rwijaya 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
              timhunt 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
              timhunt 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
              rwijaya 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
              rwijaya 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
              timhunt Tim Hunt added a comment -

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

              Show
              timhunt Tim Hunt added a comment - Just changing the issue title so that it accurately reflects the change that was done.
              Hide
              timhunt 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
              timhunt 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_frenz 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_frenz 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
              rwijaya Rossiani Wijaya added a comment -

              Thanks Ankit for reviewing.

              Updated the patch for whitespace.

              Submitting for integration review.

              Show
              rwijaya Rossiani Wijaya added a comment - Thanks Ankit for reviewing. Updated the patch for whitespace. Submitting for integration review.
              Hide
              poltawski 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
              poltawski 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
              samhemelryk Sam Hemelryk added a comment -

              Thanks Rosie, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Rosie, this has been integrated now.
              Hide
              dmonllao 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
              dmonllao 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

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

              (not really)

              Closing, thanks!

              Show
              stronk7 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:
                    Fix Release Date:
                    12/Nov/12