Wrap the plain breadcrumb in a span so themes can work properly and matches core approach

[herbdool]

When clicking on a facet it will add the filter name to the end of the breadcrumb. Currently it looks like this with core's Basis theme:

Core will wrap a plain breadcrumb (no link) in a <span>. But facetapi will leave it plain. But this should probably be wrapped in a <span> so themes can target it properly so it looks like this:

[argiepiano]

Even after applying the PR, when you do a search by filling in the search box instead of clicking a facet, and there is only one item, the last item is still not wrapped in <span>. The PR only works for searches done by clicking facets. Line 110 also needs to be patched as follows:

$breadcrumb[] = $active_items ? l($keys, current_path(), array('query' => $query))  :  '<span>' .  check_plain($keys) . '</span>';

After applying PR:

[argiepiano]

This fix, btw, is only needed by Basis. Seven and Bartik both shows the last child correctly. The problem is that Basis doesn't provide the full needed css styling to the <li> that wraps each option. The top padding is only provided for the <a> element inside the <li>. So, I'm not so sure anymore if this is the right approach. We are changing the markup (adding a span inside the li) in ways that may affect contrib themes? I haven't checked this against other contrib themes.

[herbdool]

If that's so then that's also true of theme_breadcrumb() since the Basis override is the same except without a ».

[herbdool]

You may be right that it's up to Basis to fix this. Though I think you've got it backwards. Basis provides styling to li a and li span so I think it might be up to Basis to wrap a breadcrumb in a span if it's not a link.

[argiepiano]

What I meant is that:

• None of Basis, Bartik and Seven wrap the unlinked last child of a breadcrumb with <span>. The last child is <li>Text</li> in all three
• This unlinked last child looks fine in Seven and Bartik
• But, this unlinked last child doesn't look good in Basis

So, rather than wrapping the last child as in <li><span>Text</span></li> (which may mess up other themes, since it seems like the common practice is to wrap it just like <li>Text</li>), I was suggesting that the CSS in Basis for .breadcrumb li {} should be modified to make the plain, unlinked text look good, like in the other themes, without the need of wrapping it in span.

Then again, perhaps wrapping the text in span here, like you do in the PR, is harmless and will not mess up anything (and will add the benefit making it look good in Basis). I haven't tested this.

I guess another option is to add a configuration form where the user can specify the markup to wrap the unlinked last breadcrumb. But this may be overkill if the above is harmless.

[argiepiano]

BTW, did you see my comment aboe? If we' be decide to wrap that last breadcrumb in span, there is one more place where that needs to be done.

[herbdool]

Yes, I saw that, though I now am thinking of doing something like this, after you mentioned it is only needed for Basis:

function mysubtheme_breadcrumb($variables) {
  $breadcrumb = $variables['breadcrumb'];
  $output = '';
  if (!empty($breadcrumb)) {
    array_walk($breadcrumb, function(&$b) {
      $b = (substr($b, 0, 2) === '<a') ? $b : '<span>' . $b . '</span>';
    });
    $output .= '<nav class="breadcrumb" aria-label="' . t('Website Orientation') . '">';
    $output .= '<ol><li>' . implode('</li><li>', $breadcrumb) . '</li></ol>';
    $output .= '</nav>';
  }
  return $output;
}

I'll put it in my child theme, but it could also go into Basis. That way it leaves other themes and modules untouched.

[herbdool]

Actually, now I've changed my mind. In core there is this:

function system_breadcrumb_block($settings = array()) {
  $crumbs = backdrop_get_breadcrumb();
  if (isset($settings['current']) && $settings['current'] == TRUE) {
    $crumbs[] = '<span aria-current="page">' . backdrop_get_title() . '</span>';
  }

  return theme('breadcrumb', array('breadcrumb' => $crumbs));
}

So this is getting output regardless of theme. So I think we can do the same here too. I just need to update my PR to account for both cases you mention @argiepiano

[herbdool]

@argiepiano I've updated the PR to also add a <span> around the current search. And I added aria-current="location" to make it more accessible. (Core uses aria-current="page" when adding the page title, but the search result doesn't seem like a page. And I thought it would be confusing if both were added - though that has its own problems which can be addressed elsewhere).

[argiepiano]

In core there is this:
function system_breadcrumb_block($settings = array()) {

Interesting! I knew I had seen this ability to enable/disable showing the current location somewhere!

So, now we have an interesting problem. When I enable that functionality in the breadcrumb block, we end up with two breadcrumbs that are not links - this may be confusing, but I guess this is beyond the scope of this issue.

I'll do more testing later, but so far WFM

[herbdool]

Yeah, that's what I was alluding too. Beyond the scope of this issue. (Maybe Facet shouldn't be putting them in the breadcrumb at all?)

[argiepiano]

(Maybe Facet shouldn't be putting them in the breadcrumb at all?)

I really like the way Facet API adds those breadcrumbs. It allows you to "undo" the facetted search one term at a time. I also think that site builders can disable the last breadcrumb added by the breadcrumb block. And if they want to get fancy, they can add two breadcrumb blocks, one with the last breadcrumb and another without, and use a URL path visibility condition to use the disabled one in the search pages.

So... this LGTM and RTBC!