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

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.

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.

Thanks David for reviewing.

Update patch to remove td.hide from css file.

Submitting for integration review.

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

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

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

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.

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

Hi Tim and Lori,

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

Thanks
Rosie

"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.

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.

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

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

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

Thanks Ankit for reviewing.

Updated the patch for whitespace.

Submitting for integration review.

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

Thanks Rosie, this has been integrated now.

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

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

(not really)

Closing, thanks!