Moodle
  1. Moodle
  2. MDL-32325

Glossary entry printing is not XHTML compliant and does not permit theming

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.5, 2.2.2
    • Fix Version/s: 2.1.7, 2.2.4
    • Component/s: Glossary, Themes
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a new glossary with 'Entry list' as display format
      2. Add a few entries to the glossary
      3. Go back to the main page of the glossary
      4. Click on the print icon on the top right
      5. Check that: the concepts and definitions appear nicely on the page
      6. Check that: the divs for each glossary entry have distinct classes to allow easy theming. The divs inside the glossary entry must have a distinct class as well.
      7. Apply steps 3 to 6 to a few other themes.

      Show
      1. Create a new glossary with 'Entry list' as display format 2. Add a few entries to the glossary 3. Go back to the main page of the glossary 4. Click on the print icon on the top right 5. Check that: the concepts and definitions appear nicely on the page 6. Check that: the divs for each glossary entry have distinct classes to allow easy theming. The divs inside the glossary entry must have a distinct class as well. 7. Apply steps 3 to 6 to a few other themes.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32325-master
    • Rank:
      39144

      Description

      The output of an entry when you click the print icon on a glossary is...

      <b><h3 class="nolink"><span class="nolink">Concept</span></h3>:</b> <div class="no-overflow"><span class="nolink"></span><p>Description a dgasd gdasd gsag</p></div>
      

      The <b> tag should not be used and the concept title (using <h3> and <span> should be a div with a class that is set in the base theme. Also the description <div> should have a class that is set in the base theme.

      Also, the colon (:) is not needed and make the output look silly. It should be removed.

        Issue Links

          Activity

          Hide
          Saswat Padhi added a comment -

          I see the following on my MOODLE_22_STABLE clone :

          <div class="concept"><h3 class="nolink"><span class="nolink">Concept</span></h3></div> <div class="no-overflow"><p>Description</p></div>
          
          Show
          Saswat Padhi added a comment - I see the following on my MOODLE_22_STABLE clone : <div class="concept"><h3 class="nolink"><span class="nolink">Concept</span></h3></div> <div class="no-overflow"><p>Description</p></div>
          Hide
          Michael de Raadt added a comment -

          I'd like to ask any potential GSoC students not to work on this issue. Please choose another issue.

          Michael d.

          Show
          Michael de Raadt added a comment - I'd like to ask any potential GSoC students not to work on this issue. Please choose another issue. Michael d.
          Hide
          Frédéric Massart added a comment -

          I had a look at the issue, and this is the patch that I would submit:
          https://github.com/FMCorz/moodle/compare/MOODLE_22_STABLE...MDL-32325-22

          I also discovered that the "nolink" tag does not work properly, it does not prevent links from being made. format_text() does not like when an inline tag (<span class="nolink">) encapsulates a block tag (<h3>). We should use the <nolink> tag instead. This applies to most (if not all) the glossary types, I'd create another issue for this.

          Regarding theming, I would also remove the <table> or, at least, use html_writer, what do you think?

          Show
          Frédéric Massart added a comment - I had a look at the issue, and this is the patch that I would submit: https://github.com/FMCorz/moodle/compare/MOODLE_22_STABLE...MDL-32325-22 I also discovered that the "nolink" tag does not work properly, it does not prevent links from being made. format_text() does not like when an inline tag (<span class="nolink">) encapsulates a block tag (<h3>). We should use the <nolink> tag instead. This applies to most (if not all) the glossary types, I'd create another issue for this. Regarding theming, I would also remove the <table> or, at least, use html_writer, what do you think?
          Hide
          Frédéric Massart added a comment -

          Please someone review my code.
          (Next time I will work on master instead of 2.2)

          Show
          Frédéric Massart added a comment - Please someone review my code. (Next time I will work on master instead of 2.2)
          Hide
          Andrew Davis added a comment -

          Code changes looks fine.

          "6. Check that: the CSS allow easy theming"

          You'll need to be more specific about what you mean ie view the page source and check that the divs in a particular area all have classes or something like that.

          Probably also include viewing the glossary in a handful of our included themes just to make sure there are no issues there.

          Show
          Andrew Davis added a comment - Code changes looks fine. "6. Check that: the CSS allow easy theming" You'll need to be more specific about what you mean ie view the page source and check that the divs in a particular area all have classes or something like that. Probably also include viewing the glossary in a handful of our included themes just to make sure there are no issues there.
          Hide
          Andrew Davis added a comment -

          Also, probably squish those 3 commits into one.

          And I'm not sure what the integrators policy is but you'll probably want to create a branch off master, cherry pick that commit into the master branch then add a master branch and diff URL to this issue.

          Show
          Andrew Davis added a comment - Also, probably squish those 3 commits into one. And I'm not sure what the integrators policy is but you'll probably want to create a branch off master, cherry pick that commit into the master branch then add a master branch and diff URL to this issue.
          Hide
          Frédéric Massart added a comment -

          Thanks for your feedback Andrew.
          I have updated my branches and the testing instructions.

          Show
          Frédéric Massart added a comment - Thanks for your feedback Andrew. I have updated my branches and the testing instructions.
          Hide
          Andrew Davis added a comment -

          Submit this for integration whenever you're ready

          Show
          Andrew Davis added a comment - Submit this for integration whenever you're ready
          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
          Frédéric Massart added a comment -

          Rebased! Cheers!

          Show
          Frédéric Massart added a comment - Rebased! Cheers!
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          While I like what you've done here I don't think this should should be integrated quite yet.
          The problem I have with it is in regards to the conversion from table => div for entrylist. I see that the other formats are still using tables, and I also see that there are themes that has CSS for those tables.
          So while I am 110% for conversion from tables to divs I think that we need to take real care in this case to ensure that things continue to be consistent and avoid obvious regressions in themes that would be caused by the inconsistency.
          I think the solution would be to solve the issue at hand here (the invalid XHTML) and then create a separate issue that involves converting the display of entries from tables to divs. The latter would of course be a master only change, where as the XHTML would be all stable branches.

          I will leave this as integration review in progress. If you agree we can reopen it, otherwise lets discuss it here.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, While I like what you've done here I don't think this should should be integrated quite yet. The problem I have with it is in regards to the conversion from table => div for entrylist. I see that the other formats are still using tables, and I also see that there are themes that has CSS for those tables. So while I am 110% for conversion from tables to divs I think that we need to take real care in this case to ensure that things continue to be consistent and avoid obvious regressions in themes that would be caused by the inconsistency. I think the solution would be to solve the issue at hand here (the invalid XHTML) and then create a separate issue that involves converting the display of entries from tables to divs. The latter would of course be a master only change, where as the XHTML would be all stable branches. I will leave this as integration review in progress. If you agree we can reopen it, otherwise lets discuss it here. Cheers Sam
          Hide
          Frédéric Massart added a comment - - edited

          Hi Sam,

          I agree with you. I will update my branches to fix the XHTML issues. I guess we are not considering 'master' as a target for a 'master change' just yet.

          Edit:
          I have updated the master branch with an idea. Instead of getting rid of the divs, I have included them within the origin table. The table itself only contained one cell which is not really helping for theming. What do you think? If you're ok with it, I'll update the 2.2 branch (and what about 2.1? Should have submitted a patch too right?)

          Fred

          Show
          Frédéric Massart added a comment - - edited Hi Sam, I agree with you. I will update my branches to fix the XHTML issues. I guess we are not considering 'master' as a target for a 'master change' just yet. Edit: I have updated the master branch with an idea. Instead of getting rid of the divs, I have included them within the origin table. The table itself only contained one cell which is not really helping for theming. What do you think? If you're ok with it, I'll update the 2.2 branch (and what about 2.1? Should have submitted a patch too right?) Fred
          Hide
          Sam Hemelryk added a comment -

          Hi Fred,

          In regards to master only changes, no they are not being integrated unless essential, they would have to wait till after the release now.
          However I've looked at the changes in your master branch now and I think they are spot great, and certainly fine to integrate.

          If you could please backport those changes changes to 22 and 21 is applicable and comment when done so I can integrate.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Fred, In regards to master only changes, no they are not being integrated unless essential, they would have to wait till after the release now. However I've looked at the changes in your master branch now and I think they are spot great, and certainly fine to integrate. If you could please backport those changes changes to 22 and 21 is applicable and comment when done so I can integrate. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          There are the updated branches!

          Show
          Frédéric Massart added a comment - There are the updated branches!
          Hide
          Sam Hemelryk added a comment -

          Thanks Fred, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Fred, this has been integrated now
          Hide
          Sam Hemelryk added a comment -

          Tested during integration review and passed.

          Show
          Sam Hemelryk added a comment - Tested during integration review and passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

          Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

          Many thanks for your collaboration!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3. Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish. Many thanks for your collaboration! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: