Why do this?
- Gets us up to date with CSS3 best practices
- Removes some markup bloat = faster mobile experience
- Removes some PHP logic from theme functions and templates: every little bit helps in making theming easier

However, the :(first|last|nth)-child selectors don't work in IE8. Is it okay for core to ship without table/list striping on IE8? Contrib can add something like Selectivizr, but do we need to add something like that to core as part of this patch?

CommentFileSizeAuthor
#149 odd_even_first_last-1649780-131.patch25.03 KBstar-szr
#141 interdiff.txt1.98 KBstar-szr
#141 1649780-141.patch25.37 KBstar-szr
#136 odd_even_first_last-1649780-136.patch23.72 KBkaare
#131 interdiff.txt2.06 KBWim Leers
#131 odd_even_first_last-1649780-131.patch25.03 KBWim Leers
#127 interdiff.txt3.08 KBWim Leers
#127 odd_even_first_last-1649780-127.patch25 KBWim Leers
#126 interdiff.txt16.53 KBWim Leers
#126 odd_even_first_last-1649780-126.patch22.4 KBWim Leers
#122 1649780-122-remove-zebra.patch6.99 KBjoelpittet
#96 1649780-96.patch35.8 KBhefox
#90 1649780-90.patch37.46 KBjibran
#90 interdiff.txt1.41 KBjibran
#69 nth-child-modules.png14.44 KBeffulgentsia
#68 1649780-68.patch37.22 KBfubhy
#67 1649780-67.patch37.85 KBfubhy
#54 1649780-54.patch36.47 KBfubhy
#51 drupal8.css-zebra.51.patch36.48 KBjibran
#48 drupal8.css-zebra.48.patch36.5 KBjibran
#45 drupal8.css-zebra.45.patch35.75 KBjibran
#40 drupal8.css-zebra.40.patch35.42 KBBarisW
#32 drupal8.css-zebra.32.patch33.29 KBsun
#32 interdiff.txt934 bytessun
#30 manage-fields-zebra.jpg82.53 KBeffulgentsia
#28 core-css-nth-zebra-1649780-without-last-child.txt2.84 KBeffulgentsia
#28 theme-selector-without-last-child.jpg95.02 KBeffulgentsia
#28 bartik-without-last-child.jpg79.73 KBeffulgentsia
#27 core-css-nth-zebra-1649780-27.patch33.19 KBeffulgentsia
#27 interdiff.txt820 byteseffulgentsia
#25 core-css-nth-zebra-1649780-25.patch33.08 KBeffulgentsia
#23 core-css-nth-zebra-1649780-23.patch34.57 KBnod_
#20 core-css-nth-zebra-1649780-20.patch35.25 KBrbayliss
#16 core-css-nth-zebra-1649780-16.patch34.57 KBrbayliss
#15 core-css-nth-zebra-1649780-15.patch34.38 KBnod_
#13 core-css-nth-zebra-1649780-13.patch34.37 KBnod_
#5 drupal.nth-child.5.patch35.37 KBeffulgentsia
#1 drupal.nth-child.patch34.4 KBeffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Status: Active » Needs review
FileSize
34.4 KB

Here's a start.

nod_’s picture

Issue tags: +JavaScript clean-up

Oh my, yes please.

So many JS going away can only be a good thing. And It get rid of the jQuery selector of death, that makes me happy too.

nod_’s picture

So tried out the tabledrag with the patch and tabledrag adds hidden rows to the table so this part needs more work. I'm happy to leave it alone for now and not worry about it since #1524414: Rewrite tabledrag.js to use jQuery UI will change tabledrag and will come up with a better way of dealing with this than hidden rows.

The rewrite will take significant time and should not hold back this issue. I'm all for getting rid of all this JS ASAP even if two rows have the same color for a while.

Status: Needs review » Needs work

The last submitted patch, drupal.nth-child.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
35.37 KB
droplet’s picture

Not every tables with "even, odd", this patch will change all tables. (eg. no zebra on Status Report table)

kristiaanvandeneynde’s picture

I fully support this change: striping is a visual help, not functionality.

Why we should still keep PHP/JS logic for this anno 2013 (D8 release) is beyond me. There is a perfectly valid alternative to this (CSS3), it unnecessarily increases page delivery time and it could never "break" a website if it were converted to modern CSS-only code.

sun’s picture

Status: Needs review » Needs work

Lovely patch. I agree with almost all of the removals.

Not sure whether we want to remove some of the counter variables though. There might be other use-cases besides class names for which they might be useful -- e.g., determining how many blocks are in a region, or how many links are in a link list, etc. But of course, if there are other ways to retrieve the respective counters, then my point is moot. Didn't look at the surrounding code in detail.

Second, some of the CSS changes touch selectors along the lines of:

-foo.odd, foo.even {
+foo {

I'm relatively sure that the new resulting selector duplicates an existing foo selector, either defined right above it or somewhere else. Those need to be cleaned up before this can land; i.e., we don't want to introduce duplicate selectors/styles.

Marking needs work for that.

Aside from that, I also agree with @nod_ that we should rather commit this and fix any remaining/resulting issues in separate follow-ups.

effulgentsia’s picture

Status: Needs review » Needs work

Not sure whether we want to remove some of the counter variables though. There might be other use-cases besides class names for which they might be useful -- e.g., determining how many blocks are in a region, or how many links are in a link list, etc.

I do not see any such use-cases in core. I don't think it makes sense to retain counters in template_preprocess_*() to support hypothetical contrib use-cases, when contrib can add its own hook_preprocess_*() implementation with a counter if it wants one.

I'm relatively sure that the new resulting selector duplicates an existing foo selector

Not from what I can tell (I just rechecked), but it's possible I'm overlooking something, so please point out a specific one if you find one.

Here's some questions I have, as someone who's not that proficient with CSS.

+++ b/core/themes/seven/reset.css
@@ -80,8 +80,8 @@ ul.links li,
-tr.even,
-tr.odd,
+tr:nth-child(even),
+tr:nth-child(odd),
...
-tr.odd .form-item,
-tr.even .form-item,
+tr:nth-child(odd) .form-item,
+tr:nth-child(even) .form-item,

Is it silly to have a selector for both even and odd like this (as opposed to simply tr and tr .form-item), or is it necessary in the reset file to ensure proper precedence? For that matter, are there other precedence issues we need to worry about in changing selectors from classes to pseudo-classes?

Also, in each case, I replaced .even with :nth-child(even) and .odd with :nth-child(odd), but I didn't check if in all cases whether the starting row is the same (I forget if Drupal rows/items/fields/blocks always start consistently with even or odd, and which one CSS3 starts with). This is made even more confusing in situations where we have a header row. This could use some in-depth checking from someone who knows the different cases that need to be looked at.

Finally, as per #6, in HEAD, we have some rows and items that do not get even/odd classes, including weird stuff like this:

+++ b/core/modules/field_ui/field_ui.admin.inc
@@ -238,7 +238,6 @@ function theme_field_ui_table($variables) {
-        'no_striping' => TRUE,

The CSS in #5 makes no adjustments for that.

Per last sentence in #8, I don't know which of the above are appropriate to deal with now vs. in follow-ups. Guidance from a markup/CSS maintainer would be helpful.

effulgentsia’s picture

Status: Needs work » Needs review

Back to "needs review" as I'm not aware of a concrete change that's required at this time.

nod_’s picture

I'm not sure what the reset is supposed to do here but if the same rule is applied for both of them, just tr and tr .form-item would be enough.

To work around the header issue the selectors can be changed to tbody tr:nth-child(even|odd). Any tabledrag weirdness can be overlooked for now (tabledrag likes adding "header" rows in the middle of a table, I'm not aware of an other thing in core that does the same).

Actually, better than selecting tbody tr, we should introduce a zebra class on tbody (or the whole table) and use that to prefix the rules. That mean we can keep the no_stripping option that is useful to have around and make it add/remove the new zebra class.

Also :last-child will not work on IE8. I don't particularly care but it's good to know.

The weirdness around .odd and .even is because jQuery switches the two, which lead to really really weirds selectors. Since it's all CSS now no need to worry.

droplet’s picture

Odd/Even classes have there own purpose. when removing it, seems like broken HTML5 cleanup rules? It can remove nth-child too.

+1 nod_

nod_’s picture

Status: Needs work » Needs review
FileSize
34.37 KB

updated patch keeping the no_striping thing and using a .zebra class on trs

An interesting thing is that the even/odd CSS thing is based on the position relative to it's siblings, not relative to the CSS selector. So there are still some funky things going on because of hidden table rows.

Status: Needs review » Needs work

The last submitted patch, core-css-nth-zebra-1649780-13.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
FileSize
34.38 KB
rbayliss’s picture

Status: Needs review » Needs work
FileSize
34.57 KB

Reroll of nod_'s patch, also removes a few more unused variables from theme.inc.

rbayliss’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, core-css-nth-zebra-1649780-16.patch, failed testing.

nod_’s picture

D7 patch, please reroll for D8 :)

rbayliss’s picture

nod_’s picture

Status: Needs work » Needs review
RobW’s picture

Oh yes, thank you all. I am really excited to see these helper classes being stripped out.

Why not remove :odd, set the default color with .zebra tr, and add only the :even selector for striped styling? Nth selectors are comparatively resource intensive, the less the better.

Also, if it's possible this will be applied this to mixed element siblings (<p>'s mixed in with <h3>'s for example), be aware of the differences between nth-child and nth-of-type.

nod_’s picture

reroll only rules to even or odd elements, not both.

:nth-child is always used and prefixed with .zebra, td, .block or a .simpletest-* class. I don't see how that can match mixed elements as is. Needs to be careful for the future though you're right.

Status: Needs review » Needs work

The last submitted patch, core-css-nth-zebra-1649780-23.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
33.08 KB

Rerolled without shortcut.admin.js changes, since that code was removed in #1304486: Completely remove the ability to limit the number of shortcuts per set.

jessebeach’s picture

Wow, so nice to see so much processing overhead and AJAX complication for a few stripes get removed!

To follow CSS standards, please order the CSS properties by alpha order in core/themes/seven/style.css#tr.zebra near line 444.

tr.zebra {
  background: #f3f4ee;
  border-color: #bebfb9;
  border-style: solid;
  border-width: 0 1px;
}

Not that much of the rest of the file follows the standard, but it never will unless we make small improvements like this.

Also, we're going to run into problems of inconsistent striping on <tr>'s since tables can have all kinds of crufty rows for delineating content rather than displaying data. We can address this in the future by chunking content with multiple <tbody> elements. A quick search of StackOverflow suggests this is a fine idea. The HTML DTD, as the SO page notes, allows for multiple tbody tags.

<!ELEMENT table
     (caption?, (col*|colgroup*), thead?, tfoot?, (tbody+|tr+))>

I tested back to IE7 which correctly interprets the HTML and constructs the DOM accordingly. Multiple <tbody> elements would give the <tr> nodes the correct context for odd/even selection. But this would be a refinement to an already desirable change here.

Other than that it looks great. I'd recommend committing it.

effulgentsia’s picture

please order the CSS properties by alpha order

This does so in the 2 places affected by this issue.

effulgentsia’s picture

This is looking nearly RTBC to me. Some issues though:

From the issue summary:

However, the :(first|last|nth)-child selectors don't work in IE8. Is it okay for core to ship without table/list striping on IE8?

Firstly, I was incorrect about first-child. That is supported on IE8. But last and nth aren't. In #27, nth is only used zebra striping, and I checked with Dries and webchick on that, and they said they're ok with core admin tables not being zebra striped on IE8. However, last-child presents some issues. To simulate what things would look like on IE8, here's a patch that can be applied on top of #27 to remove the last-child selectors (attached as a txt file since it's for demo purposes, not intended for review or commit). You'll notice there's selectors there for the theme selector form and for several Bartik things. So also attached are screenshots for both. Note that the theme selector one can be replicated from a completely stock D8 install. None of the Bartik issues appear on a stock install though: they only happen when you put the user login block in the header, blocks in the tryptich region, or blocks with links in the footer.

Are these IE8 degradations acceptable? As per issue summary, contrib can fix these issues with something like http://selectivizr.com/, but are these degradations acceptable for sites running core alone?

droplet’s picture

The patch introduce a new issue mentioned in #13 @nod_, wrong zebra with hidden rows

effulgentsia’s picture

FileSize
82.53 KB

Yep. Here's a screenshot that demonstrates the problem on "Manage fields". Two problems with it:
- The first row is the non-white color, whereas it should start with white. Note that on admin/modules, it correctly starts white; here it doesn't because there's a hidden first row.
- There's a hidden row before "Add new field", so the striping gets shifted by one there as well, causing two visually consecutive rows to be the same color.

webchick’s picture

Hm. I'd say leave for Dries to make the final call here, but the screenshots in #28 look pretty obviously buggy to me (especially bartik-without-last-child.jpg) and I think is inconsistent with us saying we "support" IE8. They're particularly bad because they're user-facing problems, not admin-facing problems. So I'd say this isn't really acceptable, no. :(

The screenshot in #30 though is not that big of a deal. I actually saw nothing wrong with it at all until I read the comment.

sun’s picture

FileSize
934 bytes
33.29 KB

Contrary to @webchick, the screenshot of completely broken zebra striping in #30 bugs me much more than the IE8 quirks in #28.

Status: Needs review » Needs work
Issue tags: -JavaScript clean-up

The last submitted patch, drupal8.css-zebra.32.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript clean-up

#32: drupal8.css-zebra.32.patch queued for re-testing.

effulgentsia’s picture

+++ b/core/themes/bartik/css/style.css
@@ -210,12 +206,11 @@ tr th {
+tr.zebra:nth-of-type(odd) {
+  background-color: #dddddd;
 }

If nth-of-type is better than nth-child for Bartik, let's do the same for Seven, which is what #30 was taken on, but regardless, it doesn't fix the problem since hidden rows are also tr elements.

RobW’s picture

To fix the ie8 separator issue, change the border to border-left:1px solid #cdcdcd; and remove it with ie8 supported first-child.

For the zebra striping in the manage fields section, you could apply the zebra with tr.draggable:nth-of-type(odd);, since the hidden tr doesn't have the draggable class. But, I would be careful to make sure that pattern is being repeated everywhere that tr zebra striping is being applied (or at least where all draggable zebra striping is being applied).

I haven't tested the patch yet so please tell me if I'm mistaken, but it looks like we're applying the zebra class to all tr's. It would be lighter and more re-usable if we applied them to the tr container, something like <tbody class="zebra">, .zebra tr:nth-of-type(odd);. What we should be working towards are re-usable utility classes that can be applied to a couple different html objects, maybe ol, ul, and tbody to start.

Why is the hidden tr used? Is there a way we can remove the problem, simplify the system at the same time?

Sorry I don't have more time to actually dig into the patch and code right now.

michaelfavia’s picture

@eff in re to #30: ive always wondered why the "add fields/groups" are part of that draggable table anyway. any reason not to just beak them out into their own?

RobW’s picture

As part of the draggable table you can add and arrange the new field in one step.

jide’s picture

I can't imagine the number of bytes that will be saved on the world thanks to this patch :)

BarisW’s picture

FileSize
35.42 KB

Had some troubles applying the patch from #32 on HEAD, but managed in the end after some manual changes.

I've implemented the suggestions from #38, including adding the 'zebra' class to the table element instead of every row.
The drawback is that I had to use !important on tr.drag and tr.drag-previous (which isn't a big thing, as it's an action class).

The problem on the field UI overview isn't tackled yet, even tr.draggable:nth-of-type(odd) didn't work.

Patch will fail as I didn't change the tests.

Status: Needs review » Needs work

The last submitted patch, drupal8.css-zebra.40.patch, failed testing.

Fabianx’s picture

I am fo a solution like:

.zebra tr:nth-of-type(odd),
.zebra .odd {
// ...
}

And then for IE8 add a very short JS that adds those classes dynamically to all tr's in .zebra.

catch’s picture

Assigned: Unassigned » Dries

Now that we have http://drupal.org/node/1722502 and a mobile-first layout in IE8 I'm going to assign this issue to Dries.

The question is whether the screenshots in #28 are 'acceptably broken' for IE8, I think they're fine - site looks ugly but still works. I wouldn't object to adding an IE8-only shiv to add back the classes but don't think that should hold up cleaning up the rest of core's CSS and markup (i.e. it could be a normal follow-up to this issue).

effulgentsia’s picture

Assigned: Dries » Unassigned

#40 solves the IE8 problems. #30 is still a problem for all browsers, I think, though updating this patch for 2 months of head drift and getting new screenshots would be nice before asking Dries for comment.

jibran’s picture

Status: Needs work » Needs review
FileSize
35.75 KB

re-roll of #40. Screenshots coming soon.

jibran’s picture

Title: Remove first/last/odd/even classes from markup, use CSS3 selectors instead » Remove first/last/odd/even classes in favour of CSS3 pseudo selectors
Issue tags: +Needs screenshots

Updated title and added screenshot tag

Status: Needs review » Needs work

The last submitted patch, drupal8.css-zebra.45.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
36.5 KB

I think this will fix ConfigEntityListTest

Status: Needs review » Needs work

The last submitted patch, drupal8.css-zebra.48.patch, failed testing.

tim.plunkett’s picture

Title: Remove first/last/odd/even classes in favour of CSS3 pseudo selectors » Remove first/last/odd/even classes in favor of CSS3 pseudo selectors

:)

jibran’s picture

Status: Needs work » Needs review
FileSize
36.48 KB

I think this will fix TableTest

fubhy’s picture

#51: drupal8.css-zebra.51.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs screenshots, +JavaScript clean-up

The last submitted patch, drupal8.css-zebra.51.patch, failed testing.

fubhy’s picture

Status: Needs work » Needs review
FileSize
36.47 KB

Re-roll.

catch’s picture

This looks RTBC to me but should the striping class name really be 'zebra' or something like 'stripe'?

fubhy’s picture

I like "zebra". We've been using "zebra" as the variable name ever since so it's just natural that we keep using that as the class name now.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough. Marking RTBC, I'll give it a couple of days so people have time to review before committing.

sun’s picture

Status: Reviewed & tested by the community » Needs work

This looks great.

+++ b/core/modules/system/system.admin-rtl.css
-.theme-selector .operations li.last {
+.theme-selector .operations li:last-child {

+++ b/core/themes/bartik/css/style-rtl.css
-.region-header #block-user-login .item-list li.last {
+.region-header #block-user-login .item-list li:last-child {
...
-.region-header #block-user-login ul.openid-links li.last {
+.region-header #block-user-login ul.openid-links li:last-child {
...
-#footer li.last a {
+#footer li:last-child a {

+++ b/core/themes/bartik/css/style.css
-.region-header .block-menu li.last a {
+.region-header .block-menu li:last-child a {
...
-.region-header #block-user-login .item-list li.last {
+.region-header #block-user-login .item-list li:last-child {
...
-.region-header #block-user-login ul.openid-links li.last {
+.region-header #block-user-login ul.openid-links li:last-child {
...
-#triptych .block.last {
+#triptych .block:last-child {
...
-#footer li.last a {
+#footer li:last-child a {

IE8 does not support :last-child.

However, count me +1 and all-in on simply accepting that some output might look "suboptimal" in IE8.

At the time D8 will be released, shipped, and actively used (~2014), IE10 will be fully released, stable, and mature. I am very confident that #1787012: [policy, no patch] Write D8 JS against ECMAScript 5. Prevent errors with feature detection (drop IE8 support) will still happen for D8.

Therefore, I do not see a reason to hold up this patch for that.

+++ b/core/modules/system/system.admin-rtl.css
@@ -57,11 +57,11 @@
+.theme-selector .operations li:first-child {

+++ b/core/modules/system/system.admin.css
@@ -291,12 +291,9 @@
   border-right: 1px solid #cdcdcd;  /* LTR */
...
+.theme-selector .operations li:first-child {
...
+  border-left: none; /* LTR */

It doesn't look like this has been tested in RTL — the border-left is marked as LTR, but not overridden accordingly in the RTL styles.

Also, color me confused on the border-left: none in the first place — the li only defines a border-right but not a border-left.

+++ b/core/themes/seven/style.css
@@ -395,21 +395,20 @@ table th {
 tr.drag {
-  background: #fe7;
+  background: #fe7 !important;
 }
 tr.drag-previous {
-  background: #ffb;
+  background: #ffb !important;
 }

I do not understand why !important is required here. The selector specificity should be resolved in a way that does not require !important.

I'm aware that these lines are located in a theme. However, all core themes may be used as base themes, which is why they should not contain !important rules, just like module CSS should not contain them.

jibran’s picture

What about the hidden rows problem mentioned in #13 #29 and #30?

sun’s picture

Indeed, the major remaining problem are hidden table rows. CSS3 does not provide an answer for that, since it doesn't have a :visible pseudo-selector à la jQuery.

I've grepped the net once more for potential tricks and workarounds, but there are none. Not even the :not() selector helps, because 1) browsers internally optimize the selector and execute the :nth-child() first, and 2) :nth-child() applies to the actual DOM structure, not the CSS selector.

I wonder whether we should defer this to D9, at which point there might be CSS4 with better facilities?

kristiaanvandeneynde’s picture

If hidden rows are going to be JavaScript manipulated anyhow, we could use JS to manipulate the DOM in such a way that the hidden rows don't mess up striping?

nod_’s picture

I'd say we get it in for D8. The hidden rows are an artefact of tabledrag which is going to be rewritten to be mobile-friendly and I'll make sure it doesn't messes up table rows. It comes from the fact that we try to have several "areas" inside of one table where we'd want to have several tbodies to do that properly.

Anyway, if tabledrag is the only issue it shouldn't be hold up on it, that'll only make #1261002: Draggable tables do not work on touch screen devices and #1524414: Rewrite tabledrag.js to use jQuery UI more important :) so that's a good thing.

krishanchandra’s picture

Ya, I am also facing this issue. It is important issue, So we will hope that resolved in Drupal 8.

effulgentsia’s picture

Given #62, sounds like all that's left then on this is what's in #58: changing selectors from :last-child to :first-child (which will require corresponding left/right changes to properties), ensuring RTL is correct, and removing the !important. Any volunteers?

catch’s picture

I'd be OK with breaking hidden rows/tabledrag slightly here and adding them back in the issues nod_ links to or another major. This is just completely pointless HTML weight and it'd be great to get rid of it.

fubhy’s picture

Assigned: Unassigned » fubhy

Okay, agreed.

fubhy’s picture

Assigned: fubhy » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
37.85 KB

Fixed most of the points in #58 although not all occurrences of :last-child can be fully fixed. Whenever borders were affected I changed that to :first-child instead but for paddings and such that doesn't really work. I think we just have to live with that... It's not a big issue if you ask me and yes, IE8 should get dropped anyways.

I do not understand why !important is required here. The selector specificity should be resolved in a way that does not require !important.

I'm aware that these lines are located in a theme. However, all core themes may be used as base themes, which is why they should not contain !important rules, just like module CSS should not contain them.

Fixed.

It doesn't look like this has been tested in RTL — the border-left is marked as LTR, but not overridden accordingly in the RTL styles.

Also, color me confused on the border-left: none in the first place — the li only defines a border-right but not a border-left.

Fixed.

fubhy’s picture

FileSize
37.22 KB

Whoops, forgot my RTL testing code in language.module by accident.

effulgentsia’s picture

Status: Needs review » Needs work
FileSize
14.44 KB

#68 looks really close to me.

Whenever borders were affected I changed that to :first-child instead but for paddings and such that doesn't really work

What makes it not work for padding? I especially don't understand this:

+++ b/core/themes/bartik/css/style.css
@@ -426,10 +421,10 @@ h1#site-name {
-.region-header #block-user-login .item-list li.last {
+.region-header #block-user-login .item-list li:last-child {
   padding-left: 0.5em; /* LTR */
 }

Above this, but not in the patch, is setting padding-left to 0 for all the other LIs. This seems needlessly hard-coded to assume just 2 links (which is correct for "create new account" and "request password" until someone hook_form_alter()s in some additional links). Wouldn't making the default have padding-left: 0.5em and the :first-child have padding-left: 0 make more sense in general, as well as be more IE8 friendly? I'm fine with that being a follow up though.

admin/modules seems messed up by the patch. In HEAD, all rows with module names in them have a beige color, and there's small white spacer rows between them, whereas with the patch, they alternate color (which I have no objection to), but the checkboxes are not lined up to the module name (which definitely looks wrong). Screenshot attached.

sun’s picture

Hm. I'm still concerned about hidden elements within a DOM collection of elements that should receive striping.

Draggable tables are just one of the possible incarnations in which that currently happens — obviously, a particular case can be changed and fixed, but I'm more concerned about use-cases in contributed and custom modules and themes, for which I could very well see cases, in which some really heavy refactoring would be in order - just to get a table row striping that doesn't look broken (which is a bit ridiculous, if you think about it).

As you might know, tables involve some heavier browser quirks with regard to DOM operations, so especially UI functionality that performs some heavier lifting (e.g., Ajax and other complex operations) sometimes has to leave elements in the DOM in order to work around browser quirks and they will have to implement quite massive workarounds for this. But actually, the functionality doesn't have to be super-special — for example, think of http://drupal.org/project/module_filter or http://drupal.org/project/instantfilter, which are purposively hiding table rows, and which definitely do not want to move the table rows outside of the DOM just to fix table row striping, since that would be horrible in terms of frontend performance — now, all of these cases would face a major problem.

I hope that clarifies where I'm coming from.

In addition, on the latest patch:

+++ b/core/misc/print.css
@@ -8,10 +8,10 @@ th {
-tr.odd {
+tr.zebra {
...
-tr.even {
+tr.zebra:nth-child(even) {

+++ b/core/modules/image/image.admin.css
@@ -57,8 +57,7 @@
-.image-anchor .even,
-.image-anchor .odd {
+.image-anchor .zebra {

+++ b/core/modules/update/update.css
@@ -30,8 +30,7 @@
-.update tr.even,
-.update tr.odd {
+.update tr.zebra {

+++ b/core/modules/user/user.css
@@ -4,8 +4,7 @@
-#permissions tr.odd .form-item,
-#permissions tr.even .form-item {
+#permissions tr.zebra .form-item {

+++ b/core/themes/bartik/css/style.css
@@ -936,20 +932,20 @@ ul.links {
-#footer-wrapper tr.odd {
+#footer-wrapper tr.zebra {
...
-#footer-wrapper tr.even {
+#footer-wrapper tr.zebra:nth-child(even) {

These selectors are still targeting table rows with class .zebra, but the class is actually added to the table.

For my last comment, I actually wanted to attach a patch that shows some (non-working) tricks/workarounds, and for which I also moved the .zebra class to the table rows instead of the table class, and also removed the .no-zebra class but forgot to do so

— I was also concerned about the possibility of nested tables, in which case the parent table might be zebra-striped, but the child table should most likely not be striped (since that looks odd). Off-hand, I don't think we have that case in core, but once you're into the table business, at least I had to do nested tables not only once in history.

I suspect that we could eliminate some duplicate styles by applying .zebra on the actual element that should get the striping instead of the parent container. (specifically thinking about non-table use-cases, too)

+++ b/core/themes/bartik/css/style.css
@@ -210,12 +207,10 @@ tr th {
+table.zebra tbody tr:nth-of-type(odd) {
+  background-color: #dddddd;
 }

Lastly, still connected to the .zebra class application, I wondered whether we couldn't simplify the CSS for all themes, by specifying this in system.theme.css instead:

.zebra:nth-of-type(even) {
  background-color: rgba(0, 0, 0, 0.02);
}

i.e., no .odd styles at all. Quite potentially eliminating the theme-specific styles in all core themes.

Perhaps it might be best to apply .zebra to both the table and the table rows? (eliminating .no-zebra)

nod_’s picture

Draggable tables are just one of the possible incarnations in which that currently happens — obviously, a particular case can be changed and fixed, but I'm more concerned about use-cases in contributed and custom modules and themes, for which I could very well see cases, in which some really heavy refactoring would be in order - just to get a table row striping that doesn't look broken (which is a bit ridiculous, if you think about it).

Curious about why there would be a need for a hidden element messing up the stripping. Do you have something particular in mind?

sun’s picture

Well, as mentioned, we don't even have to look very far for extreme cases - even the relatively trivial case of modulefilter/instantfilter is sufficient already:
http://drupalcode.org/project/module_filter.git/blob/refs/heads/7.x-2.x:...

In short, the need for hidden table rows is very common and appears in many flavors. We can change tableDrag and the Manage fields UI, but that does not help all of the other cases.

That said, by now, I'd be tempted to split off the removal/conversion of odd/even styling for item lists into a separate issue, since those changes are more straightforward and don't really involve any problems. — Although someone could reasonably make the very same arguments about hidden and dynamically exposed list items (via JS/Ajax) there, I think those cases can be addressed more easily by refactoring affected code, since item lists are simpler constructs (a $.detach() might even be sufficient already).

effulgentsia’s picture

In short, the need for hidden table rows is very common and appears in many flavors.

It ain't pretty, but I wonder if adding an extra hidden dummy row after every hidden non-dummy row is a good enough solution for such use cases.

nod_’s picture

So, module page, no alternate styling anymore, we can filter however we want.

Instantfilter, that's probably where we'd want to use several tbody, when search starts we add a tbody before the current one, hide the one with the results and duplicate the rows into the shown tbody.

the simplification with .zebra on elements sounds pretty good to me. It was on the parent to avoid redundant HTML markup at the expense of a more consuming CSS selector. I'm happy with the proposal in #70

If there is a issue with tables stripping in core, beside tabledrag, let me know I'll look into it. Views has a similar issue but it's using divs, not a table so it could be easier to deal with.

Kiphaas7’s picture

The most performant JavaScript filters (for really big tables) will probably need virtual scrolling anyway (if I ever get around to that for instant filter). Virtual scrolling does not use hidden table rows , so it can be worked around. That is just a very specific solution, module filter might not want to solve it like that.

However, how about going for default CSS only, with a possibility for a legacy fallback regarding php striping? Modules can then override this behavior when needed.

RobW’s picture

Why not add a .visible class at the same time the .hidden class is added by javascript? Because the classes would be added by js there would be no extra markup overhead over the wire, and something like .zebra tr.visible:nth-of-type(even) should work as expected.

The tbody tag in the middle of most of these rules could be taken out if we apply the .zebra class directly to it. It makes even more sense to abstract the rules out to pure utility classes separate from any element, in which case we would need to add a class directly to the element getting striped (which sun may have mentioned a couple posts above): .zebra.visible:nth-of-type(even).

If we want to address the nesting issue mentioned by sun, why not add a child selector? The element agnostic .zebra class would then need a container class: it could get cute and semantic by switching things around to .zebra > .stripe.visible:nth-of-type(even).

Fabianx’s picture

+1 for #76. Brilliant idea!

Wim Leers’s picture

At first sight, #76 seems feasible, but I'm no expert on this. I have to say, if this works then I totally agree with #77: brilliantly simple solution :)

fubhy’s picture

Not a bad idea indeed, however, nth-of-type is not supported by IE8.

sun’s picture

I might be missing something, but #76 seems to ignore the semantics of :nth-child() and :nth-of-type(). Quoting myself from #60, with added emphasis:

Not even the :not() selector helps, because 1) browsers internally optimize the selector and execute the :nth-child() first, and 2) :nth-child() applies to the actual DOM structure, not the CSS selector.

:nth-of-type() also applies to the DOM structure; the only difference is that it selects/filters by DOM element name instead of DOM node. None of both apply to the CSS selector.

nod_’s picture

Raised the issue in #13 too, that's the first thing I tried. That's why we never went with #76.

droplet’s picture

#80 +1

nth-of-type / nth-of child not working in that way (#76).

tr.visable <~~ tr.visable:nth-of-type(odd) / .visable:nth-of-type(odd) matched. TYPE: "TR"
tr.hide <~~ whatever it's hidden / diff class ..etc. It's SAME TYPE: "TR"
tr.visable <~ it always the 3rd.

tr.visible:nth-of-type(even) =/= .mutiple.classes =/= ( not work like .visable:hover )

casey’s picture

What about keeping the odd/even classes next to the zebra class, but only apply the former in complex situations.

RobW’s picture

Apologies to sun, _nod, and droplet; you're absolutely right. I assumed that "type" would be .visible, but if that had been true we would have been done with this issue long ago.

I think the html pattern .zebra > .stripe is still useful: it addresses sun's nested table concerns and the generalization and decoupling of .zebra-ing from specific element markup that's been mentioned a couple times above (and is just good front end practice).

The css level 4 selector :nth-match is what we're all waiting for, and it's in the realm of possibility that it could be in browsers within 2 years. I was hoping I could find a polyfill like there is for the !parent selector, but no luck. When that happens the hidden/visible fork would become useful.

Quick redux: the proposed solutions for fixing striping for hidden trs are a combination of

  1. add an empty extra tr with each hidden tr (#73)
  2. and _nod just fixes it :) (#62).
effulgentsia’s picture

What about keeping the odd/even classes next to the zebra class, but only apply the former in complex situations.

It's extra HTML bloat, extra PHP bloat, and requires keeping the restriping JS code and AJAX command that this patch otherwise allows to be removed. Since #62 indicates we'll stop having hidden table rows in core, then I don't think it should be core's job to keep all that stuff. Contrib modules that want to use hidden table rows can then choose whether to add extra ones per #73 or whether to add odd/even classes via JS or hook_preprocess_table().

sun’s picture

I think we should be very careful to not make the mistake of favoring modern feature/fanciness compliance over actual use-cases, with regard to the specific needs of the application and product that we are building.

The application/product's purpose is to solve use-cases, and in order to do so, we leverage modern technologies where possible. The application's needs dictate the technology. Not the other way around.

I already discussed this issue in some more depth with @fubhy and would recommend to spin off the first/last class part into a separate issue, since all of those changes are a no-brainer and can be committed very quickly.

The even/odd changes are the problematic part. As @RobW already mentioned in #84, :nth-match() is exactly the non-existing/vaporware CSS4 feature we need to perform this change in a sane way, as predicted in #60 already.

I'd recommend to wait for CSS4, or respectively, major browser support for :nth-match(). It does not make sense to me to introduce a regression of this extent, "just because."

joelpittet’s picture

I agree with #85, contrib modules that need to start hiding rows can use JS to re-stripe, as the currently do anyways. That would help remove the generation of the unnecessary set of CSS classes during build and save on HTML delivery of the markup. Currently the contrib modules need to do DOM manipulation for the hiding to work with stripes. This would be much the same but with less markup for the rest of everyone.

Seems like a step in the right direction even if not perfect for everybody.

joelpittet’s picture

And I agree with second last snippet in #70 is the way to go

table.zebra tbody tr:nth-of-type(odd) {
  background-color: #ddd;
}

or for good measure the same thing done differently:

table.zebra tbody tr:nth-of-type(2n+1) {
  background-color: rgba(0, 0, 0, 0.02);
}
Fabianx’s picture

#87 brings up a good point:

* Contrib always needed to re-stripe already.

What about we change the CSS to support both and remove the PHP / JS / etc. bloat:

table.zebra tbody tr:nth-of-type(odd) {
  background-color: #ddd;
}

becomes

table.zebra-js tbody tr.odd,
table.zebra tbody tr:nth-of-type(odd) {
  background-color: #ddd;
}

That way contrib can:

  1. Re-stripe when needed and add odd/even via JS like now and the CSS will magically work ( change .zebra to .zebra-js class on table)
  2. Use another method that naturally works with :nth-of-type - like we want in Core

I have just tested 1) manually and it works like a charm, with the .zebra on the actual tr element, the re-striping could also just remove the zebra element and we could have tr.zebra:nth-of-type(odd) and tr.odd as selectors.

I think that would suffice for the transition period and we can remove odd/even from CSS then in D9 and target CSS 4.

Thoughts?

jibran’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
37.46 KB

Latest reroll and some feedback from #69
#1800726: Convert theme_item_list to item-list.twig is postpone on this issue. So I am +1 for dropping tables for now and only do it for item-lists.

catch’s picture

Priority: Normal » Major

Bumping to major since this blocks Twig conversoin.

jenlampton’s picture

We'll keep adding @todos in the files about removing the even/odd striping. We'll either remove the stripes or the todos at somepoint :)

klonos’s picture

...coming from #1524414: Rewrite tabledrag.js to use jQuery UI.

I wasn't aware of this issue a couple of days ago when I filed #1942470: Zebra stripping gets messed up when rows are dynamically added/removed. precisely for the first issue mentioned in #30. Lets just leave it as a follow-up in case this issue here does not end up fixing that as well along the way.

PS: Adding it as a follow-up in the issue summary.

klonos’s picture

Issue summary: View changes

Updated issue summary.

sdboyer’s picture

@tim.plunkett just pointed me here. just wanted to say that this would be excellent from a SCOTCH perspective - any deferred rendering (ESI/hInclude) would inherently and inescapably break our current approach to zebra striping, as it is predicated on a static counter that only lives the length of the page request.

jwilson3’s picture

Status: Needs review » Needs work
+++ b/core/misc/print.cssundefined
@@ -8,10 +8,10 @@ th {
-tr.odd {
+tr.zebra {
   background-color: #ddd;
 }
-tr.even {
+tr.zebra:nth-child(even) {
   background-color: #fff;
 }

In browsers that dont support :nth-child, but that do support background image printing, this CSS snippet would have the unintended consequence that the entire table would be printed with light-gray background which is a waste of ink, and not a graceful degradation at all. This seems backwards, and the table background color should just degrade to white.

+table.zebra tbody tr:nth-of-type(odd) {
[...]
+tr.zebra:nth-child(even) {

Why is there inconsistent use of the 'zebra' class on the TR element in some places, but on the TABLE element in others? Putting zebra class on the TR means extra javascript logic if you're adding a table row dynamically, to retain the consistent striping behavior, so IMHO, this class should be placed once on the TABLE element only by PHP, and then in the case where table rows are shown/hidden, we should add additional DIFFERENT classes to those rows in Javascript, and use CSS specificity to override the default striping that would look screwed up by the hidden rows (or am I still missing something about this hidden feature from previous comments?)

+++ b/core/themes/bartik/css/style.css
@@ -40,9 +40,6 @@ p {
-tr.odd {
-  background: #e4e4e4;
-  background: rgba(0, 0, 0, 0.105);
+table.zebra tbody tr:nth-of-type(odd) {
+  background-color: #dddddd;
 }
-tr,
-tr.even {
+tr {
   background: #efefef;
   background: rgba(0, 0, 0, 0.063);
 }

This code seems to somehow conflict with the print styles, I haven't fired this up in a browser, but just from reading the code, it is not clear which rules would really have precedence here because of the excessive specificity (maybe its intended tho, in which case it should be commented). Maybe I'm just getting thrown off by .zebra tr and tr.zebra still :/

+table.zebra tbody tr:nth-of-type(odd) {

In addition to this being broad selector intent, it also has the unintended consequence of making nested tables display oddly, so maybe we could use '>' selectors to ensure nested tables don't do really strange things. Not 100% sure if this is a good idea.

hefox’s picture

Status: Needs work » Needs review
FileSize
35.8 KB

I think as far as inconstant tr.zebra and table.zebra, it was likely just not updated correctly with the switch to table.zebra, but I could be missing something also.

Anyhow, attempt to update patch to rework the tr.zebra to table.zebra

-tr.odd {
-  background: #e4e4e4;
-  background: rgba(0, 0, 0, 0.105);
+table.zebra tbody tr:nth-of-type(odd) {

This is causing more issues than the original, which was quite broad? Not sure what "broad selector intent" means exactly.

The print stuff seems like something needs to be manually tested.

Status: Needs review » Needs work

The last submitted patch, 1649780-96.patch, failed testing.

jide’s picture

For the record, I just stumbled upon this article http://abouthalf.com/development/poor-mans-nth-child-selector-for-ie-7-a....

Clever trick to be able to target nth children on IE 7 and 8 :

ul.menu > li:first-child + li + li {}

And would also be possible to target odd / even nodes :

ul.menu > li {}
ul.menu > li + li {}
kristiaanvandeneynde’s picture

@jide #98: I think the whole point is that we no longer want IE7/8 'hacks' in our CSS?

jide’s picture

@kristiaanvandeneynde : Hmmm no think the point was to remove extra markup needed to work around IE's lack of support for pseudo selectors.

It was said that legacy IE not supporting zebra was an acceptable downside, but this trick could make IE happy with zebra again, for almost no extra work and with valid-hack-free CSS, so I thought it might be considered :)

kristiaanvandeneynde’s picture

Hmm, I'll leave that point for others to comment on then.

On topic:

ul.menu > li {} /* targets every list item */
ul.menu > li + li {} /* targets every list item except the very first */
jide’s picture

Hum, I've spoken too fast it seems, because you are right, that selector won't target odd / even... Silly me.

It would be possible to cover a certain number of cases using rules like :

			table tr {
				background: red;
			}
			table tr:first-child + tr,
			table tr:first-child + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr,
			table tr:first-child + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr + tr {
				background: blue;
			}

...But it would work only for nth first children, and i guess so much overhead in CSS is not acceptable.

But hey, whenever targeting a particular nth child, the trick is useful anyway.

tkoleary’s picture

So I think this whole discussion can be resolved if we simply follow the recommendation of the new seven style guide http://groups.drupal.org/node/283223 and use border bottom, not background to delineate rows. This solves the problem of a hidden row causing two adjacent rows to have the same background, as well as the issues of background zebra striping not passing WCAG and/or not being visible with windows gamma. It's also simpler and less visually noisy.

a.ross’s picture

To clarify tkoleary's statement, this is the section of the style guide he refers to: http://groups.drupal.org/node/283223#Table

I like the proposal outlined there, however, the API shouldn't force that upon contrib. The striping option could default to disabled to be inline with the proposal, and the option can even be deprecated, but I'm against removing it altogether at this point. By the way, a (more general) API change of the theme_table function is being discussed here: #935066: theme_table() should not output the no_striping option as an html attribute

BarisW’s picture

I would turn this the other way around, so that browsers that don't support nth-of-type() show white tables instead of grey.
Also, we might add borders on the bottom instead, as fallback for IE, like this:

table.zebra tbody tr {
  background: #fff;
  border-color: #bebfb9;
  border-style: solid;
  border-width: 0 1px 1px;
 }
table.zebra tbody tr:nth-of-type(odd) {
   background: #f3f4ee;
   border-bottom: none;
}
effulgentsia’s picture

Per #72, I think this issue will end up requiring getting split into more manageable chunks. Here's the first one that's both small and would immediately help the Blocks and Layouts initiative: #1968322: Remove unused $id and $zebra variables from templates.

effulgentsia’s picture

Since there are a lot of themers who care about semantic purity and modern techniques following this issue, shameless plug for #1968360: Remove per-region block markup, which isn't strictly related to this issue, but touches on similar questions of what's needed in markup vs. css.

sun’s picture

I'm relatively sure that @sdboyer's remark in #94 was focused on #1968322: Remove unused $id and $zebra variables from templates only. That issue is RTBC already.

With regard to the main issue, odd/even classes on table rows, #84 + #86 still apply.

I'd actually recommend to stop wasting time on this issue for now and to bump it to D9 instead. In 3+ years from now, we might have the pleasure to have :nth-match at our hands. That's the only way to reliably deal with this situation without .odd/.even classes.

effulgentsia’s picture

I'd actually recommend to stop wasting time on this issue for now and to bump it to D9 instead.

In #72, you were supportive of removing these classes from everything but tables (e.g., item lists) in D8. Is that still the case? If so, let's at least keep this issue for that, or open a spin off to deal with them.

Fabianx’s picture

I still think #89 is a viable solution. You need to re-stripe via JS regardless of solution used - already.

To summarize again:

* Keep the .odd, .even CSS for now.
* Add an optional JS to re-add the odd, even classes via JS or to re-stripe via JS - this could also live in contrib.

That way we:

* Remove all HTML bloat.
* Keep full compatibility.
* Need to keep a little more legacy CSS around. But compared to keeping _all_ CSS legacy around, this is actually a good thing.

Any takers?

webchick’s picture

Issue tags: +Twig

This is listed as a blocker for Twig conversions, so tagging.

steinmb’s picture

+1 from me that odd/even classes have a legacy support in a contrib project, and that D8 is cleaned up and only use pseudo selectors. Also bumping the thread :)

jwilson3’s picture

This thread is getting pretty long and is aging, and it would be really helpful for someone to parse out all the knowledge and debate here and consolidate the current status into the issue summary.

Also, IE8 is mentioned 25 times on this thread (now 26 ;) but it hasnt been clearly stated anywhere on this thread that we now have https://drupal.org/project/ie8 to deal with Internet Explorer legacy support for Drupal 8.

Re #112, my current understanding (which could be wrong) is that we can't necessarily get rid of odd & even classes yet, because some core tables that use zebra striping have various special cases such as hidden rows and drag-and-drop row reorderability, which a) throw off the functionality of using native selectors like :nth-child(odd) and b) require that we keep around the logic for re-striping the rows after reordering, respectively.

droplet’s picture

This isn't an IE issue. Please don't quote IE anymore. It needed CSS4 or JS to provide equally feature here.

Remaining problem:
#1 Remove ZEBRA feature or not ?
If YES, commit above patches, all done

If No:
#2. Keep as it or clean up PHP & CSS and then using JS ?

If KEEP IT, Close issue.

IF Clean up, let start again.

droplet’s picture

add back

miro_dietiker’s picture

Summary: Trying to identify the currently relevant lines/cases that we are talking about and need to cover in core:

CSS .odd class usage:

$ grep -Rn "\\.odd" . | grep css | wc -l
22 (dblog, image anchor, simpletest results, diff, forms, update modules, user permissions, bartik [table, sidebar, footer], seven [table, modules, translation status])

CSS nth-child usage:

$ grep -Rn "nth-child" . | grep css | grep -v "\\.min\\.css" | wc -l
4 (system tatus report, menu-links, sidebar block)

CSS .first usage:

$ grep -R "\\.first" . | grep "\\.css" | grep -v "\\.min\\.js" | wc -l
4 ()

CSS .last usage:

$ grep -R "\\.last" . | grep "\\.css" | grep -v "\\.min\\.js" | wc -l
11 ()

JS nth-child usage:

$ grep -Rn "nth-child" . | grep js | grep -v "jquery\\.js" | wc -l
3 (tabledrag.js)

Guess that's it.

jwilson3’s picture

Issue tags: +styleguide

tagging

jwilson3’s picture

Issue summary: View changes

...adding #1942470 as a follow-up.

klonos’s picture

...moving related/parent issues to their dedicated metadata section. Done the same for the follow-up (actually added this issue here as a parent to it because currently we cannot add children/follow-up issues from their parent. See: #2130889: Allow adding child/follow-up issues directly from the parent issue and converting related issues to children.)

Wim Leers’s picture

Can we get this moving forward again?

However, the :(first|last|nth)-child selectors don't work in IE8. Is it okay for core to ship without table/list striping on IE8?

This is no longer a concern, since IE8 is no longer supported by Drupal 8. So, yes, it is okay for core to ship without table/list striping on IE8.

sun’s picture

#108 still applies, I think. In order to deal with hidden table rows, we need CSS4 :nth-match(), which is not available in all browsers yet.

However, I guess it would make sense to create a separate issue for removing the odd/even classes from lists, links, etc — quite potentially everything, except tables.

Wim Leers’s picture

#120: Yes, removing odd/even from everything except tables would be great!!

I promise fast reviews to whomever wants to work on this :)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
6.99 KB

Not sure if this gets them all or too much or not enough but here's a patch. Didn't take the table zebra stuff out but everything else I tried to take out. Not totally sure on the views UI removal?

Status: Needs review » Needs work

The last submitted patch, 122: 1649780-122-remove-zebra.patch, failed testing.

The last submitted patch, 122: 1649780-122-remove-zebra.patch, failed testing.

miro_dietiker’s picture

These failing tests need fixing as they test for "odd" classes.
DisplayBlockTest.php line 275
ContextualDynamicContextTest.php line 81, 83
MenuTest.php line 421
DisplayTest.php line 316

For FunctionsTest.php dunno as the message like "theme_item_list() rendered correctly." is unclear. But it looks like a newly introduced bug.

Strange that descriptive output of assertThemeOutput relies on verbose flag enabled. I would suggest to change this output.

Finally dunno about the TourTest.php fails and didn't look into them.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
22.4 KB
16.53 KB

Thanks for getting this going again! Unfortunately, it was not yet complete enough to be able to do a proper review:

  • it also just got rid of the existing even/odd-based CSS, instead of updating it… so I fixed that too.
  • it did not yet remove the first and last classes; this reroll does, and updates the affected CSS.
  • it did not at all update the tests; this reroll should fix all test failures.
  • … and a bunch more things.

This should get us closer, but is not at all good to go yet.

Wim Leers’s picture

I forgot two hunks.

Wim Leers’s picture

I forgot to add: I manually tested in both Chrome and Firefox.

The last submitted patch, 126: odd_even_first_last-1649780-126.patch, failed testing.

The last submitted patch, 127: odd_even_first_last-1649780-127.patch, failed testing.

Wim Leers’s picture

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

This is the technically feasible extent and compromise that is possible for D8. Let's cross our fingers and hope we can rely on CSS4 in D9 to remove the remaining stuff for tables :-)

catch’s picture

The last submitted patch, 131: odd_even_first_last-1649780-131.patch, failed testing.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Strange that the bot didn't change status.

kaare’s picture

Status: Needs work » Needs review
FileSize
23.72 KB

Re-roll

kaare’s picture

The failed hunks in core/includes/theme.inc and core/modules/system/lib/Drupal/system/Tests/Theme/FunctionsTest.php was all caused by #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links.

Status: Needs review » Needs work

The last submitted patch, 136: odd_even_first_last-1649780-136.patch, failed testing.

klonos’s picture

Wim Leers’s picture

I'll reroll this soon. If somebody beats me to it, that's fine too :)

star-szr’s picture

Wim Leers’s picture

Thanks :) Looks good!

The last submitted patch, 141: 1649780-141.patch, failed testing.

webchick’s picture

Status: Needs review » Needs work

...

star-szr’s picture

141: 1649780-141.patch queued for re-testing.

The last submitted patch, 141: 1649780-141.patch, failed testing.

star-szr’s picture

141: 1649780-141.patch queued for re-testing.

The last submitted patch, 141: 1649780-141.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
FileSize
25.03 KB
sun’s picture

Status: Needs review » Reviewed & tested by the community

Please move back directly to RTBC the next time this patch passes tests.

catch’s picture

Title: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors » Change notice: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x, thanks!

Needs a change notice.

star-szr’s picture

Issue tags: +DrupalWorkParty
jgSnell’s picture

Assigned: Unassigned » jgSnell

Working on the change record today!

jgSnell’s picture

Status: Active » Needs review

Proposed change record follows.

Topic
Core non-table HTML entities now use CSS pseudo selectors instead of first/last/odd/even classes

Description
Summary

  • .even, .odd, .last, .first have been removed from core-generated HTML entities except tables
  • CSS has been updated to use comparable CSS3 pseudo selectors in place of the removed class selectors

Reasoning

  • Better compliance with CSS3 best practices
  • Removes some markup bloat
  • Removes PHP logic from theme functions and templates
  • Old CSS selectors remain for tables due to issues caused by hidden table rows
SebCorbin’s picture

Status: Needs review » Needs work

An replacement example for each of ".even, .odd, .last, .first" would be nice for those that are not accustomed to CSS3

jgSnell’s picture

Status: Needs work » Needs review

Good point! Here's an example to include in the change record in #154

Before

//Example list output by theme_item_list function
<ul>
  <li class="first odd">Item 1</li>
  <li class="even">Item 2</li>
  <li class="odd">Item 3</li>
  <li class="last even">Item 4</li>
</ul>
//CS to style the first, last, even, and odd elements uses class selectors
li.first { . . . }
li.last { . . . }
li.even { . . . }
li.odd { . . . }

------

After

//Example list output by theme_item_list function
<ul>
  <li>Item 1</li>
  <li>Item 2</li>
  <li>Item 3</li>
  <li>Item 4</li>
</ul>
//CSS to style the first, last, even, and odd elements uses CSS3 pseudo selectors
li:first-child { . . . }
li:last-child { . . . }
li:nth-of-type(even) { . . . }
li:nth-of-type(odd) { . . . }
SebCorbin’s picture

#154 + #156 combined are ok with me!

star-szr’s picture

Thanks for your work on this @jgSnell!

I think the change record title should be shorter (and more celebratory!), something along the lines of:

Most first/last/odd/even classes removed in favor of CSS3 pseudo selectors

We can explain in the body that tables still have these classes (as you already are). HTML entities are a different thing altogether so we shouldn't be referencing them in the change record.

For the examples I think the // style comments in there are confusing (one is HTML and one is CSS and neither allows comments like that), I would put the explanations above the <code> tags and end them with a colon. The before example needs CS changed to CSS and I think they both need 'uses' changed to 'using'.

One last nitpick for the road: we should say theme_item_list() when referring to the PHP function.

SebCorbin’s picture

OK, here is a wrap-up, if nobody sees any mistake, I'll create the node.

Most first/last/odd/even classes removed in favor of CSS3 pseudo selectors

Summary

  • .even, .odd, .last, .first have been removed from core-generated HTML entities except tables
  • CSS has been updated to use comparable CSS3 pseudo selectors in place of the removed class selectors

Reasoning

  • Better compliance with CSS3 best practices
  • Removes some markup bloat
  • Removes PHP logic from theme functions and templates
  • Old CSS selectors remain for tables due to issues caused by hidden table rows

Before

Example list output by theme_item_list() in Drupal 7

<ul>
  <li class="first odd">Item 1</li>
  <li class="even">Item 2</li>
  <li class="odd">Item 3</li>
  <li class="last even">Item 4</li>
</ul>

CSS to style the first, last, even, and odd elements using class selectors

li.first { . . . }
li.last { . . . }
li.even { . . . }
li.odd { . . . }

After

Example list output by theme_item_list() in Drupal 8

<ul>
  <li>Item 1</li>
  <li>Item 2</li>
  <li>Item 3</li>
  <li>Item 4</li>
</ul>

CSS to style the first, last, even, and odd elements using CSS3 pseudo selectors

li:first-child { . . . }
li:last-child { . . . }
li:nth-of-type(even) { . . . }
li:nth-of-type(odd) { . . . }
kristiaanvandeneynde’s picture

Looks good :)

star-szr’s picture

Yup looks pretty great, thanks again @jgSnell :)

I think this:

.even, .odd, .last, .first have been removed from core-generated HTML entities except tables

Since it's technically talking about markup shouldn't use dots and again shouldn't mention HTML entities. I suggest something like this:

'even', 'odd', 'last', and 'first' classes have been removed from most of the markup in core except tables.

Ship it!

SebCorbin’s picture

Title: Change notice: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors » Remove first/last/odd/even classes in favor of CSS3 pseudo selectors
Assigned: jgSnell » Unassigned
Status: Needs review » Fixed
Issue tags: -Needs change record

Change record created: https://drupal.org/node/2178215

Again, thanks a lot to you jgSnell!

jibran’s picture

Thanks for the great change notice @jgSnell and @SebCorbin.

star-szr’s picture

Oops. Thanks @SebCorbin I hadn't yet woken up when I commented I guess :) thanks to you and @jgSnell and everyone who worked on this issue!. I'm really happy this issue is fixed!

jgSnell’s picture

Awesome! :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.