Details

      Description

      Issue
      Link purpose - The "undock this item" links that are presented once more than one element has been docked are all labeled the same and therefore are not readily distinguishable. Should probably read Undock <block name> just like the dock icon reads Doc <block name>

      Standard Level
      WCAG 2 A 2.4.4http://www.w3.org/WAI/WCAG20/quickref/#qr-navigation-mechanisms-refs

      Impact
      Serious

      Example Link
      http://demo.moodle.net/

      Test Steps

      1. Dock a block
      2. Navigate into the block with a mouse
      3. Hover over undock icon and see the text Undock this item

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Jason Fowler added a comment - - edited

            The way block docking is currently implemented (entirely in javascript) means that the string can not be modified to include the name of the block (this is the same problem I was having with the comment delete icon) without rewriting the system. The work load is too great for the pay off at this point. I've spoken to Raj, and after some deliberation, we've decided that this can't be accomplished, I will bring it up with the rest of the team tomorrow, and see if any of them have some awesome idea on how this can be accomplished, but if not, then I will have to pass on the isssue.

            Show
            Jason Fowler added a comment - - edited The way block docking is currently implemented (entirely in javascript) means that the string can not be modified to include the name of the block (this is the same problem I was having with the comment delete icon) without rewriting the system. The work load is too great for the pay off at this point. I've spoken to Raj, and after some deliberation, we've decided that this can't be accomplished, I will bring it up with the rest of the team tomorrow, and see if any of them have some awesome idea on how this can be accomplished, but if not, then I will have to pass on the isssue.
            Hide
            Jason Fowler added a comment -

            The consensus from the scrum was that an attempt to pull the data from the DOM should be made to get the title. I will attempt this today.

            Show
            Jason Fowler added a comment - The consensus from the scrum was that an attempt to pull the data from the DOM should be made to get the title. I will attempt this today.
            Hide
            Jason Fowler added a comment -

            Turns out the solution for this didn't require traversing the DOM or rewriting the whole dock javascript system at all. most of the heavy lifting had already been done, it just wasn't apparent when I looked at it first.

            Show
            Jason Fowler added a comment - Turns out the solution for this didn't require traversing the DOM or rewriting the whole dock javascript system at all. most of the heavy lifting had already been done, it just wasn't apparent when I looked at it first.
            Hide
            Adrian Greeve added a comment -

            [Y] Syntax
            [Y] Output
            [N] Whitespace
            [N] Language
            [-] Databases
            [Y] Testing
            [-] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            Hi Jason,

            I think that this solution is really close, you just have a couple of things to fix up:

            • Line 907 of blocks/dock.js has excess whitespace.
            • Line 909 - You have concatenated three strings to create the title. I think that we try to avoid concatenating strings as it makes it difficult to translate it into other languages. Creating one string and putting a variable in it would be a much better idea and allow translations to work as well.

            Thanks.

            Show
            Adrian Greeve added a comment - [Y] Syntax [Y] Output [N] Whitespace [N] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Jason, I think that this solution is really close, you just have a couple of things to fix up: Line 907 of blocks/dock.js has excess whitespace. Line 909 - You have concatenated three strings to create the title. I think that we try to avoid concatenating strings as it makes it difficult to translate it into other languages. Creating one string and putting a variable in it would be a much better idea and allow translations to work as well. Thanks.
            Hide
            Jason Fowler added a comment -

            How do I make one string and put a variable in it within the javascript system? I didn't realise that was possible ...

            Show
            Jason Fowler added a comment - How do I make one string and put a variable in it within the javascript system? I didn't realise that was possible ...
            Hide
            Adrian Greeve added a comment -

            Look at course/yui/dragdrop.js line 85. This is an example of how to include a variable in a string.

            Show
            Adrian Greeve added a comment - Look at course/yui/dragdrop.js line 85. This is an example of how to include a variable in a string.
            Hide
            Jason Fowler added a comment -

            Thanks for pointing this out, the changes have now been made. Pushing for integration.

            Show
            Jason Fowler added a comment - Thanks for pointing this out, the changes have now been made. Pushing for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Sorry, I'm reopening this. With the patch applied, the docking stops working here (Safari, Chrome, Firefox), with Y.Escape.html undefined errors.

            Show
            Eloy Lafuente (stronk7) added a comment - Sorry, I'm reopening this. With the patch applied, the docking stops working here (Safari, Chrome, Firefox), with Y.Escape.html undefined errors.
            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'm not so sure that the Y.Escape.html is needed - the discussion I have had with Petr and Marina in the past indicated it wasn't, but Fred was insisting it be included. Either way, I am removing it now, and will push for integration shortly.

            Show
            Jason Fowler added a comment - I'm not so sure that the Y.Escape.html is needed - the discussion I have had with Petr and Marina in the past indicated it wasn't, but Fred was insisting it be included. Either way, I am removing it now, and will push for integration shortly.
            Hide
            Jason Fowler added a comment -

            Pushing again

            Show
            Jason Fowler added a comment - Pushing again
            Hide
            Damyon Wiese 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.

            Cheers!

            Show
            Damyon Wiese 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. Cheers!
            Hide
            Dan Poltawski added a comment -
            1. I suggest master only for this because this introduces a new string. (Its LESS accessible to have an untranslated string)
            2. Just saying you're not sure and removing the escaping isn't the way to get code into moodle core.. You need to justify your rationale and understand the issue. It looks to me like Fred's advice was right and that if someone translates that new string to Undock "$a" block then we'd end up with invalid html. Maybe you can test that and see.
            3. Do we still need M.str.block.undockitem as the alt text?
            Show
            Dan Poltawski added a comment - I suggest master only for this because this introduces a new string. (Its LESS accessible to have an untranslated string) Just saying you're not sure and removing the escaping isn't the way to get code into moodle core.. You need to justify your rationale and understand the issue. It looks to me like Fred's advice was right and that if someone translates that new string to Undock "$a" block then we'd end up with invalid html. Maybe you can test that and see. Do we still need M.str.block.undockitem as the alt text?
            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 -
            1. that's fine
            2. as stated, I had a discussion with Petr and Marina about this when working on strings for the file picker. Petr insisted that escaping strings was not necessary because all efforts to make strings safe is undertaken. I figured Fred's concern came from the use of a user contributed string as the title, but the JavaScript methods for retrieving that string result in a safe string anyway. Sorry I wasn't verbose enough in my explanation. I didn't just remove it because I wasn't sure. I decided it wasn't needed, had valid reasons for it, and removed it.
            3. Yes, in this case the alt and title attributes should be different.

            Resubmitted for integration.

            Show
            Jason Fowler added a comment - that's fine as stated, I had a discussion with Petr and Marina about this when working on strings for the file picker. Petr insisted that escaping strings was not necessary because all efforts to make strings safe is undertaken. I figured Fred's concern came from the use of a user contributed string as the title, but the JavaScript methods for retrieving that string result in a safe string anyway. Sorry I wasn't verbose enough in my explanation. I didn't just remove it because I wasn't sure. I decided it wasn't needed, had valid reasons for it, and removed it. Yes, in this case the alt and title attributes should be different. Resubmitted for integration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm integrating this to master... but...

            if the "undock" icon needed that change, I don't get why these other actions (also missing the block name) don't need to be "improved":

            • Move this to the block.
            • Hide the dock panel.

            IMO all them should be similar. For your consideration. +1 to create issue about that.

            Show
            Eloy Lafuente (stronk7) added a comment - I'm integrating this to master... but... if the "undock" icon needed that change, I don't get why these other actions (also missing the block name) don't need to be "improved": Move this to the block. Hide the dock panel. IMO all them should be similar. For your consideration. +1 to create issue about that.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (master only), thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (master only), thanks!
            Hide
            Jason Fowler added a comment -

            Thanks Eloy. There is already another issue for that. I have it in my next sprint.

            Show
            Jason Fowler added a comment - Thanks Eloy. There is already another issue for that. I have it in my next sprint.
            Hide
            Rossiani Wijaya added a comment -

            +1 for Eloy's suggestion.

            Jason, could you add the link to the other issues.

            The patch works as expected and tested it for master only.

            Test passed.

            Show
            Rossiani Wijaya added a comment - +1 for Eloy's suggestion. Jason, could you add the link to the other issues. The patch works as expected and tested it for master only. Test passed.
            Hide
            Damyon Wiese added a comment -

            Congratulations this fix has been added to Moodle!

            You may want to dedicate this issue to someone special on this Valentines day.

            Thanks!

            Show
            Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!
            Hide
            Frédéric Massart added a comment -

            I came across a line change in this issue, and realised we were not escape the values for the alt and title attributes. It should be done otherwise unexpected results can happen if the string uses single or double quotes. To replicate the problem, change the name of the navigation block:

            $string['pluginname'] = 'Navig\'ation';
            

            I think we should be using Y.Escape.html() any time, regardless of the origin of the string (From DOM, language string, ...).

            Show
            Frédéric Massart added a comment - I came across a line change in this issue, and realised we were not escape the values for the alt and title attributes. It should be done otherwise unexpected results can happen if the string uses single or double quotes. To replicate the problem, change the name of the navigation block: $string['pluginname'] = 'Navig\'ation'; I think we should be using Y.Escape.html() any time, regardless of the origin of the string (From DOM, language string, ...).
            Hide
            Jason Fowler added a comment -

            will fix this as part of MDL-32946

            Thanks for following up on this Fred.

            Show
            Jason Fowler added a comment - will fix this as part of MDL-32946 Thanks for following up on this Fred.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: