Moodle
  1. Moodle
  2. MDL-27989

mform groups not belonging to a fieldset display badly in core themes whose parent is Canvas

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.3
    • Fix Version/s: 2.0.4, 2.1.1
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      this issue will be fixed if browsing the attached zip file, each mform element will be well drawn. I added few mform elements to the first release if test.zip in order to evaluate label vertical alignment too.

      Show
      this issue will be fixed if browsing the attached zip file, each mform element will be well drawn. I added few mform elements to the first release if test.zip in order to evaluate label vertical alignment too.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-27989_master
    • Rank:
      17669

      Description

      by adding a group to a moodleform with $mform->addGroup
      if the group is not contained in a fieldset, its position in the form is strongly on the right in formal_white.

      I am sorry, but I noticed this issue by coding!
      I did not find any other instance in moodle core of this situation so... the only way to test this bug is to add to a moodleform BEFORE an 'header 'a code like:

      $option = array('a label');
                  
      $elementgroup = array();
      $elementgroup[] =& $mform->createElement('select', 'aaa', '', $option);
      $elementgroup[] =& $mform->createElement('text', 'bbb', '');
      $mform->addGroup($elementgroup, 'group', 'one more label', ' ', false);
      

      and call the modified form from moodle.

      1. group_without_fieldset_formalwhite.png
        44 kB
      2. group_without_fieldset_fusion.png
        44 kB

        Activity

        Hide
        Daniele Cordella added a comment -

        Eloy, I agree. My working process is funny but finally I reached my first correct PULL REQUEST. Thanks a lot! (hug)

        Show
        Daniele Cordella added a comment - Eloy, I agree. My working process is funny but finally I reached my first correct PULL REQUEST. Thanks a lot! (hug)
        Hide
        Eloy Lafuente (stronk7) 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.

        Show
        Eloy Lafuente (stronk7) 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.
        Hide
        Daniele Cordella added a comment -

        Eloy, I did it. As far as I know all is 100% correct now. Thank you!

        Show
        Daniele Cordella added a comment - Eloy, I did it. As far as I know all is 100% correct now. Thank you!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ho Daniele,

        I went to test this before applying your changes and detected that fusion also had the same problem, while standard didn't. So I'm just guessing if this problem is something to be be fixed in the "canvas" theme a not in formal_white.

        So, I'm reopening this and adding some people to take a look on it.

        Finally note I'm not sure if it's "correct" to display fields / groups out from fieldsets but, in any case, should be supported, yes.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Ho Daniele, I went to test this before applying your changes and detected that fusion also had the same problem, while standard didn't. So I'm just guessing if this problem is something to be be fixed in the "canvas" theme a not in formal_white. So, I'm reopening this and adding some people to take a look on it. Finally note I'm not sure if it's "correct" to display fields / groups out from fieldsets but, in any case, should be supported, yes. Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        reopening as commented

        Show
        Eloy Lafuente (stronk7) added a comment - reopening as commented
        Hide
        Eloy Lafuente (stronk7) added a comment - - edited

        Added John and Mary as canvas experts and 2 screenshots (fusion and format_white) showing the group without fieldset incorrectly.

        For your consideration, ciao

        To reproduce: Just add the mforms code suggested by Danielle at the very beginning of any form definition.

        Show
        Eloy Lafuente (stronk7) added a comment - - edited Added John and Mary as canvas experts and 2 screenshots (fusion and format_white) showing the group without fieldset incorrectly. For your consideration, ciao To reproduce: Just add the mforms code suggested by Danielle at the very beginning of any form definition.
        Hide
        Mary Evans added a comment -

        Hi Eloy,

        Not having altered an mform before, it would have been good to see a correct view, as well as an incorrect view. What exactly am I looking for?

        I can see the fix in Daniele's Git Moodle repository, but still struggle to see which bits are being styled.

        The css in core.css in Canvas theme is the same as in Formal White's core css prior to the fix. Fusion takes it's css from Canvas, but so do other CORE themes, so if there is a change to be made, then it is better we do it in Fusion and not Canvas. However, if, an only IF ALL core themes are affected, then it is simpler to do the change in Canvas theme.

        Thanks for bringing this to my attention. I'll go and see if I can reproduce this.

        Thanks and ciao!

        Show
        Mary Evans added a comment - Hi Eloy, Not having altered an mform before, it would have been good to see a correct view, as well as an incorrect view. What exactly am I looking for? I can see the fix in Daniele's Git Moodle repository, but still struggle to see which bits are being styled. The css in core.css in Canvas theme is the same as in Formal White's core css prior to the fix. Fusion takes it's css from Canvas, but so do other CORE themes, so if there is a change to be made, then it is better we do it in Fusion and not Canvas. However, if, an only IF ALL core themes are affected, then it is simpler to do the change in Canvas theme. Thanks for bringing this to my attention. I'll go and see if I can reproduce this. Thanks and ciao!
        Hide
        Daniele Cordella added a comment -

        Attached is a test page to let this issue evident.
        Decompress the zip file and move the two resulting files to your moodle20 folder. (in the same folder of the config.php)
        Open your browser and go to your moodle20 front page.
        If your moodle20 URL ends in index.php, change "index.php" with "test.php"
        If your moodle20 URL ends in <yourmoodlefoldername>/, add to the end of your URL "test.php"
        Enjoy it.

        Show
        Daniele Cordella added a comment - Attached is a test page to let this issue evident. Decompress the zip file and move the two resulting files to your moodle20 folder. (in the same folder of the config.php) Open your browser and go to your moodle20 front page. If your moodle20 URL ends in index.php, change "index.php" with "test.php" If your moodle20 URL ends in <yourmoodlefoldername>/, add to the end of your URL "test.php" Enjoy it.
        Hide
        Daniele Cordella added a comment - - edited

        My proposal, in order to fix this issue in canvas theme, is to change the class style in canvas/style/core.css line 577:
        from

        .mform .hidden .fitem .fgroup {
            width: 100%;
            text-align: center;
            margin: 1em 0;
        }
        

        to

        .mform .hidden .fitem .fgroup {
            width: 80%;
        }
        
        Show
        Daniele Cordella added a comment - - edited My proposal, in order to fix this issue in canvas theme, is to change the class style in canvas/style/core.css line 577: from .mform .hidden .fitem .fgroup { width: 100%; text-align: center; margin: 1em 0; } to .mform .hidden .fitem .fgroup { width: 80%; }
        Hide
        Daniele Cordella added a comment -

        To Eloy.
        Even if the correction will be applied at canvas level, FW theme needs modifications too.
        This is because FW doesn't split the mform in .fitemtitle: 20% vs .felement: 80% but .fitemtitle: 25% vs .felement: 75%.
        So, in any case, please integrate the original push of this MDL to FW too.
        Thanks in adavnce.

        Show
        Daniele Cordella added a comment - To Eloy. Even if the correction will be applied at canvas level, FW theme needs modifications too. This is because FW doesn't split the mform in .fitemtitle: 20% vs .felement: 80% but .fitemtitle: 25% vs .felement: 75%. So, in any case, please integrate the original push of this MDL to FW too. Thanks in adavnce.
        Hide
        Mary Evans added a comment -

        Daniele,

        I managed to replicate this problem without the use of the zip, but I will test that later. However, the FIX you are putting forward is, in my humble opinion, not the best solution. Making width 80%, is not really fixing this as it will affect the layout of the form globally. If you look closely at the different elements, which make up this addition to the mform, and add border: 1px solid red; round them you will see that all that is needed is a text-align: left and perhaps margin-left: 10px.

        I'll go an see the test.php and check if it looks any different that the one I created.
        Mary

        Show
        Mary Evans added a comment - Daniele, I managed to replicate this problem without the use of the zip, but I will test that later. However, the FIX you are putting forward is, in my humble opinion, not the best solution. Making width 80%, is not really fixing this as it will affect the layout of the form globally. If you look closely at the different elements, which make up this addition to the mform, and add border: 1px solid red; round them you will see that all that is needed is a text-align: left and perhaps margin-left: 10px. I'll go an see the test.php and check if it looks any different that the one I created. Mary
        Hide
        Mary Evans added a comment - - edited

        Daniele:

        Please ignore my last comment.
        First of all thanks for adding the test.php that has helped in identifying the problem. In testing this 'phenomena' I did notice that in Formal White config.php you have parent themes written like so...

        $THEME->parents = array('base','canvas');

        This needs changing to so it reads...

        $THEME->parents = array('canvas','base');

        As for the form problem, I am still struggling to understand why, if you can do it correctly in Example 1: (How it should look), would anyone what to code this the wrong way? This just does not make sense. It's like asking to change css mark-up to make a mistake look better. Or am I missing something?

        Whatever the reason for doing this...and I am assuming there is a reason, then I think this patch is required in Base theme

        .mform .hidden .fitem .fgroup

        { display: inline; text-align: left; width: 80%; }

        And this removed from Formal White

        .mform .hidden .fitem .fgroup

        { width: 100%; text-align: center; margin: 1em 0; }

        Mary

        Show
        Mary Evans added a comment - - edited Daniele: Please ignore my last comment. First of all thanks for adding the test.php that has helped in identifying the problem. In testing this 'phenomena' I did notice that in Formal White config.php you have parent themes written like so... $THEME->parents = array('base','canvas'); This needs changing to so it reads... $THEME->parents = array('canvas','base'); As for the form problem, I am still struggling to understand why, if you can do it correctly in Example 1: (How it should look), would anyone what to code this the wrong way? This just does not make sense. It's like asking to change css mark-up to make a mistake look better. Or am I missing something? Whatever the reason for doing this...and I am assuming there is a reason, then I think this patch is required in Base theme .mform .hidden .fitem .fgroup { display: inline; text-align: left; width: 80%; } And this removed from Formal White .mform .hidden .fitem .fgroup { width: 100%; text-align: center; margin: 1em 0; } Mary
        Hide
        Daniele Cordella added a comment -

        Ciao Mary and thanks for your test and comment.
        > As for the form problem, I am still struggling to understand why, if you can do it correctly in Example 1: (How it should look), would anyone what to code this the wrong way?

        There is not a correct or wrong way to code it.
        If (the $elementgroup is in the frame of a fieldset)

        {it looks fine}

        otherwise

        {it looks bad}

        .

        Our purpose is to have it looking fine even if it is OUT FROM a fieldset. Nothing more.

        As you can see in my example the first $elementgroup was NOT drawn inside a frameset (no box with a title on the top left corner) and it was drawn badly.
        The second $elementgroup, with the same syntax BUT INSIDE a frameset, was correctly drawn.

        I apologise for my bloody English.

        About your suggestion: I agree with you, leaving valid what I addressed to Eloy this morning (30/Jun/11 2:29 PM) here in this MDL-issue.

        Thanks again, Mary.

        Show
        Daniele Cordella added a comment - Ciao Mary and thanks for your test and comment. > As for the form problem, I am still struggling to understand why, if you can do it correctly in Example 1: (How it should look), would anyone what to code this the wrong way? There is not a correct or wrong way to code it. If (the $elementgroup is in the frame of a fieldset) {it looks fine} otherwise {it looks bad} . Our purpose is to have it looking fine even if it is OUT FROM a fieldset. Nothing more. As you can see in my example the first $elementgroup was NOT drawn inside a frameset (no box with a title on the top left corner) and it was drawn badly. The second $elementgroup, with the same syntax BUT INSIDE a frameset, was correctly drawn. I apologise for my bloody English. About your suggestion: I agree with you, leaving valid what I addressed to Eloy this morning (30/Jun/11 2:29 PM) here in this MDL-issue. Thanks again, Mary.
        Hide
        Mary Evans added a comment -

        Ciao Daniele,

        This change only needs to be done in base/style/core.css no need to touch Canvas. But yes you need to amend your Formal White in core.css and config.php to fix the parent, as this is IMPORTANT!
        You can see the result of the change I made to Base theme's core.css on my website.
        First click this link...
        http://visible-expression.co.uk/sandbox/?theme=formal_white

        Then go to the test page...
        http://visible-expression.co.uk/sandbox/test.php

        Thanks
        Mary

        Show
        Mary Evans added a comment - Ciao Daniele, This change only needs to be done in base/style/core.css no need to touch Canvas. But yes you need to amend your Formal White in core.css and config.php to fix the parent, as this is IMPORTANT! You can see the result of the change I made to Base theme's core.css on my website. First click this link... http://visible-expression.co.uk/sandbox/?theme=formal_white Then go to the test page... http://visible-expression.co.uk/sandbox/test.php Thanks Mary
        Hide
        Daniele Cordella added a comment -

        tty, Mary.

        Show
        Daniele Cordella added a comment - tty, Mary.
        Hide
        Mary Evans added a comment - - edited

        Daniele,
        I've just been testing this some more.

        I think it may be wise to remove the .mform code from canvas theme as this is what is causing the bad display re aligning: center. In fact I'm not sure why it was added in the first place. I've just checked in standard theme and test.php works perfectly.

        Testing in base revealed another css rule which we had missed before.

        p, fieldset, table, pre

        { margin-bottom: 1em; }

        This come from YUI3 base-min.css so I think it would be wise to just add the following css rule .fieldset.group

        { margin-bottom: 0}

        this makes all the elements lines up neatly, one above the other.

        My suggestions are...

        ADD .fieldset.group

        { margin-bottom: 0}

        to theme/base/style/core.css

        REMOVE .mform .hidden .fitem .fgroup

        {width: 100%;text-align: center; margin: 1em 0;}

        from theme/canvase/style/core.css

        REMOVE .mform .hidden .fitem .fgroup

        {width: 100%;text-align: center; margin: 1em 0;}

        from theme/formal_white/style/core.css

        And I think you will find everything will work nicely again!

        Mary

        Show
        Mary Evans added a comment - - edited Daniele, I've just been testing this some more. I think it may be wise to remove the .mform code from canvas theme as this is what is causing the bad display re aligning: center. In fact I'm not sure why it was added in the first place. I've just checked in standard theme and test.php works perfectly. Testing in base revealed another css rule which we had missed before. p, fieldset, table, pre { margin-bottom: 1em; } This come from YUI3 base-min.css so I think it would be wise to just add the following css rule .fieldset.group { margin-bottom: 0} this makes all the elements lines up neatly, one above the other. My suggestions are... ADD .fieldset.group { margin-bottom: 0} to theme/base/style/core.css REMOVE .mform .hidden .fitem .fgroup {width: 100%;text-align: center; margin: 1em 0;} from theme/canvase/style/core.css REMOVE .mform .hidden .fitem .fgroup {width: 100%;text-align: center; margin: 1em 0;} from theme/formal_white/style/core.css And I think you will find everything will work nicely again! Mary
        Hide
        Daniele Cordella added a comment -

        no, please, do not assign this to Eloy. Leave it to me. I am managing a good solution for core and FW at the same time.
        I will discuss about the provided solution asap with Eloy.
        TIA

        Show
        Daniele Cordella added a comment - no, please, do not assign this to Eloy. Leave it to me. I am managing a good solution for core and FW at the same time. I will discuss about the provided solution asap with Eloy. TIA
        Hide
        Daniele Cordella added a comment -

        thanks ALL

        Show
        Daniele Cordella added a comment - thanks ALL
        Hide
        Daniele Cordella added a comment -

        test_rev2.zip is the file I used to fix the issue. I am uploading it in order to help testers to validate the fix. Credits to Mary Evans.

        Show
        Daniele Cordella added a comment - test_rev2.zip is the file I used to fix the issue. I am uploading it in order to help testers to validate the fix. Credits to Mary Evans.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks! (20, 21 and master)

        Note: there was some incorrect whitespace (one tab) in theme/formal_white/style/core.css. I've added one extra commit on each branch to fix that. New code should not arrive with incorrect whitespace.

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks! (20, 21 and master) Note: there was some incorrect whitespace (one tab) in theme/formal_white/style/core.css. I've added one extra commit on each branch to fix that. New code should not arrive with incorrect whitespace.
        Hide
        Daniele Cordella added a comment -

        Thanks Eloy, you are always better than our expectations

        Show
        Daniele Cordella added a comment - Thanks Eloy, you are always better than our expectations
        Hide
        Sam Hemelryk added a comment -

        Thanks guys - tested and confirmed fixed

        Show
        Sam Hemelryk added a comment - Thanks guys - tested and confirmed fixed
        Hide
        Mary Evans added a comment - - edited

        Great! Thanks Sam but WHY "Unresolved"???

        Show
        Mary Evans added a comment - - edited Great! Thanks Sam but WHY "Unresolved"???
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Moodle's git/cvs repositories have been updated with this piece of art! Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Moodle's git/cvs repositories have been updated with this piece of art! Thanks!
        Hide
        Sam Hemelryk added a comment -

        Hi Mary,

        Presently things stay marked as unresolved until the issue has been closed completely, e.g when the weekly it was integrated to is released or when its closed as fixed.
        Essentially closed is the final stage from which there is nowhere else the issue can go - any new issues or even the same issue if it wasn't actually fixed need to be opened as new bugs once an issue has been marked as closed.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Mary, Presently things stay marked as unresolved until the issue has been closed completely, e.g when the weekly it was integrated to is released or when its closed as fixed. Essentially closed is the final stage from which there is nowhere else the issue can go - any new issues or even the same issue if it wasn't actually fixed need to be opened as new bugs once an issue has been marked as closed. Cheers Sam

          People

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

            Dates

            • Created:
              Updated:
              Resolved: