For each theme go to the following location, you should see a tabular data. Make sure it looks identical to the one without this patch. The thing to verify mostly is alternate colors for tr background.

This must be tested with all themes.
For each theme go to the following location, you should see a tabular data. Make sure it looks identical to the one without this patch. The thing to verify mostly is alternate colors for tr background.
User enrollement table
Live logs Reports for the site
Logs Reports for the site
admin/tool/customlang/index.php (open a pack for editing)
Blocks list (admin/blocks.php)
Question bank listing all questions
Create a survey with type "COLLES(ACTUAL)" and attempt it as student

Just to clarify for anyone looking at this issue later, without making this change, if we want to dynamically add rows to a table we will have to add an offset to get the correct row numbering (odd/even). This is a much safer, and more reliable solution and is compatible with all supported browsers.

I think that this should be good to go for integration. I've been having an internal debate as to whether it would be wise to keep the r0/r1 classes in place for non-core themes - but I don't think that this is necessary or wise - it will not be spotted until something weird starts to happen in a dynamic table, and will be difficult to diagnose.

Andrew Nicols
added a comment - 23/Jan/14 6:35 AM This looks good to me and makes sense.
Just to clarify for anyone looking at this issue later, without making this change, if we want to dynamically add rows to a table we will have to add an offset to get the correct row numbering (odd/even). This is a much safer, and more reliable solution and is compatible with all supported browsers.
I think that this should be good to go for integration. I've been having an internal debate as to whether it would be wise to keep the r0/r1 classes in place for non-core themes - but I don't think that this is necessary or wise - it will not be spotted until something weird starts to happen in a dynamic table, and will be difficult to diagnose.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Eloy Lafuente (stronk7)
added a comment - 23/Jan/14 2:40 PM The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
TIA and ciao

1) Should we "kill" the r0/r1 generation at some point? Please discuss about that. If agreed this needs some // TODO or @todo, pointing to a new issue, and also upgrade.txt should point about when r0/r1 are about to disappear.

2) Curiously, just a few days ago I was reading about nth-child and friends and it was clear that using nth-of-type was far better for uses like this. Note it has no much importance in this case (they are "tr" within a "table"), but to do it 100% correctly, I think we should be using nth-of-type instead (imagine we add head/body tags, "nth-child" will lead to odd/even flip-flopping. nth-of-type won't). Reference:

Eloy Lafuente (stronk7)
added a comment - 27/Jan/14 11:27 AM - edited Hi, some comments:
1) Should we "kill" the r0/r1 generation at some point? Please discuss about that. If agreed this needs some // TODO or @todo, pointing to a new issue, and also upgrade.txt should point about when r0/r1 are about to disappear.
2) Curiously, just a few days ago I was reading about nth-child and friends and it was clear that using nth-of-type was far better for uses like this. Note it has no much importance in this case (they are "tr" within a "table"), but to do it 100% correctly, I think we should be using nth-of-type instead (imagine we add head/body tags, "nth-child" will lead to odd/even flip-flopping. nth-of-type won't). Reference:
http://css-tricks.com/the-difference-between-nth-child-and-nth-of-type/
So, mainly because of 1) we need to, always, define the TTL of any "deprecated" functionality, but also coz 2)... reopening this for another round/discussion. Thanks!
Edited, to add link to the page about nth diffs.

Discussed this with Andrew again, and we agree that we can remove this in Moodle 2.9. This method of using r0/r1 for defining CSS for alternate child is highly unreliable and should be avoided. I will try to get a few more views on this and see if there are any major objection.

Ankit Agarwal
added a comment - 28/Jan/14 4:24 AM - edited Discussed this with Andrew again, and we agree that we can remove this in Moodle 2.9. This method of using r0/r1 for defining CSS for alternate child is highly unreliable and should be avoided. I will try to get a few more views on this and see if there are any major objection.

Tim Hunt
added a comment - 28/Jan/14 9:25 AM Before this is integrated, you should post in the Themes forum and General developer forum.
(Actually, you should have done that before now, rather that just plotting this in private, but better late than never.)
Theme/upgrade.txt could be better:
You could give exact instructions for how to upgrade your .r0 and .r1 rules to the new way.
Rather than describing this as a 'recommendation' you could point out that this change is necessary, because the r0 and r1 classes will be removed in Moodle 2.x
You could upgrade codechecker to check .css files and point out uses of .r0 and .r1 classes.
Please remember that every API change makes lots of work for all add-on developers.

It is a 'recommendation' at this point. Eloy suggested to remove the classes in future and me and Andrew agreed with it. We have not yet decided to go in that direction, once we do, the upgrade.txt will be updated.

Sure. Our code checker doesn't parse CSS files at the moment, afaik. Anyway I can create an MDLSITE for this.

Ankit Agarwal
added a comment - 28/Jan/14 10:32 AM - edited Thanks Tim for the feedback.
I have created a forum post to discuss this further ( https://moodle.org/mod/forum/discuss.php?d=252996 ) (There is no plotting going on here, this was raised in the dev chat, which is always the first step to get more feedback)
About Theme/upgrade.txt
Sure, why not.
It is a 'recommendation' at this point. Eloy suggested to remove the classes in future and me and Andrew agreed with it. We have not yet decided to go in that direction, once we do, the upgrade.txt will be updated.
Sure. Our code checker doesn't parse CSS files at the moment, afaik. Anyway I can create an MDLSITE for this.
Thanks

David Scotson
added a comment - 28/Jan/14 2:32 PM I filed a similar bug a while back ( MDL-40237 ).
As noted in that bug Moodle also uses the classes .odd and .even for this same thing.
It's also always struck me as odd (no pun intended) that the first row (and all other odd-numbered rows) got the number 0, and the second row (and all other even numbered rows) got the number 1.
I don't think the Bootstrapbase or Clean themes currently use these classes at all so the change shouldn't affect them too much. Actually I just checked and someone added them for the survey module late last year ( https://github.com/moodle/moodle/commit/1765866fff644d1652adde38d151377d87bafe5d ) rather than re-use the table-striped class that stripes the rest of the tables in the theme.
Also, is it just my imagination or are people going out of their way to not mention that this doesn't work in IE8?

David Scotson
added a comment - 28/Jan/14 2:38 PM Looking back at the stuff I did to get striped tables with Bootstrap:
https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/less/moodle/expendable.less#L1-L14
I note that apparently only targetting the table.flexible and .generaltable classes didn't seem to be enough, so you may want to go through that list of tables:
table#explaincaps,
table#defineroletable,
table.grading-report,
table#listdirectories,
table.rolecaps,
table.userenrolment,
table#form,
form#movecourses table,
#page-admin-course-index .editcourse,
.forumheaderlist,
and add one of the two "standard" table classes, to ensure they get striped properly ( MDL-40497 suggests that these two classnames should really be collapsed into a single one, and suggests .table).

Does IE8 matter so much in M2.6+ given -> http://docs.moodle.org/dev/Moodle_2.6_release_notes#Requirements. Surely we have to move on and improve and not be saddled to the constraints of the past. Otherwise we would still have to support Netscape Navigator and people would be asking for a Windows 286 version encapsulated within a virtual browser.

Gareth J Barnard
added a comment - 28/Jan/14 2:58 PM Does IE8 matter so much in M2.6+ given -> http://docs.moodle.org/dev/Moodle_2.6_release_notes#Requirements . Surely we have to move on and improve and not be saddled to the constraints of the past. Otherwise we would still have to support Netscape Navigator and people would be asking for a Windows 286 version encapsulated within a virtual browser.

Which reminds me, another small issue that this uncovers is the use of .generaltable for things that are neither general nor strictly tables, e.g. the layout for the assign roles screens are laid out via a table with the class .generaltable, so the current patch will probably give that layout table a pale gray background. There's more info in MDL-41361.

David Scotson
added a comment - 28/Jan/14 3:02 PM Regarding Eloy's comment about nth-of-type, the correct solution looks more like this:
https://github.com/twbs/bootstrap/blob/master/less/tables.less#L107-L114
i.e. only add the striping to the rows within the tbody element.
Which reminds me, another small issue that this uncovers is the use of .generaltable for things that are neither general nor strictly tables, e.g. the layout for the assign roles screens are laid out via a table with the class .generaltable, so the current patch will probably give that layout table a pale gray background. There's more info in MDL-41361 .

I just posted this in the forum but I think it's important enough to post it here too, looking at the diff I think we could do this much easier by lowering the specificity on our selector (sorry for copy pasta from forum):

just for the sake of consideration (and ease on themers...) I think we could lower the specificity on those selectors too. Instead of:

`table.generaltable tr:nth-of-type(odd)`

and the 50 other rules in the diff, how about

`.generaltable tr:nth-of-type(odd)`

or

`tr:nth-of-type(odd)`

here's my reasoning:

#lower specificity is ALWAYS better in CSS.
#you'll never have a semantically valid tr outside of a table so you can never unintentionally target a tr
#unfortunately not all tables in Moodle use the generaltable class - so you'll end up adding another rule or selector at some point (actually looking at the diff this is already happening.)

also, do we want all tables to be striped or should we give the option to opt out with something like this:

Daniel Wahl
added a comment - 29/Jan/14 3:13 AM I just posted this in the forum but I think it's important enough to post it here too, looking at the diff I think we could do this much easier by lowering the specificity on our selector (sorry for copy pasta from forum):
just for the sake of consideration (and ease on themers...) I think we could lower the specificity on those selectors too. Instead of:
`table.generaltable tr:nth-of-type(odd)`
and the 50 other rules in the diff, how about
`.generaltable tr:nth-of-type(odd)`
or
`tr:nth-of-type(odd)`
here's my reasoning:
#lower specificity is ALWAYS better in CSS.
#you'll never have a semantically valid tr outside of a table so you can never unintentionally target a tr
#unfortunately not all tables in Moodle use the generaltable class - so you'll end up adding another rule or selector at some point (actually looking at the diff this is already happening.)
also, do we want all tables to be striped or should we give the option to opt out with something like this:
`tr.no-stripe
{background:inherit;}
`
or
`.no-stripe tr
{background:inherit;}
`
which is probably easier for programmers to output and is less code

David, as Gareth mentioned IE8 is not supported any more , hence there is no need to worry about it.

Also thanks for pointing out the issue in the forum, r0 should be odd and r1 should be even, not the other way around. I have updated the patch accordingly. Regarding, changing css to look for tr inside tbody, do we really need to go to that specific level?

Daniel, I am not in favour of adding a CSS this generic, 'tr:nth-of-type(odd)' , since it is going to create a lot of work for not only theme developers but also other plugin developers who don't want their tables stripped.

I have also created a deprecation todo for Moodle 2.9 and updated the readme.(MDL-43902)

Ankit Agarwal
added a comment - 29/Jan/14 9:42 AM Thanks for the feedback.
David, as Gareth mentioned IE8 is not supported any more , hence there is no need to worry about it.
Also thanks for pointing out the issue in the forum, r0 should be odd and r1 should be even, not the other way around. I have updated the patch accordingly. Regarding, changing css to look for tr inside tbody, do we really need to go to that specific level?
Daniel, I am not in favour of adding a CSS this generic, 'tr:nth-of-type(odd)' , since it is going to create a lot of work for not only theme developers but also other plugin developers who don't want their tables stripped.
I have also created a deprecation todo for Moodle 2.9 and updated the readme.( MDL-43902 )

Regarding #2, if you don't target within the tbody then you are targetting the table header row(s) too. So it might work most of the time, but it's not doing what most people would expect it to do, which is to stripe the data rows in the table, not just every row.

It's also a change from the current behaviour where the header row doesn't get a r0 class added to it, but it will now be targetted by this rule.

As an added benefit, the use of this rule in Bootstrap helped us identify several areas where people hadn't marked up the header row correctly, which is an accessibility problem. This way of doing it neatly guides people to do the right thing.

It's also, as mentioned, the way that Bootstrap does it, so it simplifies people's lives if they don't have to worry about the subtle corner-cases in the way that their tables might display between different themes.

David Scotson
added a comment - 29/Jan/14 10:56 AM Regarding #2, if you don't target within the tbody then you are targetting the table header row(s) too. So it might work most of the time, but it's not doing what most people would expect it to do, which is to stripe the data rows in the table, not just every row.
It's also a change from the current behaviour where the header row doesn't get a r0 class added to it, but it will now be targetted by this rule.
As an added benefit, the use of this rule in Bootstrap helped us identify several areas where people hadn't marked up the header row correctly, which is an accessibility problem. This way of doing it neatly guides people to do the right thing.
It's also, as mentioned, the way that Bootstrap does it, so it simplifies people's lives if they don't have to worry about the subtle corner-cases in the way that their tables might display between different themes.

Also, I compared the logs page in Standard and BootstrapBase/Clean. Standard has a .generaltable .cell class which overrides the stripes by re-applying a white background to all the cells. Anyone know if that's a mistake or not?

As far as I can tell it's done here on the HTML side, and it's true for every table generated via html_writer::table.

David Scotson
added a comment - 29/Jan/14 12:14 PM For reference, the CSS that Bootstrap uses is:
.table-striped>tbody>tr:nth-child(odd)>td,
.table-striped>tbody>tr:nth-child(odd)>th
{
background-color:#f9f9f9
}
Also, I compared the logs page in Standard and BootstrapBase/Clean. Standard has a .generaltable .cell class which overrides the stripes by re-applying a white background to all the cells. Anyone know if that's a mistake or not?
As far as I can tell it's done here on the HTML side, and it's true for every table generated via html_writer::table.
https://github.com/moodle/moodle/blob/master/lib/outputcomponents.php#L1615
With this being the CSS that overules the stripes (but only for the Standard theme, not themes that inherit from Base):
https://github.com/moodle/moodle/blob/master/theme/standard/style/core.css#L68

Ankit Agarwal
added a comment - 03/Feb/14 7:40 AM I have added the tbody, as suggested by David after discussing with Andrew and Fred, since all tables must have a tbody anyway and we don't want to strip the header rows.

So I agree that the best thing to do here is to add tbody into the mix.
I think that the distinction between nth-child and nth-of-type should be moot here. Technically, a tbody can contain tr tags and other script-supporting elements (http://www.w3.org/TR/html5/tabular-data.html#the-tbody-element) so it doesn't make a huge amount of difference.

My only other thought would be to add the new selectors without removing the old ones. I say this because it although we no longer support IE8 I don't think we should deliberately remove support for it yet. The new selector is more specific so should take precedence, and the only place in which it will appear squiffy will be the new dynamically update tables.

If we add the old selectors back in such a way that they're above the new ones on their own line, we can remove them in 2.9 with the minimum of history loss:

.que .r0,

.que > tbody > tr:nth-of-type(odd) {background-color: #F5F5F5;}

.que .r1,

.que > tbody > tr:nth-of-type(even) {background-color: #EEE;}

It may be worth waiting for other's views before making the above change and checking with Integrators too.

Andrew Nicols
added a comment - 03/Feb/14 8:01 AM Cheers Ankit,
So I agree that the best thing to do here is to add tbody into the mix.
I think that the distinction between nth-child and nth-of-type should be moot here. Technically, a tbody can contain tr tags and other script-supporting elements ( http://www.w3.org/TR/html5/tabular-data.html#the-tbody-element ) so it doesn't make a huge amount of difference.
My only other thought would be to add the new selectors without removing the old ones. I say this because it although we no longer support IE8 I don't think we should deliberately remove support for it yet. The new selector is more specific so should take precedence, and the only place in which it will appear squiffy will be the new dynamically update tables.
If we add the old selectors back in such a way that they're above the new ones on their own line, we can remove them in 2.9 with the minimum of history loss:
.que .r0,
.que > tbody > tr:nth-of-type(odd) {background-color: #F5F5F5;}
.que .r1,
.que > tbody > tr:nth-of-type(even) {background-color: #EEE;}
It may be worth waiting for other's views before making the above change and checking with Integrators too.
Otherwise, this issue looks good to go for integration IMO.
Cheers, and welcome to the frontend team
Andrew

Ankit Agarwal
added a comment - 03/Feb/14 8:17 AM Thanks Andrew for the review.
I don't really like the idea of keeping the old selector. Basically because:-
Ie8 is ancient, we should not keep code, just for browsers that we don't support anymore.
Removing both css and the class from html suddenly in 2.9, doesn't sound as per our deprecation policy.
Having said that, I donot have any major objections keeping these selectors in place, if integrators agree with it.

I'd vote for keeping the rc0 and rc1 being generated till 2.9, but cleaning any use from core, and get rid of their generation in 2.9 (standard deprecation + annihilation process).

So, under those premises, I think your current patch looks perfect:

1) r0, r1 continue being generated.
2) No r0, r1 uses in core css (all them replaced by the new nth ones).
3) Correct upgrade.txt message (perhaps it could include a link to the MDL-43902).
4) There is a task to implement the check by the code-checker.

Eloy Lafuente (stronk7)
added a comment - 03/Feb/14 10:08 AM I'd vote for keeping the rc0 and rc1 being generated till 2.9, but cleaning any use from core, and get rid of their generation in 2.9 (standard deprecation + annihilation process).
So, under those premises, I think your current patch looks perfect:
1) r0, r1 continue being generated.
2) No r0, r1 uses in core css (all them replaced by the new nth ones).
3) Correct upgrade.txt message (perhaps it could include a link to the MDL-43902 ).
4) There is a task to implement the check by the code-checker.
+1 Ciao

You could add the .ie8 class before the rules that are intended for IE8 only.

This a) completely stops them affecting other browsers (as there might be some interference depending on how the stripes are defined, sometimes you only specify color for one or the other and removing a row via javascript would mean that all the odds after that become even and vice versa, but r0 and r1 will remain attached to the same row), b) documents what's going on a bit better, c) encourages developers (who will not be using IE8) to make the shift to the new way of doing things and d) makes these rules easy to find and remove at a later date,

(I already have script that can strip .ie7 and .ie6 specific code that's coded in this way, which helps save some space, and lets people who know they don't need the support get a head start on the benefits)

David Scotson
added a comment - 03/Feb/14 4:43 PM You could add the .ie8 class before the rules that are intended for IE8 only.
This a) completely stops them affecting other browsers (as there might be some interference depending on how the stripes are defined, sometimes you only specify color for one or the other and removing a row via javascript would mean that all the odds after that become even and vice versa, but r0 and r1 will remain attached to the same row), b) documents what's going on a bit better, c) encourages developers (who will not be using IE8) to make the shift to the new way of doing things and d) makes these rules easy to find and remove at a later date,
(I already have script that can strip .ie7 and .ie6 specific code that's coded in this way, which helps save some space, and lets people who know they don't need the support get a head start on the benefits)

I am submitting this for integration as I do not think we should be considering backward compatibility for unsupported versions, while writing the improvements. Also this can go forward now, since an integrator as +1ed it.

Ankit Agarwal
added a comment - 06/Feb/14 7:34 AM Thanks for all the feedback on this, really appreciated.
I am submitting this for integration as I do not think we should be considering backward compatibility for unsupported versions, while writing the improvements. Also this can go forward now, since an integrator as +1ed it.
Cheers

Sam Hemelryk
added a comment - 09/Feb/14 9:05 PM Thanks Ankit this has been integrated now.
Please note I did add a commit to add a little more detail to theme/upgrade.txt (pre-empting the "why" questions)

David Scotson
added a comment - 10/Feb/14 5:16 PM Just to note that on Survey, at least one of the Survey question types (mis-)used the r0 and r1 classes to highlight the rows two at a time.
I think it works better with alternating rows, but thought I'd mention it anyway.