Moodle

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

Details

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

is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-40278 Form buttons too far left in IE 8-9

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
will help resolve

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-38074 META: existing issues in Moodle core that affect Bootstrap

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Tim Hunt added a comment -

Please could someone peer review this.

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

Thanks Mary.

Show
Tim Hunt added a comment - Thanks Mary.
Hide
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: