Skip to:
Content

bbPress.org

#3217 closed defect (fixed)

Invalid HTML generated for forums list

Reported by: scmsteve Owned by: johnjamesjacoby
Milestone: 2.6 Priority: normal
Severity: normal Version:
Component: Appearance - Theme Compatibility Keywords: commit
Cc:

Description (last modified by netweb)

Since commit [6628] the forums list is generating invalid HTML. The list is in a UL with each forum being a LI. However, the change was made and it is generating the comma separator outside the LI tag which is not allowed in the HTML spec.

A tentative fix might be as follows, it seems to work for us here, but however the devs handle it is fine. :) I left the old code in place as a reference.

includes/forums/template.php

<?php
 758                 // Build this sub forums link
 759                 // $links[] = $r['link_before'] . '<a href="' . esc_url( $permalink ) . '" ' . $subforum_classes_attr . '>' . $title . $counts . '</a>' . $r['link_after'];
 760                 $links[] = $r['link_before'] . '<a href="' . esc_url( $permalink ) . '" ' . $subforum_classes_attr . '>' . $title . $counts . '</a>';
 761         }
 762 
 763         // Maybe wrap output
 764         $output = ! empty( $links )
 765                 ? $r['before'] . implode( $r['sep'] . $r['link_after'], $links ) . $r['link_after'] . $r['after']
 766                 // ? $r['before'] . implode( $r['sep'], $links ) . $r['after']
 767                 : '';

Change History (15)

#1 @johnjamesjacoby
13 months ago

  • Component changed from General to Appearance - Theme Compatibility
  • Keywords commit added
  • Milestone changed from Awaiting Review to 2.6
  • Owner set to johnjamesjacoby

#2 @johnjamesjacoby
13 months ago

In 6860:

Forums: revert default mark-up back to div's and span's in bbp_list_forums().

This fixes potentially invalid mark-up from attempting to use list elements instead of spans.

Also includes changes to CSS selectors to make them more flexible with different child elements.

See #3217.

#3 @johnjamesjacoby
13 months ago

  • Resolution set to fixed
  • Status changed from new to closed

This is fixed, though admittedly by way of defeat.

I'd hoped we could use an unordered list for these, but that's a bit too specific for the generally flexible nature of theme compatibility.

Thanks @scmsteve!

#4 @scmsteve
13 months ago

Hi, but in my way of thinking this does two things:

  • It breaks previous styling from 2.5 where they were UL/LI
  • The comma is still outside of the scope of the forum name/link

If we are styling how the forum title is displayed it makes sense we would want that comma styled in the same way. But with both versions of the 2.6 change, the original and your fix, you have moved the comma outside of the li or span tag, so it won't be styled the same.

Talking it over here, the thought came out that maybe if you just removed the comma entirely and did it in CSS, but that also would break existing themes.

Can you share your thinking of why you are wanting to change the 2.5 behavior and tke the comma outside of the element that encloses the link?

#5 @douglsmith
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If it's just trying to get the comma out of the list item for semantics then the comma is sort of just for display and might actually belong in the css. Something like this should do it

.bbp-forums-list > .bbp-forum:not(:last-child):after {
    content: ", "
}

That would be conducive to other styling options that might not want a comma. The only breakage from 2.5 should be that commas would go missing on sites with themes that completely suppress the supplied css.

Last edited 13 months ago by douglsmith (previous) (diff)

#6 @johnjamesjacoby
13 months ago

@douglsmith is correct, that the sep is largely just for display, and we won't want the sep inside the list-item - we want it in-between. It's literally a way to get in-between them, so folks could set sep to <li class="sep">&rarr;</li> if they wanted to. In 2.5, that wasn't possible, and was mistakenly broken.

That also means we can't move it to CSS, because folks using the sep value (like above) would end up with both a comma and a right arrow.

The fix as committed is the best compromise I can see. Open to more suggestions though.

#7 @douglsmith
13 months ago

That also means we can't move it to CSS, because folks using the sep value (like above) would end up with both a comma and a right arrow.

I must be missing something. How would they be setting that now?

#8 @johnjamesjacoby
13 months ago

I must be missing something. How would they be setting that now?

A template-part override in a plugin or theme (passing in their own array( 'sep' => '&rarr;' )), or via the bbp_before_list_forums_parse_args or bbp_after_list_forums_parse_args filters where the args can get overridden.

#9 @netweb
13 months ago

  • Description modified (diff)

#10 @scmsteve
12 months ago

My concern about this change is the note "revert default mark-up back to div's and span's", whereas in the version 2.5 that we were running, they weren't div and span tags, but where ul and li tags. The issue is that before, the comma was inside the li and the change put it outside the li. But the change was not making div and span into ul and li. This change broke our display and our css, as the selectors are all different, and the div/span combo does not display like a ul/li combo does. So, when was this done with div and span, because it seems it was before the 2.6 changes. EDIT: I just looked, it was UL/LI in 2.0 so it has been a long time. I expect, if this was customized by many, or even not, there will be a lot of site design breakage by moving to DIV/SPAN here.

Last edited 12 months ago by scmsteve (previous) (diff)

#11 @johnjamesjacoby
11 months ago

@scmsteve what if we revert back to ul/li, and change the sep to also be an li element that wraps a comma?

#12 @douglsmith
11 months ago

Any change to the markup is likely to break display on some sites. Would it be any more breakage to eliminate sep entirely in favor of a list that behaves as expected and can have CSS separators? Maybe that along with a statement in the upgrade notes or a warning on upgrade if a non-default sep is defined?

I know that takes away the ability to change the separator with a filter and I don't have a clue how important that is to the larger community. :-)

#13 @johnjamesjacoby
11 months ago

You’re probably right @douglsmith. At this point, re’reverting and adding CSS seps is probably the best option we have?

#14 @johnjamesjacoby
11 months ago

In 6872:

Forums: revert part of r6860.

Go back to ul and li to avoid breaking CSS for existing installs, and use a CSS separator instead to address the original invalid markup issues.

This might show an extra separator in circumstances where filters or template-overrides are targeting very specific things, but that's better than breaking mark-up changes.

See #3217.

#15 @johnjamesjacoby
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

I believe this is as fixed as it can be, so let's close this to clean up the milestone.

If I'm mistaken, please feel free to reopen. Thanks everyone!

Note: See TracTickets for help on using tickets.