Moodle
  1. Moodle
  2. MDL-32946

Controls to edit blocks do not have accessible image alts

    Details

    • Database:
      Any
    • Testing Instructions:
      Hide
      1. Turn editing on for blocks
      2. Create a HTML block with quotes and double quotes in the title
      3. Hover over the Dock icon on the block, ensure the block's name is mentioned in the tool tip
      4. Turn editing on for blocks
      5. Hover over the Dock icon on the HTML block, ensure the block's name is mentioned in the tool tip
      Show
      Turn editing on for blocks Create a HTML block with quotes and double quotes in the title Hover over the Dock icon on the block, ensure the block's name is mentioned in the tool tip Turn editing on for blocks Hover over the Dock icon on the HTML block, ensure the block's name is mentioned in the tool tip
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-32946-master
    • Rank:
      40058

      Description

      Funcion edit_controls on blocklib.php prints control images that have the same alternative text to each block.

      Example:

      $controls[] = array('url' => $actionurl . '&bui_moveid=' . $block->instance->id,
                           'icon' => 't/move', 'caption' => get_string('move'));
      

      caption "get_string('move')" is the same to each block and is not accessible to be used.

      The solution is to add contextual information to each control this way screen readers can differentiate each control:

          get_string('moveblock','',$block->title)
      

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting this. It is related to recent work. We will hopefully get to this soon.

          Show
          Michael de Raadt added a comment - Thanks for reporting this. It is related to recent work. We will hopefully get to this soon.
          Hide
          Jason Fowler added a comment -

          must remember that all the javascript strings (dock and undock etc) need to be escaped to prevent quotes from breaking everything.

          Show
          Jason Fowler added a comment - must remember that all the javascript strings (dock and undock etc) need to be escaped to prevent quotes from breaking everything.
          Hide
          Jason Fowler added a comment -

          For the Javascript input I have altered the title, rather than the alt, as I feel the title is more appropriate in this case.

          Show
          Jason Fowler added a comment - For the Javascript input I have altered the title, rather than the alt, as I feel the title is more appropriate in this case.
          Hide
          Jason Fowler added a comment -

          The rest of the controls already have more accessible image alts (probably as a result of work done by Raj in the accessibility sprint).

          Show
          Jason Fowler added a comment - The rest of the controls already have more accessible image alts (probably as a result of work done by Raj in the accessibility sprint).
          Hide
          Jason Fowler added a comment -

          Master only patch due to the required string changes.

          Show
          Jason Fowler added a comment - Master only patch due to the required string changes.
          Hide
          Andrew Davis added a comment -

          You've got a couple of really long lines. If they could be broken up that would be nice.

          Does the string 'this' exist in any other lang file? Would it be advisable to use amos in the commit message to copy one of those strings so that we get existing translations for free?

          You're missing testing instructions.

          Show
          Andrew Davis added a comment - You've got a couple of really long lines. If they could be broken up that would be nice. Does the string 'this' exist in any other lang file? Would it be advisable to use amos in the commit message to copy one of those strings so that we get existing translations for free? You're missing testing instructions.
          Hide
          Jason Fowler added a comment - - edited

          I can't find an existing string for 'this' anywhere in the code. Not sure what you mean about AMOS in any case, could you please elaborate on that?

          Will look at breaking up the lines and adding testing instructions now.

          Thanks for the review Andrew.

          Show
          Jason Fowler added a comment - - edited I can't find an existing string for 'this' anywhere in the code. Not sure what you mean about AMOS in any case, could you please elaborate on that? Will look at breaking up the lines and adding testing instructions now. Thanks for the review Andrew.
          Hide
          Andrew Davis added a comment -

          http://docs.moodle.org/dev/Commit_cheat_sheet#Include_AMOS_script_in_the_commit_if_needed
          re amos if we already have a string in one lang file and you need an identical string you put something in your commit message so that amos will create the new as a copy of the old one. The point of that being that if the old string already has Japanese and French translations your new string will get those same translations for free instead of a human being having to come along and do them.

          Anyhow, submit for integration whenever you think this is ready to go.

          Show
          Andrew Davis added a comment - http://docs.moodle.org/dev/Commit_cheat_sheet#Include_AMOS_script_in_the_commit_if_needed re amos if we already have a string in one lang file and you need an identical string you put something in your commit message so that amos will create the new as a copy of the old one. The point of that being that if the old string already has Japanese and French translations your new string will get those same translations for free instead of a human being having to come along and do them. Anyhow, submit for integration whenever you think this is ready to go.
          Hide
          Jason Fowler added a comment -

          cool, thanks for the link, will remember that in the future.

          Show
          Jason Fowler added a comment - cool, thanks for the link, will remember that in the future.
          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 -

          Hi Jason, I'm sorry but I don't think we can replace the title of the block by "this" at all. For example, in Spanish we say (for "move"):

          • "Mover el bloque XXXXXXX",

          but when no name exists, the phrase is

          • "Mover este bloque".

          Aka, different positions, so no way to proceed with that replacement / string construction.

          So I bet that only having 2 different strings would make the trick, one for "titled" blocks and another for untitled ones.

          Alternatively... you can try to find always a title, so for example, given one html block without title, instead of trying to say "move this block" you can say "move html block" (using the type of the block as title).

          Note this not only affects undock/move but all the accessible buttons. Just your "this" invention made me to think about that.

          So reopening... feel free to discuss and agree about any of the alternatives above, or another better one.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Jason, I'm sorry but I don't think we can replace the title of the block by "this" at all. For example, in Spanish we say (for "move"): "Mover el bloque XXXXXXX", but when no name exists, the phrase is "Mover este bloque". Aka, different positions, so no way to proceed with that replacement / string construction. So I bet that only having 2 different strings would make the trick, one for "titled" blocks and another for untitled ones. Alternatively... you can try to find always a title, so for example, given one html block without title, instead of trying to say "move this block" you can say "move html block" (using the type of the block as title). Note this not only affects undock/move but all the accessible buttons. Just your "this" invention made me to think about that. So reopening... feel free to discuss and agree about any of the alternatives above, or another better one. Ciao
          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
          Jason Fowler added a comment -

          I think what I will do is create an alternative string ... "Move $

          {a}

          block" will stay and I will add "Move this block" which can be arranged better for different languages.

          Show
          Jason Fowler added a comment - I think what I will do is create an alternative string ... "Move $ {a} block" will stay and I will add "Move this block" which can be arranged better for different languages.
          Hide
          Jason Fowler added a comment -

          Changed, and pushed. Back for peer review.

          Show
          Jason Fowler added a comment - Changed, and pushed. Back for peer review.
          Hide
          Andrew Davis added a comment -

          You are go for integration.

          Show
          Andrew Davis added a comment - You are go for integration.
          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
          Eloy Lafuente (stronk7) added a comment -

          Jason, am I confused or are you mixing issues? It seems that this issue was originally about the "move block" control (both the description, my early rejection and your answer are talking all them about "moving".

          But, instead, your patch is about "docking". Looks correct but it's not about "moving". Plz, can you clarify this?

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Jason, am I confused or are you mixing issues? It seems that this issue was originally about the "move block" control (both the description, my early rejection and your answer are talking all them about "moving". But, instead, your patch is about "docking". Looks correct but it's not about "moving". Plz, can you clarify this? TIA and ciao
          Hide
          Jason Fowler added a comment -

          I am really confused by this too Eloy, I remember writing the code you rejected, and fixing it. Can't understand why it isn't there now.

          I also remember that it was only a small part of this issue. The other parts are there now. The docking is correct, it is supposed to have the "move" code there too, if I am not forgetting something...

          Show
          Jason Fowler added a comment - I am really confused by this too Eloy, I remember writing the code you rejected, and fixing it. Can't understand why it isn't there now. I also remember that it was only a small part of this issue. The other parts are there now. The docking is correct, it is supposed to have the "move" code there too, if I am not forgetting something...
          Hide
          Jason Fowler added a comment -

          On further inspection, the code was WAAAY wrong before these latest changes, I was "moving" when it should have been "docking". It's all good now Eloy. the code is correct. I just forgot to comment on the discovery I was working on the wrong thing.

          Show
          Jason Fowler added a comment - On further inspection, the code was WAAAY wrong before these latest changes, I was "moving" when it should have been "docking". It's all good now Eloy. the code is correct. I just forgot to comment on the discovery I was working on the wrong thing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          But... this does not have much sense... blocks without title... aren't dockable at all, so then... where is the point about to have the "dock this block" string?

          In the other side, other options, like the move/configure/delete/hide/assign roles have their icons there and all them are showing, for blocks without title:

          "action [whitespace] block"

          so there are not specialized "xxxxthisblock" strings for them, that I think it was the primary goal of this issue.

          Sorry, I'm reopening. The current patch is useless (blocks without title cannot be docked), and all the other editing options, present always, are not behaving correctly when the block has no title.

          Let's sort that 2nd problem, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - But... this does not have much sense... blocks without title... aren't dockable at all, so then... where is the point about to have the "dock this block" string? In the other side, other options, like the move/configure/delete/hide/assign roles have their icons there and all them are showing, for blocks without title: "action [whitespace] block" so there are not specialized "xxxxthisblock" strings for them, that I think it was the primary goal of this issue. Sorry, I'm reopening. The current patch is useless (blocks without title cannot be docked), and all the other editing options, present always, are not behaving correctly when the block has no title. Let's sort that 2nd problem, ciao
          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
          Jason Fowler added a comment - - edited

          In my investigations, this is not a problem. I haven't seen the icons with "Configure <ws> block" or anything. I am looking at my branch, and it includes Raj's previous patch (as referenced in my comment from 11/Mar/13 10:59 AM) so I don't see what you are talking about here Eloy.

          And the primary goal of this issue was to put the title (where there is one, or the type of the block where there isn't a title) into the move-to-dock icon, as Raj had already handled the rest.

          Show
          Jason Fowler added a comment - - edited In my investigations, this is not a problem. I haven't seen the icons with "Configure <ws> block" or anything. I am looking at my branch, and it includes Raj's previous patch (as referenced in my comment from 11/Mar/13 10:59 AM) so I don't see what you are talking about here Eloy. And the primary goal of this issue was to put the title (where there is one, or the type of the block where there isn't a title) into the move-to-dock icon, as Raj had already handled the rest.
          Hide
          Jason Fowler added a comment -

          I have removed the dockthisblock functionality. pushing for integration. Again.

          Show
          Jason Fowler added a comment - I have removed the dockthisblock functionality. pushing for integration. Again.
          Hide
          Damyon Wiese added a comment -

          Hi Jason,

          What are you trying to do here?

          Adding contextual infomation with title attributes is not recommended by WCAG.

          http://www.w3.org/TR/WCAG20-TECHS/H33.html

          You already have alt tags on the images - why don't they have the extra information? This is even in the title of the bug!

          You also have not addressed Eloys comments above. If you add a html block and do not give it a title, all of the links (movetodock included) will say "delete block" which provides no context. I would suggest you use the pluginname in this situation.

          There are also issues with the javascript in this patch:

          "._node.innerHTML"
          

          Please use public methods on the class - not private class variables (Even though in js they aren't very private).

                  if (node.one('.header .title h2')) {  
                      var blocktitle = Y.Escape.html(M.util.get_string('dockblock', 'block', node.one('.header .title h2')._node.innerHTML)) ;
                  }                                                                                                                           
                  // Must set the image src seperatly of we get an error with XML strict headers                                              
                  var moveto = Y.Node.create('<input type="image" class="moveto customcommand requiresjs" alt="'+M.str.block.addtodock+'" title="'+blocktitle+'" />');
          

          The scope for blocktitle is wrong.

          Show
          Damyon Wiese added a comment - Hi Jason, What are you trying to do here? Adding contextual infomation with title attributes is not recommended by WCAG. http://www.w3.org/TR/WCAG20-TECHS/H33.html You already have alt tags on the images - why don't they have the extra information? This is even in the title of the bug! You also have not addressed Eloys comments above. If you add a html block and do not give it a title, all of the links (movetodock included) will say "delete block" which provides no context. I would suggest you use the pluginname in this situation. There are also issues with the javascript in this patch: "._node.innerHTML" Please use public methods on the class - not private class variables (Even though in js they aren't very private). if (node.one('.header .title h2')) { var blocktitle = Y.Escape.html(M.util.get_string('dockblock', 'block', node.one('.header .title h2')._node.innerHTML)) ; } // Must set the image src seperatly of we get an error with XML strict headers var moveto = Y.Node.create('<input type= "image" class= "moveto customcommand requiresjs" alt= "'+M.str.block.addtodock+'" title= "'+blocktitle+'" />'); The scope for blocktitle is wrong.
          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
          Jason Fowler added a comment -

          As I stated in previous comments, I've looked at the code for the "If you add a html block and do not give it a title, all of the links (movetodock included) will say "delete block" which provides no context" issue, I can't see what you are talking about. I've spoken to Raj, and we have tried to replicate that, and we couldn't.

          As for the rest of it, these titles match the other elements in the dock controls, that was what I was trying to achieve.

          Show
          Jason Fowler added a comment - As I stated in previous comments, I've looked at the code for the "If you add a html block and do not give it a title, all of the links (movetodock included) will say "delete block" which provides no context" issue, I can't see what you are talking about. I've spoken to Raj, and we have tried to replicate that, and we couldn't. As for the rest of it, these titles match the other elements in the dock controls, that was what I was trying to achieve.
          Hide
          Jason Fowler added a comment -

          Also, that file you linked to talks about titles on links with text, not inputs with images as the rendered output. These are entirely different situations, and the alternative suggestions, such as using CSS to hide the additional text of the link don't apply.

          Show
          Jason Fowler added a comment - Also, that file you linked to talks about titles on links with text, not inputs with images as the rendered output. These are entirely different situations, and the alternative suggestions, such as using CSS to hide the additional text of the link don't apply.
          Hide
          Jason Fowler added a comment -

          also, what is wrong with "._node.innerHTML" it's working in all the browsers I tested it under. Could you explain what the "issues with the javascript" you've discovered are?

          Show
          Jason Fowler added a comment - also, what is wrong with "._node.innerHTML" it's working in all the browsers I tested it under. Could you explain what the "issues with the javascript" you've discovered are?
          Hide
          Jason Fowler added a comment -

          just figured out how to reproduce the HTML no title thing you've spoken about. Why wasn't this caught when Raj submitted his patch for the titles etc during the Accessibility Sprint?

          Show
          Jason Fowler added a comment - just figured out how to reproduce the HTML no title thing you've spoken about. Why wasn't this caught when Raj submitted his patch for the titles etc during the Accessibility Sprint?
          Hide
          Jason Fowler added a comment -

          https://tracker.moodle.org/browse/MDL-35832 shows that the group assessing Moodle for accessibility believe the controls for undocking blocks needed a title, and I was told the dock needed it too. This was where the work went, as Raj's issue https://tracker.moodle.org/browse/MDL-35873 was supposed to have handled everything else.

          Show
          Jason Fowler added a comment - https://tracker.moodle.org/browse/MDL-35832 shows that the group assessing Moodle for accessibility believe the controls for undocking blocks needed a title, and I was told the dock needed it too. This was where the work went, as Raj's issue https://tracker.moodle.org/browse/MDL-35873 was supposed to have handled everything else.
          Hide
          Jason Fowler added a comment -

          Fixed the missing HTML title issue, resolved the "out of scope blocktitle issue" still waiting on feedback about the _node.innerHTML problem Damyon mentioned.

          Show
          Jason Fowler added a comment - Fixed the missing HTML title issue, resolved the "out of scope blocktitle issue" still waiting on feedback about the _node.innerHTML problem Damyon mentioned.
          Hide
          Damyon Wiese added a comment -

          _ in js is equivalent to a private variable (or that's its intent anyway). While it might work now - YUI is free to change _node to _Node or whatever they want.

          Use getHTML() and you are done.

          Show
          Damyon Wiese added a comment - _ in js is equivalent to a private variable (or that's its intent anyway). While it might work now - YUI is free to change _node to _Node or whatever they want. Use getHTML() and you are done.
          Hide
          Damyon Wiese added a comment -

          This has been integrated. I had to add a fix to make the use of alt and title consistent.

          Tested on NVDA.

          Show
          Damyon Wiese added a comment - This has been integrated. I had to add a fix to make the use of alt and title consistent. Tested on NVDA.
          Hide
          Dan Poltawski added a comment -

          It passes - however i noticed while testing the hide/show thing is not properly escaping.

          Show
          Dan Poltawski added a comment - It passes - however i noticed while testing the hide/show thing is not properly escaping.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!
          Hide
          Matteo Scaramuccia added a comment -

          Hi all,
          for the (my?) record: updating Moodle from 2.5beta+ (Build: 20130418) (2013041801.00) to 2.5beta+ (Build: 20130426) - being a new string added here: https://github.com/moodle/moodle/commit/473327030e370569116a2018f4b05ce10cdd6dac#L2R46 - returns the notice below due to the out-of-date strings cache; MDL-39343 will help on this too.

          dockblock/block: from version 2.5beta+ (Build: 20130418) (2013041801.00) to 2.5beta+ (Build: 20130426) (2013042600.00)
          String does not exist. Please check your string definition for dockblock/block
          line 11396 of \lib\moodlelib.php: call to debugging()
          line 1088 of \lib\outputrequirementslib.php: call to lang_string->__construct()
          line 767 of \lib\outputrequirementslib.php: call to page_requirements_manager->string_for_js()
          line 248 of \lib\outputrequirementslib.php: call to page_requirements_manager->js_module()
          line 600 of \lib\pagelib.php: call to page_requirements_manager->__construct()
          line 717 of \lib\pagelib.php: call to moodle_page->magic_get_requires()
          line 440 of \blocks\moodleblock.class.php: call to moodle_page->__get()
          line 765 of \blocks\moodleblock.class.php: call to block_base->get_required_javascript()
          line 238 of \blocks\moodleblock.class.php: call to block_list->formatted_contents()
          line 951 of \lib\blocklib.php: call to block_base->get_content_for_output()
          line 1003 of \lib\blocklib.php: call to block_manager->create_block_contents()
          line 353 of \lib\blocklib.php: call to block_manager->ensure_content_created()
          line 3 of \theme\base\layout\frontpage.php: call to block_manager->region_has_content()
          line 841 of \lib\outputrenderers.php: call to include()
          line 771 of \lib\outputrenderers.php: call to core_renderer->render_page_layout()
          line 99 of \index.php: call to core_renderer->header()
          
          Show
          Matteo Scaramuccia added a comment - Hi all, for the (my?) record: updating Moodle from 2.5beta+ (Build: 20130418) (2013041801.00) to 2.5beta+ (Build: 20130426) - being a new string added here: https://github.com/moodle/moodle/commit/473327030e370569116a2018f4b05ce10cdd6dac#L2R46 - returns the notice below due to the out-of-date strings cache; MDL-39343 will help on this too. dockblock/block: from version 2.5beta+ (Build: 20130418) (2013041801.00) to 2.5beta+ (Build: 20130426) (2013042600.00) String does not exist. Please check your string definition for dockblock/block line 11396 of \lib\moodlelib.php: call to debugging() line 1088 of \lib\outputrequirementslib.php: call to lang_string->__construct() line 767 of \lib\outputrequirementslib.php: call to page_requirements_manager->string_for_js() line 248 of \lib\outputrequirementslib.php: call to page_requirements_manager->js_module() line 600 of \lib\pagelib.php: call to page_requirements_manager->__construct() line 717 of \lib\pagelib.php: call to moodle_page->magic_get_requires() line 440 of \blocks\moodleblock.class.php: call to moodle_page->__get() line 765 of \blocks\moodleblock.class.php: call to block_base->get_required_javascript() line 238 of \blocks\moodleblock.class.php: call to block_list->formatted_contents() line 951 of \lib\blocklib.php: call to block_base->get_content_for_output() line 1003 of \lib\blocklib.php: call to block_manager->create_block_contents() line 353 of \lib\blocklib.php: call to block_manager->ensure_content_created() line 3 of \theme\base\layout\frontpage.php: call to block_manager->region_has_content() line 841 of \lib\outputrenderers.php: call to include() line 771 of \lib\outputrenderers.php: call to core_renderer->render_page_layout() line 99 of \index.php: call to core_renderer->header()
          Hide
          Damyon Wiese added a comment -

          Thanks Matteo - I just should have added a version bump when I integrated this change (but just for people tracking the beta).

          Show
          Damyon Wiese added a comment - Thanks Matteo - I just should have added a version bump when I integrated this change (but just for people tracking the beta).

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: