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

      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

          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: