Details
-
Type:
Bug
-
Status: Closed
-
Priority:
Minor
-
Resolution: Fixed
-
Affects Version/s: 2.3.1, 2.4.4, 2.5, 2.6
-
Component/s: Forms Library, Themes
-
Labels:
-
Testing Instructions:
-
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:
-
Pull Master Diff URL:
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
@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.
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
/** Fix IE double margin + float bugs **/
.ie .mform .fitem .fitemtitle
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.
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
Those legacy rules are causing MDL-40278. I am going to take this, and submit a fix that removes those rules.
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.
Just adding Mary Evans as a watcher, since she is a likely peer reviewer.
No. That was a misate. I will amend everything in a moment.
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.
Right. All commits amended. Any more comments, or can I submit this for integration?
Do you want me to review the changes? Or am I a mistake! LOL
You need to add some test Instructions Tim. Otherwise the Integrator will shout at you!
It's not too much of a change and if it causes problems for RTL commuity we can fix it if need be.
I have added testing instructions. I would like you to review this, Mary, but I can't tell you what to do.
Sorry Tim I missed them. I'm used to seeing a numbered list.
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.
Just thinking...if there is a problem it is likely to be when using XP & IE8.
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.
Tested, passing this now thanks Tim & Mary.
Thanks for giving me joys and smiles
Thanks for sharing my trouble's pileThanks for wipeing the tears of my eye
Thanks for showing me the glad view of skyThanks for lending me your shoulders to lean
Thanks for giving my words a proper meanThanks for telling me the value of life
Thanks for showing me the rules to surviveThanks for lending me the sympathetic ears
Thanks for showing how much you careFrom all this what I mean in the end
Is thanks for being my special friend.– Seema Chowdhury
Sent upstream so... closing, thanks!
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.