Moodle
  1. Moodle
  2. MDL-31040

mouseover text on group toggle button inconsistant and breaks RTL support

    Details

    • Testing Instructions:
      Hide

      Replication Instructions

      • Disable AJAX in your profile
      • Navigate to a course
      • Turn editing on
      • Inspect a Group Mode toggle icon for an activity in your browser (I'm using Chrome)
        • You'll find that the text for the anchor and image's alt and title text differs
      • Note that the icon and parent anchor have a mixture of titles
      • Edit the Course Settings to set "Force Group Mode" to "Yes", and 'Group Mode' to something other than 'No Groups'
      • Inspect a Group Mode toggle icon for an activity in your browser (I'm using Chrome)
        • You'll find that the text for the anchor and image's alt and title text differs
      • Edit the Course Settings to set the Course Format to 'Social'
      • Add the 'Social Activities' block
      • Inspect a Group Mode toggle icon for an activity in the block
        • You'll find that the text for the anchor and image's alt and title text differs
      • Turn AJAX back on in your profile
      • Refresh the page
      • Rinse, Wash Repeat

      Testing the fix

      The above instructions still apply, but the alt and title text should no longer differ

      Show
      Replication Instructions Disable AJAX in your profile Navigate to a course Turn editing on Inspect a Group Mode toggle icon for an activity in your browser (I'm using Chrome) You'll find that the text for the anchor and image's alt and title text differs Note that the icon and parent anchor have a mixture of titles Edit the Course Settings to set "Force Group Mode" to "Yes", and 'Group Mode' to something other than 'No Groups' Inspect a Group Mode toggle icon for an activity in your browser (I'm using Chrome) You'll find that the text for the anchor and image's alt and title text differs Edit the Course Settings to set the Course Format to 'Social' Add the 'Social Activities' block Inspect a Group Mode toggle icon for an activity in the block You'll find that the text for the anchor and image's alt and title text differs Turn AJAX back on in your profile Refresh the page Rinse, Wash Repeat Testing the fix The above instructions still apply, but the alt and title text should no longer differ
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31040-master-2
    • Rank:
      37448

      Description

      When in editing mode, the alt and title text for the image and anchor for the groupmode toggle is inconsistant
      The anchor title is "<mode> (Click to change)" whilst the title and alt for the image is simply "<mode>"

      with ajax enabled, all are set to the "<mode> (Click to change)" version, but the way in which this is implemented breaks RTL support as it uses something akin to:

      <mode> . ' (' . get_string('clicktochange') . ')'
      

      in both the JS and PHP

        Issue Links

          Activity

          Hide
          Andrew Nicols added a comment -

          I've been working on a patch for this and will submit in the morning when I've finished testing it

          Show
          Andrew Nicols added a comment - I've been working on a patch for this and will submit in the morning when I've finished testing it
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this.

          Show
          Michael de Raadt added a comment - Thanks for working on this.
          Hide
          Andrew Nicols added a comment -

          I've created a patch which addresses both of these issues and another related issue.
          Please note that it's not possible to use the M.util.get_string() functions here because they're loaded too late for the existing code in section_classes.js so I've resorted to changing the existing strings to suit.

          I've started work on migrating the existing YUI2 code in lib/ajax/section_classes.js to a YUI3 module in course/yui which will resolve this issue properly for 2.3 onwards.

          Show
          Andrew Nicols added a comment - I've created a patch which addresses both of these issues and another related issue. Please note that it's not possible to use the M.util.get_string() functions here because they're loaded too late for the existing code in section_classes.js so I've resorted to changing the existing strings to suit. I've started work on migrating the existing YUI2 code in lib/ajax/section_classes.js to a YUI3 module in course/yui which will resolve this issue properly for 2.3 onwards.
          Hide
          Andrew Davis added a comment -

          Generally, don't put replication instructions in the test instructions field. If however the testing instructions are effectively just the replication instructions but with a slightly different result you can refer the tester to the replication instructions with an explanation of what should be different.

          The code is no longer using 'forcedmode'. Instead it uses the new string 'forcedmodeinbrackets'. I can't see anywhere else that uses the string 'forcedmode'. If its not being used remove it from the lang file. Same for 'clicktochange'. If they're genuinely not being used its always nice to get rid of strings to save people wasting time translating them into other languages.

          Show
          Andrew Davis added a comment - Generally, don't put replication instructions in the test instructions field. If however the testing instructions are effectively just the replication instructions but with a slightly different result you can refer the tester to the replication instructions with an explanation of what should be different. The code is no longer using 'forcedmode'. Instead it uses the new string 'forcedmodeinbrackets'. I can't see anywhere else that uses the string 'forcedmode'. If its not being used remove it from the lang file. Same for 'clicktochange'. If they're genuinely not being used its always nice to get rid of strings to save people wasting time translating them into other languages.
          Hide
          Andrew Nicols added a comment -

          I've updated the commit to remove the 'forcedmode' and 'clicktochange' strings – I'd not removed them before as I wasn't sure what the deal was with removing language strings.

          I've also rebased on the latest weeklies.

          In addition to master, this should cherry-pick cleanly onto MOODLE_22_STABLE.
          I've provided a separate MOODLE_21_STABLE branch as this doesn't cherry-pick cleanly onto 21

          Show
          Andrew Nicols added a comment - I've updated the commit to remove the 'forcedmode' and 'clicktochange' strings – I'd not removed them before as I wasn't sure what the deal was with removing language strings. I've also rebased on the latest weeklies. In addition to master, this should cherry-pick cleanly onto MOODLE_22_STABLE. I've provided a separate MOODLE_21_STABLE branch as this doesn't cherry-pick cleanly onto 21
          Hide
          Dan Poltawski added a comment -

          Adding David here..

          David - Andrew asked me to submit for integration for I just wanted to pick your expertise before doing this. Is this sort of change better in master only because of the ability to break non-english language strings - is it better we introduce a new string?? The problem I see with doing it in the stable branch that we break <2.2.3 for example

          Show
          Dan Poltawski added a comment - Adding David here.. David - Andrew asked me to submit for integration for I just wanted to pick your expertise before doing this. Is this sort of change better in master only because of the ability to break non-english language strings - is it better we introduce a new string?? The problem I see with doing it in the stable branch that we break <2.2.3 for example
          Hide
          David Mudrak added a comment -

          Summarizing the discussion that just happened in the chat:

          1. remove the strings from master only
          2. do not remove them on stable branches
          3. let me know that I shall put the strings on a greylist in AMOS for 2.1 and 2.2

          Show
          David Mudrak added a comment - Summarizing the discussion that just happened in the chat: 1. remove the strings from master only 2. do not remove them on stable branches 3. let me know that I shall put the strings on a greylist in AMOS for 2.1 and 2.2
          Hide
          Andrew Nicols added a comment -

          Updated branches as discussed with Dan and David in the developer chat.

          The 21 branch only adds strings but the code is different to 22 and master
          The 22 branch only adds strings but has the same code as master
          The master branch removes the old string, and adds new strings

          Show
          Andrew Nicols added a comment - Updated branches as discussed with Dan and David in the developer chat. The 21 branch only adds strings but the code is different to 22 and master The 22 branch only adds strings but has the same code as master The master branch removes the old string, and adds new strings
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
          Hide
          Rossiani Wijaya added a comment -

          This is working fine.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working fine. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now available in the git and cvs repositories.

          Consider the responsibility of your fingerprints engraved there for future generations!

          Thanks for the work, closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now available in the git and cvs repositories. Consider the responsibility of your fingerprints engraved there for future generations! Thanks for the work, closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: