Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-58760

{{#pix}} does not work if the icon key has been escaped

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.3
    • Fix Version/s: 3.3
    • Component/s: JavaScript, Themes
    • Labels:

      Description

      Steps to reproduce

      1. Login as an admin/manager
      2. Go to Site administration > Competencies > Competency frameworks
      3. Create a framework with a competency
      4. Go to your profile and follow 'Learning plans'
      5. Add a new learning plan named 'My plan'.
      6. Note the "t/collapsed" (in Clean) or the "fa-plus-square" (Boost) icon next to the "Comments" link.
      7. Add a competency to the plan.

      Expected: When the competency picker is closed, the icon for expanding the comments area should remain.

      Actual: The icon is replaced with a broken image.

      Note: The icon currently does not change when comments are expanded/collapsed. It is a static icon.

      Reason

      In MDL-40759 (commit 663640f5b), the pix_url has been converted to pix_icon. Mustache templates were changed to use the {{#pix}} helper. This works well as long as the icon key is hard coded in the template. But in some cases, the icon key is being populated dynamically from the template context variable, as in:

      {{#pix}}{{collapsediconkey}}, {{linktext}}{{/pix}}
      

      In such cases, there comes a problem with the way how HTML escaping is implemented in our Mustache.

      • When the template is processed by the PHP Mustache engine, we use the Moodle's function s() as a custom escaping function. If the icon key has a value like t/collapse it is not changed.
      • But when the template is rendered by the JS Mustache engine, there is a different escaping logic used (see escapeHtml() in lib/amd/src/mustache.js) and forward slashes are replaced with the HTML entity / so the icon key becomes t/collapse and is not rendered correctly.

      Escaping of forward slashes is a known surprise to many Mustache users and the common recommendation on the web is to disable escaping via triple curly brackets.

      There seem to be multiple alternative (not exclusively) approaches to this:

      1. Harmonize the escaping logic we do in PHP and JS mustache engines so they both produce same result in all cases (ideal).
      2. Rewrite all templates that use dynamic value of the icon key to not escape it (ugly, insecure, unmaintainable, prone to same mistakes in the future).
      3. Fix the {{#pix}} helper in the JS mustache engine so that it undoes the escaping for this particular case only (pragmatic).

      For now given the circumstances, I am tempted to follow the third approach.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    15/May/17