Moodle
  1. Moodle
  2. MDL-34610

IE specific CSS in forms may not apply to IE 8, 9, 10 etc.

    Details

    • Testing Instructions:
      Hide
      1. Using Internet Explorer select Standard theme
      2. TEST a range of forms.
      3. Verify that the layout is good. In particular, verify that for any form fields that do no have labels (for example the submit buttons, or the Show more... links) are lined up with the other form fields, not hard over on the left.
      4. Test also in a RTL language like Hebrew or Arabic.
      Show
      Using Internet Explorer select Standard theme TEST a range of forms. Verify that the layout is good. In particular, verify that for any form fields that do no have labels (for example the submit buttons, or the Show more... links) are lined up with the other form fields, not hard over on the left. Test also in a RTL language like Hebrew or Arabic.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      There's a couple of CSS rules that start .ie .mform that confused me when trying to get forms laying out consistently across browsers. I'm not sure what they're doing, I assume they're working around some IE bug, but it's not clear that the bug applies to versions of IE that are still supported by Moodle 2.3 and if not then it might be better to have a single layout that applies to all browsers so that people aren't seeing subtly different things if there is no longer a reason for that.

      Also, there's a variety of rules in the base css that start .ie6 for IE6 (and possibly the same for 7). If these are no longer supported then they can just be deleted to save download size.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for spotting that.

            If you could add more details about locations, that would help. If you could go a step further and test what happens when they are removed that would be even more helpful. Submitting a patch which implements changes would be the ultimate in helpfulness.

            Show
            Michael de Raadt added a comment - Thanks for spotting that. If you could add more details about locations, that would help. If you could go a step further and test what happens when they are removed that would be even more helpful. Submitting a patch which implements changes would be the ultimate in helpfulness.
            Hide
            Mary Evans added a comment -

            @David

            The .ie .ie6 .ie7 .ie8 .ie9 bodyclasses are added to every page when using any one of these versions of Internet Explorer browsers. To check this, just 'View Source Code' when in IE and look at the various classes listed in the <body> tag.

            There are, as you have correctly stated, a number of IE6 CSS rules in BASE theme which perhaps don't need to be there in Moodle 2.3.x as IE6 is no longer supported, but that is something that needs clarification from Moodle HQ before I can remove them. The fact you have raised this issue is reason enough to look into this, so thanks for that.

            As for the rules you mentioned, relating to forms. It would be helpful to me if you could list the locations of these specific stylesheets within Moodle.

            Show
            Mary Evans added a comment - @David The .ie .ie6 .ie7 .ie8 .ie9 bodyclasses are added to every page when using any one of these versions of Internet Explorer browsers. To check this, just 'View Source Code' when in IE and look at the various classes listed in the <body> tag. There are, as you have correctly stated, a number of IE6 CSS rules in BASE theme which perhaps don't need to be there in Moodle 2.3.x as IE6 is no longer supported, but that is something that needs clarification from Moodle HQ before I can remove them. The fact you have raised this issue is reason enough to look into this, so thanks for that. As for the rules you mentioned, relating to forms. It would be helpful to me if you could list the locations of these specific stylesheets within Moodle.
            Hide
            David Scotson added a comment -

            Around line 235 in base/style/core.css there's a couple of lines with comments.

            base/style/core.css:235:/** Fix IE double margin + float bugs **/

            /** Browser corrections for mforms **/
            .ie .mform .fitem .felement

            {margin-left:0;text-align:left;float:left;}

            /** Fix IE double margin + float bugs **/
            .ie .mform .fitem .fitemtitle

            {padding-right:1em;}

            Googling "double margin + float bugs" seems to suggest this is an IE 5/6 bug, and I suppose looking back through CVS could show when it was added.

            Show
            David Scotson added a comment - Around line 235 in base/style/core.css there's a couple of lines with comments. base/style/core.css:235:/** Fix IE double margin + float bugs **/ /** Browser corrections for mforms **/ .ie .mform .fitem .felement {margin-left:0;text-align:left;float:left;} /** Fix IE double margin + float bugs **/ .ie .mform .fitem .fitemtitle {padding-right:1em;} Googling "double margin + float bugs" seems to suggest this is an IE 5/6 bug, and I suppose looking back through CVS could show when it was added.
            Hide
            Mary Evans added a comment -

            We could probably drop those easy enough. Or at best identify them as .ie7 or .ie8 as we are, still committed to IE7 & IE8. if they are not needed then they can go.

            Thanks

            Show
            Mary Evans added a comment - We could probably drop those easy enough. Or at best identify them as .ie7 or .ie8 as we are, still committed to IE7 & IE8. if they are not needed then they can go. Thanks
            Hide
            Tim Hunt added a comment -

            Those legacy rules are causing MDL-40278. I am going to take this, and submit a fix that removes those rules.

            Show
            Tim Hunt added a comment - Those legacy rules are causing MDL-40278 . I am going to take this, and submit a fix that removes those rules.
            Hide
            Tim Hunt added a comment -

            Please could someone peer review this.

            Show
            Tim Hunt added a comment - Please could someone peer review this.
            Hide
            David Scotson added a comment -

            Is the change to the #portfolio-add-button intentional? It looks like it might be grouped with the other rules, but it doesn't start with .ie so you'd see the effects on all browsers. It's also a bit more specific so it's probably only on one single page which should be specifically checked if we know where it is.

            Show
            David Scotson added a comment - Is the change to the #portfolio-add-button intentional? It looks like it might be grouped with the other rules, but it doesn't start with .ie so you'd see the effects on all browsers. It's also a bit more specific so it's probably only on one single page which should be specifically checked if we know where it is.
            Hide
            Tim Hunt added a comment -

            Just adding Mary Evans as a watcher, since she is a likely peer reviewer.

            Show
            Tim Hunt added a comment - Just adding Mary Evans as a watcher, since she is a likely peer reviewer.
            Hide
            Tim Hunt added a comment -

            No. That was a misate. I will amend everything in a moment.

            Show
            Tim Hunt added a comment - No. That was a misate. I will amend everything in a moment.
            Hide
            David Scotson added a comment -

            There's also some .ie rules in grade/report/grader/styles.css and a smattering of other places that probably need looked at to see if they're still relevant. Not directly linked to your change, but should either be fixed at the same time or spun off into another bug I think.

            Show
            David Scotson added a comment - There's also some .ie rules in grade/report/grader/styles.css and a smattering of other places that probably need looked at to see if they're still relevant. Not directly linked to your change, but should either be fixed at the same time or spun off into another bug I think.
            Hide
            Tim Hunt added a comment -

            Right. All commits amended. Any more comments, or can I submit this for integration?

            Show
            Tim Hunt added a comment - Right. All commits amended. Any more comments, or can I submit this for integration?
            Hide
            Mary Evans added a comment -

            Do you want me to review the changes? Or am I a mistake! LOL

            Show
            Mary Evans added a comment - Do you want me to review the changes? Or am I a mistake! LOL
            Hide
            Mary Evans added a comment -

            You need to add some test Instructions Tim. Otherwise the Integrator will shout at you!

            Show
            Mary Evans added a comment - You need to add some test Instructions Tim. Otherwise the Integrator will shout at you!
            Hide
            Mary Evans added a comment -

            It's not too much of a change and if it causes problems for RTL commuity we can fix it if need be.

            Show
            Mary Evans added a comment - It's not too much of a change and if it causes problems for RTL commuity we can fix it if need be.
            Hide
            Tim Hunt added a comment -

            I have added testing instructions. I would like you to review this, Mary, but I can't tell you what to do.

            Show
            Tim Hunt added a comment - I have added testing instructions. I would like you to review this, Mary, but I can't tell you what to do.
            Hide
            Mary Evans added a comment -

            Sorry Tim I missed them. I'm used to seeing a numbered list.

            Show
            Mary Evans added a comment - Sorry Tim I missed them. I'm used to seeing a numbered list.
            Hide
            Mary Evans added a comment -

            Tested an area in an assignment setting page form. The CSS which you have removed from base core.css turned out to be used in the box that the tinyMCE editor sits and found that removing the CSS made no difference.

            I used a user agent simulator for IE6/IE7 & IE8 none of which showed any problems.

            I expect that in Integration Review the real test will need to be carried out on the real browsers. It works OK in IE9.

            I think this is OK so will push it for integration.

            Show
            Mary Evans added a comment - Tested an area in an assignment setting page form. The CSS which you have removed from base core.css turned out to be used in the box that the tinyMCE editor sits and found that removing the CSS made no difference. I used a user agent simulator for IE6/IE7 & IE8 none of which showed any problems. I expect that in Integration Review the real test will need to be carried out on the real browsers. It works OK in IE9. I think this is OK so will push it for integration.
            Hide
            Mary Evans added a comment -

            Just thinking...if there is a problem it is likely to be when using XP & IE8.

            Show
            Mary Evans added a comment - Just thinking...if there is a problem it is likely to be when using XP & IE8.
            Hide
            Tim Hunt added a comment -

            Thanks Mary.

            Show
            Tim Hunt added a comment - Thanks Mary.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            It really stresses me to change things like this for sooo long stable branches (23, 24), but I trust you. If they are useless, they are useless.

            Integrated (23, 24, 25 & master), thanks!

            To tester: It would be great to test this under IE8 and upwards.

            Show
            Eloy Lafuente (stronk7) added a comment - It really stresses me to change things like this for sooo long stable branches (23, 24), but I trust you. If they are useless, they are useless. Integrated (23, 24, 25 & master), thanks! To tester: It would be great to test this under IE8 and upwards.
            Hide
            Sam Hemelryk added a comment - - edited

            Tested, passing this now thanks Tim & Mary.

            Show
            Sam Hemelryk added a comment - - edited Tested, passing this now thanks Tim & Mary.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Thanks for giving me joys and smiles
            Thanks for sharing my trouble's pile

            Thanks for wipeing the tears of my eye
            Thanks for showing me the glad view of sky

            Thanks for lending me your shoulders to lean
            Thanks for giving my words a proper mean

            Thanks for telling me the value of life
            Thanks for showing me the rules to survive

            Thanks for lending me the sympathetic ears
            Thanks for showing how much you care

            From all this what I mean in the end
            Is thanks for being my special friend.

            – Seema Chowdhury

            Sent upstream so... closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: