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

      Description

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

        Gliffy Diagrams

          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: