Details

    • Testing Instructions:
      Hide

      In a course question bank, create new question for the following question types using a screen reader and verify that elements have sensible labels on them:

      • shortanswer
      • numerical
      • calculated
      • essay
      • match
      • multianswer
      Show
      In a course question bank, create new question for the following question types using a screen reader and verify that elements have sensible labels on them: shortanswer numerical calculated essay match multianswer
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34570-master
    • Rank:
      43003

      Description

      There are several select input fields that do not have labels explicitly tied to them. Often this is because there is a visual cue as to what information is being asked for but the visual cue is not explicitly linked to the input element.

      Because this problem is sporadic it might be necessary to break this task out into smaller sub-tasks for each instance of the problem.

      Here is a tutorial for methods of labeling text input elements. http://oit.ncsu.edu/itaccess/forms#select

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Seems like Tim hasn't been added as a watcher to this issue. Added him.

          Show
          Dan Poltawski added a comment - Seems like Tim hasn't been added as a watcher to this issue. Added him.
          Hide
          Tim Hunt added a comment -

          Sorry, but -1 from me. The labels that have been added are completely meaningless.

          To start with, look at match. You have added a label: "Select an answer". Come on! The input control is a <select> menu. People who use a screen-reader have difficulty seeing / reading. They have not had their brain surgically removed. Of course you use a select menu in a question to select your answer. What they need to know is, which question should they be answering with this select menu. In a matching question there are several inputs, adding the same label "Select an answer" does nothing to help anyone.

          The <label> needs to go around the subquestion text. Unfortunately, you can't just do that, because the subquestion can contain arbitrary HTML like <p> tags, and <label> is an inline-level HTML element. At least it was in HTML 4. I keep meaning to check if that restriction is still there for valid HTML 5.

          Then we get to shortanswer. You are using $inputattributes['name'] as the label text. Err, what! That is the name="..." attribute from the HTML - a variable name, not anything the user should see. It will typically look like q123:456_answer.

          I know improving Moodle accessibility is important, and that unlabelled inputs are an accessibility problem, but that does not mean adding any-old label to each input magically fixes the problem. The labels have to be helpful of they are just distracting clutter.

          If this issue is typical of the quality of work done, I am a bit worried about the thought that there have been lots of issues recently adding lots of labels all over the place.

          Has anyone at HQ ever tried to use a screen-reader themselves? Do you have access to people who are experts on accessibility? I had a long discussion about this sort of question accessibility problem, and we were not able to come up with a good solution yet, which is why I have not fixed these myself ages ago. (This issues is a duplicate of several existing ones http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=component+IN+%28Quiz%2C+Questions%29+AND+component+%3D+Accessibility+And+resolution+%3D+Unresolved).

          Adding Michael D as a watcher so he is aware of the issue.

          Show
          Tim Hunt added a comment - Sorry, but -1 from me. The labels that have been added are completely meaningless. To start with, look at match. You have added a label: "Select an answer". Come on! The input control is a <select> menu. People who use a screen-reader have difficulty seeing / reading. They have not had their brain surgically removed. Of course you use a select menu in a question to select your answer. What they need to know is, which question should they be answering with this select menu. In a matching question there are several inputs, adding the same label "Select an answer" does nothing to help anyone. The <label> needs to go around the subquestion text. Unfortunately, you can't just do that, because the subquestion can contain arbitrary HTML like <p> tags, and <label> is an inline-level HTML element. At least it was in HTML 4. I keep meaning to check if that restriction is still there for valid HTML 5. Then we get to shortanswer. You are using $inputattributes ['name'] as the label text. Err, what! That is the name="..." attribute from the HTML - a variable name, not anything the user should see. It will typically look like q123:456_answer. I know improving Moodle accessibility is important, and that unlabelled inputs are an accessibility problem, but that does not mean adding any-old label to each input magically fixes the problem. The labels have to be helpful of they are just distracting clutter. If this issue is typical of the quality of work done, I am a bit worried about the thought that there have been lots of issues recently adding lots of labels all over the place. Has anyone at HQ ever tried to use a screen-reader themselves? Do you have access to people who are experts on accessibility? I had a long discussion about this sort of question accessibility problem, and we were not able to come up with a good solution yet, which is why I have not fixed these myself ages ago. (This issues is a duplicate of several existing ones http://tracker.moodle.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=component+IN+%28Quiz%2C+Questions%29+AND+component+%3D+Accessibility+And+resolution+%3D+Unresolved ). Adding Michael D as a watcher so he is aware of the issue.
          Hide
          Dan Poltawski added a comment -

          Reopening based on tims comments on this.

          Show
          Dan Poltawski added a comment - Reopening based on tims comments on this.
          Hide
          Michael de Raadt added a comment -

          Carrying over to the next sprint.

          Show
          Michael de Raadt added a comment - Carrying over to the next sprint.
          Hide
          Frédéric Massart added a comment -

          Hi, so I am pushing for peer review again. I will backport the branches if it passes this step. A few remarks:

          • In XHTML export I have set a basic label because the class accesshide is not supported as it is exported. And as the label is displayed I thought that it would look silly to be too explicit.
          • In the multianswer question, AFAIK there is no 'question' name hence the generic label
          • Most other places also have a generic label. I thought that having the question text/name would require a formatting to remove <div><p> etc... and could be too long. Also, as the select/input is positioned close to the question text/name itself, the user should really know what he's doing.

          Up to you!

          Show
          Frédéric Massart added a comment - Hi, so I am pushing for peer review again. I will backport the branches if it passes this step. A few remarks: In XHTML export I have set a basic label because the class accesshide is not supported as it is exported. And as the label is displayed I thought that it would look silly to be too explicit. In the multianswer question, AFAIK there is no 'question' name hence the generic label Most other places also have a generic label. I thought that having the question text/name would require a formatting to remove <div><p> etc... and could be too long. Also, as the select/input is positioned close to the question text/name itself, the user should really know what he's doing. Up to you!
          Hide
          Tim Hunt added a comment -

          How many times do I have to type the same explanation? http://tracker.moodle.org/browse/MDL-34570?focusedCommentId=171923&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-171923. See also the comments on MDL-34568.

          I am not going to point out all the problems with your patch one-by-one (there are many). Please think about it.

          Show
          Tim Hunt added a comment - How many times do I have to type the same explanation? http://tracker.moodle.org/browse/MDL-34570?focusedCommentId=171923&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-171923 . See also the comments on MDL-34568 . I am not going to point out all the problems with your patch one-by-one (there are many). Please think about it.
          Hide
          Michael de Raadt added a comment -

          Hi, Tim.

          You are being too harsh. We're grateful that you are reviewing this, you are not helping by being condescending.

          Fred is trying to follow the advice that you are giving, but in some cases what you are suggesting, while correct, is unnecessary and will impact on the interface for users of visual browsers.

          If you're going to provide feedback, please make it constructive or you may find other developers avoiding working with you.

          Show
          Michael de Raadt added a comment - Hi, Tim. You are being too harsh. We're grateful that you are reviewing this, you are not helping by being condescending. Fred is trying to follow the advice that you are giving, but in some cases what you are suggesting, while correct, is unnecessary and will impact on the interface for users of visual browsers. If you're going to provide feedback, please make it constructive or you may find other developers avoiding working with you.
          Hide
          Tim Hunt added a comment -

          Sorry about that last comment. I was tired last night, which is no excuse. More to the point, I know Fréd normally does better work than that, so I was disappointed.

          1. https://github.com/FMCorz/moodle/compare/MDL-34570-master#L1R97 - the label is obviously a good thing. I just wonder if it should be accesshidden. In a formslib form, that label would be visible to everyone.

          Should you use exactly the same lang string as formslib, rather than a new string?

          (These comments also apply to the qtype essay changes.)

          2. I am sure adding identical labels "Matching answer" https://github.com/FMCorz/moodle/compare/MDL-34570-master#L7R83 to each dropdown in the question does absolutely nothing to help accessibility. Better to do nothing than to do this.

          I was hoping that HTML5 would allow labels to contain 'Flow content' rather than just 'Phrasing content', then we could fix this well by using the subquestion text as the label, but it seems they have not done that. (http://dev.w3.org/html5/spec/the-label-element.html#the-label-element).

          The same problem affects generic 'answer' labels for multianswer.

          3. In shortanswer (and numerical, calculated) there are two ways things can be displayed.

          a. Using a placeholder ______ in the question text, in which case the input box is shown there. In that case, probably the only option is an accesshide "Answer" label.

          b. No placeholder in the question text. In that case the input is shown after the question text. In that case the text Answer is already shown on the screen, and that should be the label.

          numerical & calculated are complicated by the fact that the units may be a separate input, or there may be just one text box.

          Show
          Tim Hunt added a comment - Sorry about that last comment. I was tired last night, which is no excuse. More to the point, I know Fréd normally does better work than that, so I was disappointed. 1. https://github.com/FMCorz/moodle/compare/MDL-34570-master#L1R97 - the label is obviously a good thing. I just wonder if it should be accesshidden. In a formslib form, that label would be visible to everyone. Should you use exactly the same lang string as formslib, rather than a new string? (These comments also apply to the qtype essay changes.) 2. I am sure adding identical labels "Matching answer" https://github.com/FMCorz/moodle/compare/MDL-34570-master#L7R83 to each dropdown in the question does absolutely nothing to help accessibility. Better to do nothing than to do this. I was hoping that HTML5 would allow labels to contain 'Flow content' rather than just 'Phrasing content', then we could fix this well by using the subquestion text as the label, but it seems they have not done that. ( http://dev.w3.org/html5/spec/the-label-element.html#the-label-element ). The same problem affects generic 'answer' labels for multianswer. 3. In shortanswer (and numerical, calculated) there are two ways things can be displayed. a. Using a placeholder ______ in the question text, in which case the input box is shown there. In that case, probably the only option is an accesshide "Answer" label. b. No placeholder in the question text. In that case the input is shown after the question text. In that case the text Answer is already shown on the screen, and that should be the label. numerical & calculated are complicated by the fact that the units may be a separate input, or there may be just one text box.
          Hide
          Frédéric Massart added a comment -

          Hi Tim. Thank you for your feedback, it was really helpful!

          1. Done!

          2. I agree with you, that does not help very much but at least it is an information about the purpose of the select, and I can't think of anything better at the moment.

          3. I didn't know about the placeholder. It is now using accesshide with placeholders and the existing label without. I had to change the string used because Answer: {$a} was not flexible enough to be used with label.

          Show
          Frédéric Massart added a comment - Hi Tim. Thank you for your feedback, it was really helpful! 1. Done! 2. I agree with you, that does not help very much but at least it is an information about the purpose of the select, and I can't think of anything better at the moment. 3. I didn't know about the placeholder. It is now using accesshide with placeholders and the existing label without. I had to change the string used because Answer: {$a} was not flexible enough to be used with label.
          Hide
          Tim Hunt added a comment -

          As we just discussed on Jabber:

          2. change match label to Answer {$a}. Sadly, it is hard to make the same change in multianswer, so we won't bother for now.

          3. Improve short-answer code more, to eliminate the string concatenation. This includes hanging one <div> to <span>

          Also, amend the commit comment to include the word question somewhere.

          Show
          Tim Hunt added a comment - As we just discussed on Jabber: 2. change match label to Answer {$a}. Sadly, it is hard to make the same change in multianswer, so we won't bother for now. 3. Improve short-answer code more, to eliminate the string concatenation. This includes hanging one <div> to <span> Also, amend the commit comment to include the word question somewhere.
          Hide
          Frédéric Massart added a comment -

          Thanks Tim. As discussed, amended, modified and patched, pushing for integration!

          Show
          Frédéric Massart added a comment - Thanks Tim. As discussed, amended, modified and patched, pushing for integration!
          Hide
          Tim Hunt added a comment -

          Ah, I just found a duplicate for the matching qtype issue: MDL-30848. That was reported by an accessibility expert, and he points out the aria-labelledby attribute, that lets us fix this better. I will leave MDL-30848 open, rather than closing it as a duplicate of this, to remind me to implement the better solution one day.

          Show
          Tim Hunt added a comment - Ah, I just found a duplicate for the matching qtype issue: MDL-30848 . That was reported by an accessibility expert, and he points out the aria-labelledby attribute, that lets us fix this better. I will leave MDL-30848 open, rather than closing it as a duplicate of this, to remind me to implement the better solution one day.
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Aparup Banerjee added a comment -

          Fred is making some more changes here for master for better standard use of html_writer where possible. (as discussed)

          Show
          Aparup Banerjee added a comment - Fred is making some more changes here for master for better standard use of html_writer where possible. (as discussed)
          Hide
          Frédéric Massart added a comment -

          Pushed one more commit on master to use html_writer where some changes have been made.

          Show
          Frédéric Massart added a comment - Pushed one more commit on master to use html_writer where some changes have been made.
          Hide
          Aparup Banerjee added a comment -

          I think Tim Hunt has raised a very valid point here about whether anyone@HQ is using a screen reader here to test their patches aiming to improve accessibility here. And i'm not sure, but further testing would be beneficial if accessibility was the whole point for this meta.

          I've added Tim Barker as watcher here so he can organise some proper testing towards all these labelling patches with screen readers and/or other accessibility tools (perhaps even @ Meta MDL-34551) if necessary.

          Tim Barker: feel free to make it exploratory even.

          Show
          Aparup Banerjee added a comment - I think Tim Hunt has raised a very valid point here about whether anyone@HQ is using a screen reader here to test their patches aiming to improve accessibility here. And i'm not sure, but further testing would be beneficial if accessibility was the whole point for this meta. I've added Tim Barker as watcher here so he can organise some proper testing towards all these labelling patches with screen readers and/or other accessibility tools (perhaps even @ Meta MDL-34551 ) if necessary. Tim Barker: feel free to make it exploratory even.
          Hide
          Frédéric Massart added a comment -

          We should also have access to multiple servers, operating systems and database engines... just saying.

          Show
          Frédéric Massart added a comment - We should also have access to multiple servers, operating systems and database engines... just saying.
          Hide
          Aparup Banerjee added a comment -

          stopping review here for any further comment from Tim or Tim but mainly lack of time. (i need to get a screen reader setup on my mac too)

          Show
          Aparup Banerjee added a comment - stopping review here for any further comment from Tim or Tim but mainly lack of time. (i need to get a screen reader setup on my mac too)
          Hide
          Tim Hunt added a comment -

          I was actually not intending to comment further, having read your discussions, because you seem to be discussing the right questions, and I don't really have any answers at the moment.

          Show
          Tim Hunt added a comment - I was actually not intending to comment further, having read your discussions, because you seem to be discussing the right questions, and I don't really have any answers at the moment.
          Hide
          Tim Barker added a comment -

          I only just saw this. I think that this would need a bit more planning for testing because the use of external tools may be required.

          I'd like to take a look at what Fred has done myself before commenting further (i.e. me doing some pre-integration exploration) before I decide on a (lightweight) test plan for it.

          I think because this involves creating a page element we may be able to do some Selenium/Behat automation stuff with it.

          Show
          Tim Barker added a comment - I only just saw this. I think that this would need a bit more planning for testing because the use of external tools may be required. I'd like to take a look at what Fred has done myself before commenting further (i.e. me doing some pre-integration exploration) before I decide on a (lightweight) test plan for it. I think because this involves creating a page element we may be able to do some Selenium/Behat automation stuff with it.
          Hide
          Aparup Banerjee added a comment -

          Hi Timb,
          just to note, there have been many other MDLs created under the meta @ MDL-34551 and not just specific to this MDL so i'll comment there now.

          Show
          Aparup Banerjee added a comment - Hi Timb, just to note, there have been many other MDLs created under the meta @ MDL-34551 and not just specific to this MDL so i'll comment there now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P

          Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologises for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          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
          Dan Poltawski added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
          This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Tim Hunt added a comment -

          Fred, it sucks that this keeps not getting integrated. Anyway, there is a suggestion for a better way to do match questions using ARIA attributes in MDL-30848. Do you want to amend the fix for this bug to include that suggestion?

          (If not, after this has been integrated, I will do a separate fix for MDL-30848.)

          Show
          Tim Hunt added a comment - Fred, it sucks that this keeps not getting integrated. Anyway, there is a suggestion for a better way to do match questions using ARIA attributes in MDL-30848 . Do you want to amend the fix for this bug to include that suggestion? (If not, after this has been integrated, I will do a separate fix for MDL-30848 .)
          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
          Michael de Raadt added a comment -

          Bringing this into the current sprint so we don't lose sight of it.

          Show
          Michael de Raadt added a comment - Bringing this into the current sprint so we don't lose sight of it.
          Hide
          Dan Poltawski added a comment -

          Integrated to 22, 23 and master. This needs some good testing, good luck!

          Show
          Dan Poltawski added a comment - Integrated to 22, 23 and master. This needs some good testing, good luck!
          Hide
          Frédéric Massart added a comment -

          Hi Tim,

          sorry for replying a bit late to your comment. Even though this issue is not integrated, I think that would be better to take care of MDL-30848 in its own issue. I am not entirely convinced that labels should be entirely replaced by aria-labelledby. And I don't think this would have been backportable as those are HTML5 attributes.

          Thanks!

          Show
          Frédéric Massart added a comment - Hi Tim, sorry for replying a bit late to your comment. Even though this issue is not integrated, I think that would be better to take care of MDL-30848 in its own issue. I am not entirely convinced that labels should be entirely replaced by aria-labelledby. And I don't think this would have been backportable as those are HTML5 attributes. Thanks!
          Hide
          Jason Fowler added a comment -

          Good work Fred

          Show
          Jason Fowler added a comment - Good work Fred
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed, many thanks for your awesome collaboration.

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed, many thanks for your awesome collaboration.
          Hide
          Tim Hunt added a comment -

          This caused a regression: MDL-35858.

          Why was that not picked up in testing>

          Show
          Tim Hunt added a comment - This caused a regression: MDL-35858 . Why was that not picked up in testing>

            People

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

              Dates

              • Created:
                Updated:
                Resolved: