Moodle
  1. Moodle
  2. MDL-33569

Labels create extra margin below content

    Details

    • Testing Instructions:
      Hide

      Note to tester: The requires activities or resources in either the front page site topic or in a Course page.

      1. Select Standard theme
      2. Enable Editing in either the front page site topic or in a course.
      3. Next add two labels above a list of pre existing activities/resources. The first label should contain only a short title, and the second two or three short paragraphs.
      4. If not already added add another activity below the last label.
      5. Turn editing off and TEST that the first label sits directly above the second label with a small gap between the two.
      6. Test that the last label sits above the list of activities with a gap between equal to the gap between the individual activities.
      7. Change to Clean theme and TEST that the labels look the same again.
      Show
      Note to tester: The requires activities or resources in either the front page site topic or in a Course page. Select Standard theme Enable Editing in either the front page site topic or in a course. Next add two labels above a list of pre existing activities/resources. The first label should contain only a short title, and the second two or three short paragraphs. If not already added add another activity below the last label. Turn editing off and TEST that the first label sits directly above the second label with a small gap between the two. Test that the last label sits above the list of activities with a gap between equal to the gap between the individual activities. Change to Clean theme and TEST that the labels look the same again.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE

      Description

      Labels force a paragraph wrapper around content which produces a carriage return that may be undesirable and cannot be removed. Deleting these tags when in HTML mode doesn't help; the tags return.

      • Create a new label resource with a single line of text.
      • Create a second label in the same section as the first.
      • Notice the extra space between the first a second labels.

      This can be confirmed another way when editing the label content in an HTML source editor. "<p></p>" wrap around any content that is saved in the WYSIWYG editor.

        Gliffy Diagrams

          Activity

          Hide
          Mary Evans added a comment -

          I think you will find that this happens whenever you add a LABEL in whatever theme you happen to be using, and so not just restricted to Afterburner. The paragraph tag is automatic in a LABEL and nothing to do with the theme itself. To make matters worse, the paragraph <p></p> tag is styled by YUI CSS which again is automated and nothing to do with the theme, neither is it a carrage return, it is a 1em margin applied by YUI CSS to the bottom edge of the LABEL.

          The problem is in the way the LABEL is generated.

          Show
          Mary Evans added a comment - I think you will find that this happens whenever you add a LABEL in whatever theme you happen to be using, and so not just restricted to Afterburner. The paragraph tag is automatic in a LABEL and nothing to do with the theme itself. To make matters worse, the paragraph <p></p> tag is styled by YUI CSS which again is automated and nothing to do with the theme, neither is it a carrage return, it is a 1em margin applied by YUI CSS to the bottom edge of the LABEL. The problem is in the way the LABEL is generated.
          Hide
          Michael de Raadt added a comment -

          Perhaps it's possible to change the YUI CSS.

          Show
          Michael de Raadt added a comment - Perhaps it's possible to change the YUI CSS.
          Hide
          Mary Evans added a comment - - edited

          And risk changing all of the paragraph formatting for Moodle? I don't think so!
          unless we go for

          p { margin: 0; line-height: normal;}

          that would work better. In which case it could go into BASE theme and override YUI.

          Another alternative, and I think this would be better, would be to add a class selector in the label when it is generated, so it can be styled in a theme's stylesheet. So the tag for label would look something like...

          <div class="label"><p> </p></div>

          Then the CSS would be something like:

          .label p {margin: 0; line-height: normal;}

          So this would only affect the label and nothing else.

          Show
          Mary Evans added a comment - - edited And risk changing all of the paragraph formatting for Moodle? I don't think so! unless we go for p { margin: 0; line-height: normal;} that would work better. In which case it could go into BASE theme and override YUI. Another alternative, and I think this would be better, would be to add a class selector in the label when it is generated, so it can be styled in a theme's stylesheet. So the tag for label would look something like... <div class="label"><p> </p></div> Then the CSS would be something like: .label p {margin: 0; line-height: normal;} So this would only affect the label and nothing else.
          Hide
          Eric Strom added a comment -

          Modifying the label class seems like a good choice. I would add that this adjustment, however it is applied, should also be done for the section summary class which also has the extra margin below. This would give better layout control for sections to the end user.

          Show
          Eric Strom added a comment - Modifying the label class seems like a good choice. I would add that this adjustment, however it is applied, should also be done for the section summary class which also has the extra margin below. This would give better layout control for sections to the end user.
          Hide
          Mary Evans added a comment - - edited

          Good point Eric. I'll take a look at this later, and see if I can fathom it out and create a patch to fix this, it should be pretty straight forward.

          Show
          Mary Evans added a comment - - edited Good point Eric. I'll take a look at this later, and see if I can fathom it out and create a patch to fix this, it should be pretty straight forward.
          Hide
          Eric Strom added a comment -

          Thanks, Mary, for digging into this! So this has prompted me to investigate if there are other treatments of this kind in the rest of the resource/activity options that appear on the front page of a course and it appears there are.

          When enabling the option to display descriptions of resource items, it is apparent that the margin is built-in to these descriptions also. Although my instinct still tells me the margin should not be forced and control should be in the content builder's hands, this is beginning to be a bit larger that originally anticipated. I would prefer to hear others' opinions on this. Is there a "description" class that would cover these other components? I am aware of some activities (like quiz) that uses the language "instructions" instead of "description". Could be worth looking into.

          Show
          Eric Strom added a comment - Thanks, Mary, for digging into this! So this has prompted me to investigate if there are other treatments of this kind in the rest of the resource/activity options that appear on the front page of a course and it appears there are. When enabling the option to display descriptions of resource items, it is apparent that the margin is built-in to these descriptions also. Although my instinct still tells me the margin should not be forced and control should be in the content builder's hands, this is beginning to be a bit larger that originally anticipated. I would prefer to hear others' opinions on this. Is there a "description" class that would cover these other components? I am aware of some activities (like quiz) that uses the language "instructions" instead of "description". Could be worth looking into.
          Hide
          Eric Strom added a comment -

          Ok, so I think came across a wrinkle that may be helpful or another issue or both :7).

          It appears that the News Forum activity which is populated in a course shell (from core I suppose) contains a description that is using moodle auto-formatting instead of HTML format like other resources or activities that are manually created.

          As a result, the forum introduction text when displayed below the activity doesn't have the <p></p> tags. Upon element inspection I see:

          <div class="contentafterlink"><div class="no-overflow"><div class="no-overflow">General news and announcements</div></div></div></div>

          Changing the format of this content to HTML format and saving the forum again creates the <p> tags, and also removes the option to change the formatting back to moodle auto-format or any other formatting choice.

          Show
          Eric Strom added a comment - Ok, so I think came across a wrinkle that may be helpful or another issue or both :7). It appears that the News Forum activity which is populated in a course shell (from core I suppose) contains a description that is using moodle auto-formatting instead of HTML format like other resources or activities that are manually created. As a result, the forum introduction text when displayed below the activity doesn't have the <p></p> tags. Upon element inspection I see: <div class="contentafterlink"><div class="no-overflow"><div class="no-overflow">General news and announcements</div></div></div></div> Changing the format of this content to HTML format and saving the forum again creates the <p> tags, and also removes the option to change the formatting back to moodle auto-format or any other formatting choice.
          Hide
          Michael de Raadt added a comment -

          Thanks for looking at this, guys. It looks like this is progressing along.

          Show
          Michael de Raadt added a comment - Thanks for looking at this, guys. It looks like this is progressing along.
          Hide
          Caroline Moore added a comment -

          Has there been any further progress on this issue since June? I'd really love to resolve it; it's driving my instructors crazy.

          Show
          Caroline Moore added a comment - Has there been any further progress on this issue since June? I'd really love to resolve it; it's driving my instructors crazy.
          Hide
          Eric Strom added a comment -

          Pinging. Is there enough clarity on this issue to more forward on the CSS?

          Show
          Eric Strom added a comment - Pinging. Is there enough clarity on this issue to more forward on the CSS?
          Hide
          Mary Evans added a comment -

          OK...you have my +1 for this and a long time in getting fixed for which I apologise. I'll do it now and then we can all get some sleep!

          Show
          Mary Evans added a comment - OK...you have my +1 for this and a long time in getting fixed for which I apologise. I'll do it now and then we can all get some sleep!
          Hide
          Mary Evans added a comment -

          Well this is proving harder than I would have thought. The label is an insignificant class selector trapped in a myriad of containers made up of classless divs and no-overflow, all with different dimensions, paddings and margins.

          Show
          Mary Evans added a comment - Well this is proving harder than I would have thought. The label is an insignificant class selector trapped in a myriad of containers made up of classless divs and no-overflow, all with different dimensions, paddings and margins.
          Hide
          Eric Strom added a comment -

          Thanks for investigating this. Is there any hope to remedy or start rebuilding this in future versions?

          Show
          Eric Strom added a comment - Thanks for investigating this. Is there any hope to remedy or start rebuilding this in future versions?
          Hide
          Eric Strom added a comment -

          Ping. Continue to get requests from faculty building their courses wondering what they are doing wrong that they cannot remove the extra carriage return produced in labels.

          Show
          Eric Strom added a comment - Ping. Continue to get requests from faculty building their courses wondering what they are doing wrong that they cannot remove the extra carriage return produced in labels.
          Hide
          Mary Evans added a comment -

          Hi Eric, which theme are you using?
          If it has a Custom CSS settings page like Afterburner that you can add a quick fix to by adding...

          .label .no-overflow p {margin: 0;}
          

          Or alternatively add ...

          <style type="text/css">
          .label .no-overflow p {margin: 0;}
          </style>
          

          to the HEAD section inside the Additional HTML page found in Site Administration > Appearance > Additional HTML (its the last in the list)

          That should fix it site wide.

          In the mean time I'll be fixing this this week.
          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi Eric, which theme are you using? If it has a Custom CSS settings page like Afterburner that you can add a quick fix to by adding... .label .no-overflow p {margin: 0;} Or alternatively add ... <style type="text/css"> .label .no-overflow p {margin: 0;} </style> to the HEAD section inside the Additional HTML page found in Site Administration > Appearance > Additional HTML (its the last in the list) That should fix it site wide. In the mean time I'll be fixing this this week. Cheers Mary
          Hide
          Mary Evans added a comment - - edited

          Adding David Scotson as a watcher.
          David can you suggest a better way to fix the paragraph CSS that makes the mod_type_label spaced as it is at present?

          Considering the problems with the label class and Bootstrap in MDL-38086, it may make better sense to fix this once and for all for this particular label. What do you think?

          Show
          Mary Evans added a comment - - edited Adding David Scotson as a watcher. David can you suggest a better way to fix the paragraph CSS that makes the mod_type_label spaced as it is at present? Considering the problems with the label class and Bootstrap in MDL-38086 , it may make better sense to fix this once and for all for this particular label. What do you think?
          Hide
          David Scotson added a comment -

          Hi Mary Evans, I just noticed the email about this and, coincidentally, one of the staff at my institution complained about this too.

          Isn't the paragraph tag added by TinyMCE? (as Eric mentions above) I'm thinking that if the label has multiple paragraphs then you only want to remove the padding from the last one.

          So what about:

          .modtype_label .no-overflow > p:last-child

          {margin-bottom: 0;}
          Show
          David Scotson added a comment - Hi Mary Evans , I just noticed the email about this and, coincidentally, one of the staff at my institution complained about this too. Isn't the paragraph tag added by TinyMCE? (as Eric mentions above) I'm thinking that if the label has multiple paragraphs then you only want to remove the padding from the last one. So what about: .modtype_label .no-overflow > p:last-child {margin-bottom: 0;}
          Hide
          Mary Evans added a comment -

          Thanks David,

          I seem to have missed this in my emails, but I did write a postit-note to myself which is stuck on my screen here to get this fixed today! Glad you thought this need a pseudo class last-child as this was my first thought because of the different paragraphs that a label can have added.

          Personally I think the whole set up for the label is wrong. Have you ever noticed when hovering over a label, even when it only contains one word or a short phrase, one of the div containers makes it twice the size it needs to be?

          Fixing this now.

          Thanks for clarification in feedback.

          Mary

          Show
          Mary Evans added a comment - Thanks David, I seem to have missed this in my emails, but I did write a postit-note to myself which is stuck on my screen here to get this fixed today! Glad you thought this need a pseudo class last-child as this was my first thought because of the different paragraphs that a label can have added. Personally I think the whole set up for the label is wrong. Have you ever noticed when hovering over a label, even when it only contains one word or a short phrase, one of the div containers makes it twice the size it needs to be? Fixing this now. Thanks for clarification in feedback. Mary
          Hide
          Mary Evans added a comment - - edited

          Adding Andrew Nicols as a watcher to ask his advice about an empty div container which has a class="activityinstance" and found in course section and site topic section that adds at least 20px to the top of a label and other activity elements on the page. I'm trying to find out why it is there and wonder if it actually needs to be there, and if it does, say for AJAX or JS purposes as a marker, then can it not be hidden?

          Show
          Mary Evans added a comment - - edited Adding Andrew Nicols as a watcher to ask his advice about an empty div container which has a class="activityinstance" and found in course section and site topic section that adds at least 20px to the top of a label and other activity elements on the page. I'm trying to find out why it is there and wonder if it actually needs to be there, and if it does, say for AJAX or JS purposes as a marker, then can it not be hidden?
          Hide
          Mary Evans added a comment - - edited

          More observations with respect to the label.

          This is what I see when viewing only ONE label in the Site Topic of the front page.
          Is it actually necessary to have such a plethora of div containers? Not to mention all those selector classes?
           

          <div class="box generalbox sitetopic">
          <ul class="section img-text">
          <li id="module-11" class="activity label modtype_label ">
          <div class="mod-indent">
          <div class="activityinstance"></div>
          <div class="">
          <div class="no-overflow">
          <div class="no-overflow">
          <p>Frontpage Label</p>
          </div>
          </div>
          </div>
          </div>
          </li>
          </ul>
          </div>
          

          Show
          Mary Evans added a comment - - edited More observations with respect to the label. This is what I see when viewing only ONE label in the Site Topic of the front page. Is it actually necessary to have such a plethora of div containers? Not to mention all those selector classes?   <div class="box generalbox sitetopic"> <ul class="section img-text"> <li id="module-11" class="activity label modtype_label "> <div class="mod-indent"> <div class="activityinstance"></div> <div class=""> <div class="no-overflow"> <div class="no-overflow"> <p>Frontpage Label</p> </div> </div> </div> </div> </li> </ul> </div>
          Hide
          Sam Hemelryk added a comment -

          Thanks Mary - this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks Mary - this has been integrated now.
          Hide
          Andrew Nicols added a comment -

          Thanks Mary,

          As I recall, this is present to make the JS simpler. It's primarily used by drag and drop content - that's:

          • moving existing activities to an empty section; and
          • dropping new files into Moodle.

          Without it, we have to work out whether there is such a section, and then add it if it's not there. Adding it can be quite expensive and has to be done at page load whenever editing is on.

          I do wonder whether we can simplify the structure of that - e.g. moving the modindent class to the same as the activityinstance. We may have to make some JS changes in order to do so.

          Andrew

          Show
          Andrew Nicols added a comment - Thanks Mary, As I recall, this is present to make the JS simpler. It's primarily used by drag and drop content - that's: moving existing activities to an empty section; and dropping new files into Moodle. Without it, we have to work out whether there is such a section, and then add it if it's not there. Adding it can be quite expensive and has to be done at page load whenever editing is on. I do wonder whether we can simplify the structure of that - e.g. moving the modindent class to the same as the activityinstance. We may have to make some JS changes in order to do so. Andrew
          Hide
          David Scotson added a comment -

          There's discussion about this on the forum too: https://moodle.org/mod/forum/discuss.php?d=240360

          Personally I don't see the link to JS, all the extra stuff there is just label related. To see what the JS requires you'd have to look at what's there when there's nothing present, not even a label.

          Show
          David Scotson added a comment - There's discussion about this on the forum too: https://moodle.org/mod/forum/discuss.php?d=240360 Personally I don't see the link to JS, all the extra stuff there is just label related. To see what the JS requires you'd have to look at what's there when there's nothing present, not even a label.
          Hide
          Mary Evans added a comment -

          Thanks Andrew,

          What I don't like about 'activityinstance' class is the fact it is being styled. If it's primary function is to be used as a target to aid in JS/D&D should it not be named such...something like class="dnd-activity-instance" then we would all know its a dnd class and leave well alone!

          Show
          Mary Evans added a comment - Thanks Andrew, What I don't like about 'activityinstance' class is the fact it is being styled. If it's primary function is to be used as a target to aid in JS/D&D should it not be named such...something like class="dnd-activity-instance" then we would all know its a dnd class and leave well alone!
          Hide
          Mary Evans added a comment - - edited

          I think I have just realised what's happened.
          Recently in MDL-(need to find it) the edit icons were placed inline and then the div container class="activityinstance" was given padding to move them away from the activity. So if we shift the padding to...
           

          .activityinstance a {padding-right: 3em}

          This then makes the empty div in the label vanish

          Show
          Mary Evans added a comment - - edited I think I have just realised what's happened. Recently in MDL-(need to find it) the edit icons were placed inline and then the div container class="activityinstance" was given padding to move them away from the activity. So if we shift the padding to...   .activityinstance a {padding-right: 3em} This then makes the empty div in the label vanish
          Hide
          Andrew Nicols added a comment -

          Ah - if we're just talking about labels, I don't think that this has anything to do with JS at all. At least, not what I originally suggested. I misunderstood your original point.

          I think that this has always been the case hasn't it?

          Show
          Andrew Nicols added a comment - Ah - if we're just talking about labels, I don't think that this has anything to do with JS at all. At least, not what I originally suggested. I misunderstood your original point. I think that this has always been the case hasn't it?
          Hide
          Mary Evans added a comment -

          @Andrew,
          Whether or not 'sctivityinstance' is used in JS/D&D is of no consequence, what was the problem is that the padding it contained made the otherwise empty div visible.
          I've just pushed a fix for MDL-42061 to my github and set it for Integration Review.

          @Sam,
          Is there any chance of MDL-42061 getting integrated too this week as it relates to this problem?

          Thanks
          Mary

          Show
          Mary Evans added a comment - @Andrew, Whether or not 'sctivityinstance' is used in JS/D&D is of no consequence, what was the problem is that the padding it contained made the otherwise empty div visible. I've just pushed a fix for MDL-42061 to my github and set it for Integration Review. @Sam, Is there any chance of MDL-42061 getting integrated too this week as it relates to this problem? Thanks Mary
          Hide
          Andrew Nicols added a comment -

          Hi Mary,

          I'm not sure whether this is working as described so I'm going to fail the issue. Attaching some screenshots to demonstrate.

          Show
          Andrew Nicols added a comment - Hi Mary, I'm not sure whether this is working as described so I'm going to fail the issue. Attaching some screenshots to demonstrate.
          Hide
          Mary Evans added a comment - - edited

          That's the effect of the bug which MDL-42061 fixes, that's is why I asked if that can get integrated at same time? If you passed this as it stands now MDL-42061 will fix that wide gap.
          Also I wonder if there is actually something wrong when the label is created...perhaps that is another bug altogether? Can you check that Andrew?

          I really feel I have unearthed a nest of worms!

          Thanks
          Mary

          Show
          Mary Evans added a comment - - edited That's the effect of the bug which MDL-42061 fixes, that's is why I asked if that can get integrated at same time? If you passed this as it stands now MDL-42061 will fix that wide gap. Also I wonder if there is actually something wrong when the label is created...perhaps that is another bug altogether? Can you check that Andrew? I really feel I have unearthed a nest of worms! Thanks Mary
          Hide
          Sam Hemelryk added a comment -

          Ok I've been looking into this issue this morning.

          It definitely appears to be a regression from MDL-36449 which while improving the display also styled the .activityinstance element despite it being a bad choice to style (because for mods without a title such as label it gets produced empty).
          Having taken before + after shots and really playing around with this I don't think the solution you've got here Mary is necessarily the best solution to this problem.
          It appears to me that the issue stems from the activityinstance having padding when it is empty and I wonder whether a better solution would be to add an additional class to that element if its empty to signify it is empty and then style that element to have padding:0.
          That of course is part core change and part themes, we'd also need to amend the dnd code to add that class as required as well (should be simple though).

          As this is very tided to MDL-42061 and because I suspect there may be a better solution at the moment I feel that we would be best to revert this change and investigate those other solutions.
          Does that sound like an OK idea to you?

          Show
          Sam Hemelryk added a comment - Ok I've been looking into this issue this morning. It definitely appears to be a regression from MDL-36449 which while improving the display also styled the .activityinstance element despite it being a bad choice to style (because for mods without a title such as label it gets produced empty). Having taken before + after shots and really playing around with this I don't think the solution you've got here Mary is necessarily the best solution to this problem. It appears to me that the issue stems from the activityinstance having padding when it is empty and I wonder whether a better solution would be to add an additional class to that element if its empty to signify it is empty and then style that element to have padding:0. That of course is part core change and part themes, we'd also need to amend the dnd code to add that class as required as well (should be simple though). As this is very tided to MDL-42061 and because I suspect there may be a better solution at the moment I feel that we would be best to revert this change and investigate those other solutions. Does that sound like an OK idea to you?
          Hide
          Mary Evans added a comment -

          Yes indeed it does sound like a good idea.
          I have been looking at the renderer that outputs the code for the Label and frankly found it needing some TLC, as it appears to assume too much and ends up adding div tags willy nilly all over the place, which not only looks bad, it makes it a hell of a thing to style.

          Show
          Mary Evans added a comment - Yes indeed it does sound like a good idea. I have been looking at the renderer that outputs the code for the Label and frankly found it needing some TLC, as it appears to assume too much and ends up adding div tags willy nilly all over the place, which not only looks bad, it makes it a hell of a thing to style.
          Hide
          Mary Evans added a comment -

          OK I'll rework MDL-42061

          Show
          Mary Evans added a comment - OK I'll rework MDL-42061
          Hide
          Sam Hemelryk added a comment -

          Alrighty - this has been reverted now - sorry about the kick up Mary, hopefully we can address this in core as well as in themes and try to detangle the mess a little!

          Show
          Sam Hemelryk added a comment - Alrighty - this has been reverted now - sorry about the kick up Mary, hopefully we can address this in core as well as in themes and try to detangle the mess a little!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: