Moodle
  1. Moodle
  2. MDL-34610

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

    Details

    • Rank:
      43046

      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.

        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: