Moodle

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
  • Pull Master Diff URL:

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

has a non-specific relationship to

Improvement - An enhancement to an existing Moodle feature. MDL-31052 update Course AJAX/javascript to YUI3 and replace section_classes.js

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
Michael de Raadt added a comment -

Thanks for working on this.

Show
Michael de Raadt added a comment - Thanks for working on this.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Rossiani Wijaya added a comment -

This is working fine.

Test passed.

Show
Rossiani Wijaya added a comment - This is working fine. Test passed.
Hide
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: