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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              saswatpadhi 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
              saswatpadhi 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
              salvetore 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
              salvetore 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
              fred 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
              fred 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
              fred Frédéric Massart added a comment -

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

              Show
              fred Frédéric Massart added a comment - Please someone review my code. (Next time I will work on master instead of 2.2)
              Hide
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              andyjdavis 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
              fred Frédéric Massart added a comment -

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

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

              Submit this for integration whenever you're ready

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

              Rebased! Cheers!

              Show
              fred Frédéric Massart added a comment - Rebased! Cheers!
              Hide
              samhemelryk 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
              samhemelryk 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
              fred 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
              fred 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
              samhemelryk 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
              samhemelryk 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
              fred Frédéric Massart added a comment -

              There are the updated branches!

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

              Thanks Fred, this has been integrated now

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

              Tested during integration review and passed.

              Show
              samhemelryk Sam Hemelryk added a comment - Tested during integration review and passed.
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    9/Jul/12