Moodle
  1. Moodle
  2. MDL-35816 Accessibility Review issues (Deque)
  3. MDL-35826

Re-write calendar blocks to include the month heading as a caption within the calendar table

    Details

    • Rank:
      44587

      Description

      Issue
      Table structure - A table caption is presented for the month (calendar), but only coded inside a <a> element. Use <caption> instead.

      Standard Level
      WCAG 2 A 1.3.1 http://www.w3.org/TR/WCAG20/#content-structure-separation-programmatic

      Impact
      Moderate

      Example Link
      http://demo.moodle.net/ calendar block may need to be added

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          The month isn't even part of the table, does it really still need to be a caption for the table?

          Show
          Jason Fowler added a comment - The month isn't even part of the table, does it really still need to be a caption for the table?
          Hide
          Ankit Agarwal added a comment -

          Hi Jason,
          The patch fixes the issue in hand, however there are some things that needs to be discussed and decided.

          1. Is caption absolutely necessary for accessibility? Even at the cost of redundant data?
            As per http://webaim.org/techniques/tables/data#caption , it is just nice to have thing. And looking at the specific use case here, it doesn't make any sense to use it with 'accesshide'.
          2. I see two ways to this:-
            1. Make use of captions mandatory, which means have support for it on all table related apis, slowly rewrite places like calendar where table apis are not used and update all existing table uses.
            2. Declare it nice to have feature, in which case the table apis should be updated to support it, should be used in all new code and all messed up code like this issue should be left alone.

          In any case my -1 for existing solution.

          Thanks

          Show
          Ankit Agarwal added a comment - Hi Jason, The patch fixes the issue in hand, however there are some things that needs to be discussed and decided. Is caption absolutely necessary for accessibility? Even at the cost of redundant data? As per http://webaim.org/techniques/tables/data#caption , it is just nice to have thing. And looking at the specific use case here, it doesn't make any sense to use it with 'accesshide'. I see two ways to this:- Make use of captions mandatory, which means have support for it on all table related apis, slowly rewrite places like calendar where table apis are not used and update all existing table uses. Declare it nice to have feature, in which case the table apis should be updated to support it, should be used in all new code and all messed up code like this issue should be left alone. In any case my -1 for existing solution. Thanks
          Hide
          Jason Hardin added a comment -

          Just to follow up. The issue in the ticket is actually focus on the fact that the table doesn't have a caption tag with in it. Captions are used by screen readers to caption the table. Currently the screen reader uses the link with the month title as the caption simply because it is in the reading order. It will make the table compliant with WCAG 2 A by adding an official caption to the table. All tables that are not used for layout should have a caption and a summary.

          Show
          Jason Hardin added a comment - Just to follow up. The issue in the ticket is actually focus on the fact that the table doesn't have a caption tag with in it. Captions are used by screen readers to caption the table. Currently the screen reader uses the link with the month title as the caption simply because it is in the reading order. It will make the table compliant with WCAG 2 A by adding an official caption to the table. All tables that are not used for layout should have a caption and a summary.
          Hide
          Jason Hardin added a comment -

          To Ankit's comment. Based on what I have read in the removal of the AJAX and Screen reader setting in the user profile Moodle HQ is focused on providing 1 interface that is accessible. To me this means that all apis should force the developer to provide the maximum accessibility and not slack off. The web aim link says that caption is a nice to have and good practice. To me good practice means if you are dedicated to accessibility you include it. So I would vote for 2.1. This will help to future proof the apis if WCAG 3 requires captions in tables later.

          Show
          Jason Hardin added a comment - To Ankit's comment. Based on what I have read in the removal of the AJAX and Screen reader setting in the user profile Moodle HQ is focused on providing 1 interface that is accessible. To me this means that all apis should force the developer to provide the maximum accessibility and not slack off. The web aim link says that caption is a nice to have and good practice. To me good practice means if you are dedicated to accessibility you include it. So I would vote for 2.1. This will help to future proof the apis if WCAG 3 requires captions in tables later.
          Hide
          Jason Fowler added a comment -

          I've added a caption to the table in my solution, without removing the h3/anchor above it because the anchor is often accompanied by the Prev/Next buttons, both of which have no place being in the table in terms of semantic HTML. The caption is then shuffled out of sight using the .accesshide css selector. This prevents clutter on the screen, while allowing the table to conform to accessibility guidelines.

          The table itself isn't written by an API, but is just rows and cells output procedurally. There for there will not be an accessible interface change for this. It may be worth raising an issue to address this for other tables though.

          Show
          Jason Fowler added a comment - I've added a caption to the table in my solution, without removing the h3/anchor above it because the anchor is often accompanied by the Prev/Next buttons, both of which have no place being in the table in terms of semantic HTML. The caption is then shuffled out of sight using the .accesshide css selector. This prevents clutter on the screen, while allowing the table to conform to accessibility guidelines. The table itself isn't written by an API, but is just rows and cells output procedurally. There for there will not be an accessible interface change for this. It may be worth raising an issue to address this for other tables though.
          Hide
          Jason Fowler added a comment -

          After discussing this in scrum this morning, the idea was floated that the blocks be re-written to take away the header, allowing the caption to act as the header naturally, and then remove the < Prev Next > links from the calendar block, because the idea of a block reloading the current page is a bad one, according to Michael. I will be progressing with the changes, attempting to minimalize the changes have on themes, and if they are too great, make this a master only change and provide fixes for the themes along with it.

          Show
          Jason Fowler added a comment - After discussing this in scrum this morning, the idea was floated that the blocks be re-written to take away the header, allowing the caption to act as the header naturally, and then remove the < Prev Next > links from the calendar block, because the idea of a block reloading the current page is a bad one, according to Michael. I will be progressing with the changes, attempting to minimalize the changes have on themes, and if they are too great, make this a master only change and provide fixes for the themes along with it.
          Hide
          Jason Fowler added a comment -

          After looking at the additional functionality of the block, with the event filters etc, I have decided that semantic HTML needs to yield to usability and accessibility, and have moved the prev/next controls into the caption along with the date for the calendar.

          Show
          Jason Fowler added a comment - After looking at the additional functionality of the block, with the event filters etc, I have decided that semantic HTML needs to yield to usability and accessibility, and have moved the prev/next controls into the caption along with the date for the calendar.
          Hide
          Jason Fowler added a comment -

          Because this is more than just an output change, I am considering it being for master only. Not 100% set on that, but would like some feed back in regards to to back porting it before I go to the effort.

          Show
          Jason Fowler added a comment - Because this is more than just an output change, I am considering it being for master only. Not 100% set on that, but would like some feed back in regards to to back porting it before I go to the effort.
          Hide
          Ankit Agarwal added a comment - - edited

          Hi Jason,
          Here are a couple of things I noticed:-

          1. wasn’t sure if divs are allowed as children for caption, but it looks like they are, atleast I couldn't find anything saying they are not.
          2. The provided patch breaks backwards compatibility completely, am not sure integrators will like that.
          3. I don’t think we consider, having required arguments after optional arguments as a valid signature, since this effectively means all arguments are required.
          4. Also any api changes need to be stated in respective upgrade.txt

          So basically if you can get a plus one about your new function signature from an integrator, that will be great.

          Thanks

          Show
          Ankit Agarwal added a comment - - edited Hi Jason, Here are a couple of things I noticed:- wasn’t sure if divs are allowed as children for caption, but it looks like they are, atleast I couldn't find anything saying they are not. The provided patch breaks backwards compatibility completely, am not sure integrators will like that. I don’t think we consider, having required arguments after optional arguments as a valid signature, since this effectively means all arguments are required. Also any api changes need to be stated in respective upgrade.txt So basically if you can get a plus one about your new function signature from an integrator, that will be great. Thanks
          Hide
          Jason Fowler added a comment -

          Thanks for the point about the function signature, fixed that now. I am thinking this should be a master only patch, so backwards compatibility is only of minor concern.

          Show
          Jason Fowler added a comment - Thanks for the point about the function signature, fixed that now. I am thinking this should be a master only patch, so backwards compatibility is only of minor concern.
          Hide
          Jason Fowler added a comment -

          Just added to the upgrade.txt too. All ready to rumble.

          Show
          Jason Fowler added a comment - Just added to the upgrade.txt too. All ready to rumble.
          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
          Jason Fowler added a comment -

          Rebased

          Show
          Jason Fowler added a comment - Rebased
          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 -

          Ankit is exactly right. You can't just change the function definition like that, especially adding parameters at the start of the function.

          How is a third party developer going to discover they are passing the wrong arguments? I imagine with an obstuse php error about an array being used as scalar value.

          I suggest you introduce a new function and deprecate the old one to get around this, therefore ensuring that a third party developers code won't be immediately broken when they move to 2.5.

          Show
          Dan Poltawski added a comment - Ankit is exactly right. You can't just change the function definition like that, especially adding parameters at the start of the function. How is a third party developer going to discover they are passing the wrong arguments? I imagine with an obstuse php error about an array being used as scalar value. I suggest you introduce a new function and deprecate the old one to get around this, therefore ensuring that a third party developers code won't be immediately broken when they move to 2.5.
          Hide
          Jason Fowler added a comment -

          If I make the parameters optional, and move them to the end of the function signature, would that be acceptable? I object to complicating Moodle's codebase further by cloning an already existing function just to add a little extra functionality to it, even if it is only temporary.

          Show
          Jason Fowler added a comment - If I make the parameters optional, and move them to the end of the function signature, would that be acceptable? I object to complicating Moodle's codebase further by cloning an already existing function just to add a little extra functionality to it, even if it is only temporary.
          Hide
          Jason Fowler added a comment -

          Should be better now Dan, pushing for integration again.

          Show
          Jason Fowler added a comment - Should be better now Dan, pushing for integration again.
          Hide
          Sam Hemelryk added a comment -

          Thanks Jason, I've integrated this now.

          I did make one change, I gave $placement a default of false. This corrects the issue of having required params after optional params.
          As this will break any themes that have styled the calendar controls I think this is best not backported as well.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Jason, I've integrated this now. I did make one change, I gave $placement a default of false. This corrects the issue of having required params after optional params. As this will break any themes that have styled the calendar controls I think this is best not backported as well. Cheers Sam
          Hide
          Andrew Davis added a comment -

          I think this is ok although the testing instructions are rather vague. I couldn't get anything to not work. Passing.

          Show
          Andrew Davis added a comment - I think this is ok although the testing instructions are rather vague. I couldn't get anything to not work. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Surely you will be happy to know that your code is now part of Moodle upstream. Thanks, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: