Uploaded image for project: '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

          Attachments

            Issue Links

              Activity

              Hide
              salvetore 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
              salvetore 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
              lazydaisy 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
              lazydaisy 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
              bawjaws 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
              bawjaws 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
              lazydaisy 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
              lazydaisy 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
              timhunt 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
              timhunt 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
              timhunt Tim Hunt added a comment -

              Please could someone peer review this.

              Show
              timhunt Tim Hunt added a comment - Please could someone peer review this.
              Hide
              bawjaws 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
              bawjaws 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
              timhunt Tim Hunt added a comment -

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

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

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

              Show
              timhunt Tim Hunt added a comment - No. That was a misate. I will amend everything in a moment.
              Hide
              bawjaws 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
              bawjaws 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
              timhunt Tim Hunt added a comment -

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

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

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

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

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

              Show
              lazydaisy Mary Evans added a comment - You need to add some test Instructions Tim. Otherwise the Integrator will shout at you!
              Hide
              lazydaisy 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
              lazydaisy 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
              timhunt 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
              timhunt 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
              lazydaisy Mary Evans added a comment -

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

              Show
              lazydaisy Mary Evans added a comment - Sorry Tim I missed them. I'm used to seeing a numbered list.
              Hide
              lazydaisy 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
              lazydaisy 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
              lazydaisy Mary Evans added a comment -

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

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

              Thanks Mary.

              Show
              timhunt Tim Hunt added a comment - Thanks Mary.
              Hide
              stronk7 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
              stronk7 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
              samhemelryk Sam Hemelryk added a comment - - edited

              Tested, passing this now thanks Tim & Mary.

              Show
              samhemelryk Sam Hemelryk added a comment - - edited Tested, passing this now thanks Tim & Mary.
              Hide
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    8/Jul/13