Problem/Motivation

The widget "Links with checkboxes" displays differently depending on whether it's inactive or active:

  • Inactive facets are displayed as an unchecked checkbox followed by a linked label.
  • Active facets are displayed as checked checkboxes followed by an unlinked label.

There should be an option to make active facets displayed as checked checkboxes followed by a linked label. This will make the two displays more consistent.

Proposed resolution

Add an option to the "Links with checkboxes" widget settings to display active facets as checked checkboxes followed by a linked label.

The default for this option should be to do what it did before (i.e.: display active facets as checked checkboxes followed by an unlinked label). This will prevent it from breaking existing settings.

Remaining tasks

  1. Write patch
  2. Module maintainer should approve small JavaScript API change (see below and comment #33) or move issue to 7.x-2.x.
  3. Review and RTBC
  4. Commit.

User interface changes

A checkbox with the title "Show the labels of active checkboxes" and the description "Displays an active item as a checkbox followed by a link." is added to the "Display settings" fieldset of the "Configure facet display" page when "Display widget" is set to "Links with checkboxes".

When this setting is set, active facets are displayed as checked checkboxes followed by a linked label.

API changes

Before this patch, Drupal.facetapi.makeCheckbox() required the HTML element to manipulate (the facet link) to be the calling scope (this).

After this patch, Drupal.facetapi.makeCheckbox() requires the HTML element to manipulate to be passed as the first parameter. Existing code can be easily changed by calling Drupal.facetapi.makeCheckbox(this).

I don't anticipate this JavaScript-API change to be a problem, because, as far as I know, the only JavaScript that calls Drupal.facetapi.makeCheckbox() is inside facetapi.js, and I've fixed that. I also searched as many other modules as I could think of that depended on FacetAPI (current_search, search_api_facetapi, date_facets, facetapi_bonus, facetapi_i18n, wetkit_search, wetkit_apachesolr, and facetapi_select) — none of these modules call this function.

Original report by @Jon Pugh

A client has requested to make an active facet text a link, just like the inactive facets.

I am not a usability expert, but I think this makes sense. Whether it is checkboxes or the (-) link , it is pretty small and a pain to click, which is frustrating because you likely got there by clicking the word. My intuition tells me to click the word again to "uncheck".

I do think there should be a style change for that link (There is already a class), as making the facet text a link would make it harder to see.

Thoughts? We are going to start with a patch. We will post it here.

I do understand this will likely be responded to with "Make a FacetApiWidget", but I wanted to open the conversation to usability folks since this really seems like a good change to make.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cpliakas’s picture

Category: feature » support

Hi careernerd.

A widget is probably the most repeatable approach, although it is not necessary. You can simply override theme_facetapi_link_inactive() and theme_facetapi_link_active() in your theme's template.php file to have control over display of these links.

At this point in the game I don't have any interest in modifying the core widgets since we are so close to a 1.0, however I would love to discuss a strategy for default widgets with better UX going forward.

Thanks,
Chris

fastangel’s picture

But the problem is:

    if (active) {
      // Add the checkbox, hide the link.
      $link.before(checkbox).hide();
    }
.......

in facetapi.js

So I think that you could not make such changes to themes functions. It would not be correct to add a setting to make this work?

Regards.

fastangel’s picture

Status: Active » Needs review
FileSize
2.28 KB

I attached a patch for push the active link in configuration.

cpliakas’s picture

Category: support » feature

Changing to a feature request.

cpliakas’s picture

Status: Needs review » Needs work

Marking as needs work. The patch is made against Drupal's document root as opposed to the module's directory, it looks like there is a double ",," after an aray item, and there is also a fatal error in the patch due to a missing parenthesis after the drupal_add_js() function.

cpliakas’s picture

Fixed the issues noted above, keeping the status as "needs work" since there are a ton of "Warning: Illegal offset type in drupal_add_js() (line 4085 of /Users/cpliakas/Sites/facetapi/includes/common.inc)." errors and the setting doesn't seem to make the text a part of the link.

cpliakas’s picture

Fixed the issues noted above, keeping the status as "needs work" since there are a ton of "Warning: Illegal offset type in drupal_add_js() (line 4085 of /Users/cpliakas/Sites/facetapi/includes/common.inc)." errors and the setting doesn't seem to make the text a part of the link.

cpliakas’s picture

Sorry for the dup.

tecjam’s picture

I've been trying to get this to work also, but so far no luck.

I have rewritten the theme_facetapi_deactivate_widget($variables) {} function in my theme template.php file to return an image (a red x) rather than the standard (-), but I also wish to make the whole string the link, not just the image.

Looking forward to a fix for this.

ianthomas_uk’s picture

How about just using label?

<input type="checkbox" id="foo"/> <label for="foo">Facet label</label>

This is good practise for accessibility, and is less likely to affect the styling. You can use full tags for the actual link too, or just use <a for="foo">

cpliakas’s picture

Hi guys.

Patches are very welcome for this. Given other priorities I don't see myself getting to this any time soon, so if you are just waiting and not contributing then the wait will be pretty long!

Thanks,
Chris

David_Rothstein’s picture

Just a quick note that I have a patch at #1809030: Facet checkbox links don't have labels which might help a bit with #10. It doesn't implement that exactly at the moment, but it may be the direction the issue eventually heads (and in any case the patch there should be a good start).

Jon Pugh’s picture

I totally agree with #10 #11 and #12!

I hadn't even thought about the label tag... those are clickable, too! Shall we abandon this option to use #1809030: Facet checkbox links don't have labels?

Rob230’s picture

I have clients that want this. For usability, accessibility and just following web guidelines, the checkboxes should use labels, which would mean the invisible text is not even necessary for screen readers. Additionally, having to click on a checkbox for an active facet is more awkward. Even if it's not changed to labels, I think the deactivate widget should not be used for checkboxes, and the link shouldn't be hidden.

Unfortunately I found it was very difficult to override this for my site because as has been said, the link gets removed by JavaScript. So even when I override theme_facetapi_link_active to not use theme_facetapi_deactivate_widget or theme_facetapi_accessible_markup, I end up with no text or link.

I don't have time to investigate doing this myself at the moment.

ace11’s picture

Need solution for this too. Pretty odd that text does not have link.

mpp’s picture

Why not simply altering the theme function for the current search keys?

You'll need to add in the path to the theme variables. Something like this:

/**
* Returns HTML for a search keys facet item.
*
* @param $variables
* An associative array containing the keys 'keys' and 'adapter'.
*
* @ingroup themeable
*/
function theme_current_search_keys($variables) {
return theme('facetapi_link_active', array(
'text' => check_plain($vars['keys']),
'path' => $vars['path'],
'options' => array(
'attributes' => array(),
'html' => TRUE,
'query' => drupal_get_query_parameters(),
),
));
}

derekwebb1’s picture

This is still an issue. Even if you do make the text a link this js:

if (active) {
// Add the checkbox and label, hide the link.
$link.before(label).before(checkbox).hide();
}

Will hide it... and leave me with not one but two checkboxes!

So unless I write some js to unhide my added linked text I am apparently out of luck. I can't understand why the linked part of an active facet is made to be un-clickably small (-).

EDIT:

I got around this issue by adding a theme_facetapi_link_active function that adds a classed span around the text part of the active facetlink like so:

return theme_link($variables) . '<span class="linktxt">'.$link_text.'</span>';

Then in a js file that runs after facetapi I update the link text to have an actual link inside it. This is a tidbit from the search related js file that creates the link:

$active.each(function() {
  var $linkParent = $(this).parent('li');
  var $activeLink = $linkParent.find('.lnktxt');
  var $resetLink = $linkParent.find('a');
  var href = $resetLink.attr('href');
  $activeLink.html('<a href="'+href+'" rel="nofollow">'+$activeLink.text()+'</a>');
});
mparker17’s picture

Issue summary: View changes
FileSize
2.38 KB

Patch in #7 didn't apply anymore, so I re-rolled it.

There was a merge conflict in facetapi.js, I resolved it to...

    if (active && !Drupal.facetapi.active_link) {
      // Add the checkbox and label, hide the link.
      $link.before(label).before(checkbox).hide();

... as it looked like the if() statement was the part that this issue is trying to change, not the hide() statement.

mparker17’s picture

FileSize
2.38 KB
689 bytes

Hmm, a misplaced closing parenthesis in the call to drupal_add_js() in plugins/facetapi/widget_links.inc causes Warning: Illegal offset type in drupal_add_js() (line 4200 of includes/common.inc). errors. The attached patch should fix this.

mparker17’s picture

Status: Needs work » Needs review

Actually, this addresses the problem @cpliakas pointed out in #6 so I'm marking this as "Needs review" again.

Status: Needs review » Needs work

The last submitted patch, 19: active-link-option-1526020-19.patch, failed testing.

mparker17’s picture

Version: 7.x-1.0-rc4 » 7.x-1.x-dev
Status: Needs work » Needs review

Ah, I was patching against 7.x-1.x-dev, not 7.x-1.0-rc4...

mparker17’s picture

mparker17’s picture

Assigned: Unassigned » mparker17
Status: Needs review » Needs work

Hmm... the patch doesn't work because the JS isn't getting the data from Drupal.settings.

Also, the active_link setting is not saved in the facet display settings.

I'll fix.

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
FileSize
3.15 KB
1.72 KB

The "active_link" setting not being displayed in the facet display settings was a result of #735528: FAPI #states: Fix conditionals to allow OR and XOR constructions.

Getting data from Drupal.settings was easy.

Also I fixed a comment to conform with coding standards.

Feedback welcome!

cpliakas’s picture

Status: Needs review » Needs work

Patch looks good, but marking as needs work because it no longer applies to HEAD due to some recent commits.

Thanks for the contribution and continued efforts on this issue.
Chris

mparker17’s picture

Status: Needs work » Needs review
FileSize
3.07 KB

Straight re-roll, no changes, so no interdiff.

mparker17’s picture

Status: Needs review » Needs work

A few issues:

  • The active_link toggle passed to the front-end is not keyed per-facet. In other words, the last facet to have it's list items built determines whether all checkboxes are built or not.
  • The active_link checkbox does not disappear when the Display widget is set to Links.
  • The !empty($this->settings->settings['active_link']) returns TRUE even if active_link has not been set. If this change was to be released, existing site facets would work normally until someone saved the facet settings, at which point it would change behavior, even if they did not touch the "Active facet is a link" checkbox. This change would be unexpected.
  • The checkbox title "Active facet is a link" and description text "The active facet is a link. The javascript will not disable the link." don't clearly explain what the setting does.
mparker17’s picture

Assigned: Unassigned » mparker17
FileSize
3.17 KB
3.06 KB

Okay, I'm going to work on the problems I identified one at a time. I apologize in advance for the number of new comments+patches this is going to create; but I was having trouble doing it all in one patch. On the plus side, hopefully reviewing the interdiffs will be easier.

This patch fixes the name of the setting not being very descriptive; i.e.: this:

The checkbox title "Active facet is a link" and description text "The active facet is a link. The javascript will not disable the link." don't clearly explain what the setting does.

The name of the setting has been changed for consistency. That means if you applied a previous patch from this issue, and you update to the patch attached to this comment or later, you'll need to re-save your configuration! Note that, due to the "The active_link toggle passed to the front-end is not keyed per-facet" bug I identified earlier, this means you'll have to re-save all your configuration. Sorry... it had to happen sometime, might as well be early, right?

mparker17’s picture

FileSize
3.17 KB
426 bytes

This patch fixes:

The !empty($this->settings->settings['active_link']) returns TRUE even if active_link has not been set. If this change was to be released, existing site facets would work normally until someone saved the facet settings, at which point it would change behavior, even if they did not touch the "Active facet is a link" checkbox. This change would be unexpected.

mparker17’s picture

FileSize
4.22 KB
4.11 KB

This patch fixes:

The active_link checkbox does not disappear when the Display widget is set to Links.

mparker17’s picture

FileSize
3.45 KB
739 bytes

Whoops... forgot to remove some Javascript from the facetapi.admin.js file. This was supposed to happen in the previous comment (#31).

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
5.8 KB
3.55 KB

Okay, here's the patch to fix:

The active_link toggle passed to the front-end is not keyed per-facet. In other words, the last facet to have it's list items built determines whether all checkboxes are built or not.

Marking as "needs issue summary update".


To get this working, I ended up having to change the JavaScript-API for Drupal.facetapi.makeCheckbox() slightly. Any function that calls it and expects it to treat this as the HTML element to manipulate will not work. These functions will have to pass this as the first argument.

However, I don't anticipate this JavaScript-API change to be a problem, because, as far as I know, the only JavaScript that calls Drupal.facetapi.makeCheckbox() is inside facetapi.js, and I've fixed that. I also searched as many other modules as I could think of that depended on FacetAPI (current_search, search_api_facetapi, date_facets, facetapi_bonus, facetapi_i18n, wetkit_search, wetkit_apachesolr, and facetapi_select) — none of these call it.


Detailed notes on my JavaScript changes if you want to follow along:

  • Goal: pass the current facet settings into the Drupal.facetapi.makeCheckbox() function.
  • A line like $links.once('facetapi-makeCheckbox').each(Drupal.facetapi.makeCheckbox(facet_settings)); would NOT work because the jQueryeach() function passes in it's own two arguments (index and element).
  • Furthermore, because makeCheckbox() was defined separately (i.e.: not an anonymous function in the first argument of the each() call), JavaScript scoping rules didn't apply, so I could not access variables defined in Drupal.facetapi.makeCheckboxes().
  • My solution was to create an anonymous function in the first argument of the each() call, which could take advantage of the scoping rules. The only thing this function does is call makeCheckbox() with the additional parameters:
    $links.once('facetapi-makeCheckbox').each(function(index, element) {
      Drupal.facetapi.makeCheckbox(element, facet_settings);
    });
    
  • Note that I also pass in element to makeCheckbox() and I have replaced references to this with references to element inside it. This was necessary because the this object exists in the scope of the anonymous function in the first parameter of the each() call, but it's not passed to anything called by that anonymous function.
  • Fortunately, a quick assertion inside the anonymous function proved that element === this (exactly equivalent i.e.: literally the same object in memory), so I know I can safely do that.
  • Unfortunately, once we get inside Drupal.facetapi.makeCheckbox(), this has been assigned to something different, so I can't easily add something like if (!this) { this = element; } to the top of makeCheckbox().
mparker17’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs issue summary update

Updated the issue summary.

But, as I was writing the update, I realized the output is [checkbox] [linked deactivate widget] [unlinked label], which is not Jon Pugh's desired output. So back to needs work.

mparker17’s picture

Turns out that the output comes from the theme_facetapi_link_active() function, and is the same as the active "Link" widget text.

I guess we have a few different ways to proceed:

... some feedback from someone else would be appreciated at this point.

mparker17’s picture

Status: Needs work » Needs review

Needs someone else to review...

legolasbo’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed the patch and could only find nitpicks. Besides that I can report that the patch works as advertised.

  1. +++ b/facetapi.js
    @@ -132,8 +134,12 @@ Drupal.facetapi.makeCheckbox = function() {
    +    // Hide the link if set to do so.
    

    I think this comment only adds noise because the code speaks for itself.

  2. +++ b/plugins/facetapi/widget_links.inc
    @@ -74,6 +74,19 @@ class FacetapiWidgetLinks extends FacetapiWidget {
       /**
    +   * Gets the base class array for a facet item.
    +   *
    +   * Classes that extend FacetapiWidgetLinks will often overide this method to
    +   * alter the link displays via CSS without having to touch the render array.
    +   *
    +   * @return array
    +   *   An array of classes.
    +   */
    +  function getItemClasses() {
    +    return array();
    +  }
    +
    +  /**
        * Transforms the render array for use with theme_item_list().
        *
        * The recursion allows this function to act on the various levels of a
    @@ -150,19 +163,6 @@ class FacetapiWidgetLinks extends FacetapiWidget {
    
    @@ -150,19 +163,6 @@ class FacetapiWidgetLinks extends FacetapiWidget {
       }
     
       /**
    -   * Gets the base class array for a facet item.
    -   *
    -   * Classes that extend FacetapiWidgetLinks will often overide this method to
    -   * alter the link displays via CSS without having to touch the render array.
    -   *
    -   * @return array
    -   *   An array of classes.
    -   */
    -  function getItemClasses() {
    -    return array();
    -  }
    -
    -  /**
    

    It seems this code was moved within the same file. This adds noise to the patch and makes it harder to review.

  3. +++ b/plugins/facetapi/widget_links.inc
    @@ -261,4 +261,43 @@ class FacetapiWidgetCheckboxLinks extends FacetapiWidgetLinks {
    +    // Pass some of this facet's settings to the front-end.
    +    $this->jsSettings['show_active_label'] = $this->settings->settings['show_active_label'];
    

    This could be a simple but descriptive function call, like addShowActiveLabelJsSetting.
    That way we could lose the comment and have cleaner more descriptive code.

mparker17’s picture

@legolasbo, makes sense to me. I'll try to upload a new patch in the next week or so with the nitpicks, but I'll leave it as RTBC in case the maintainer gets to it before I do.

mparker17’s picture

Patch in #33 needed a re-roll. Since this is a re-roll, an interdiff is not necessary.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: when_a_facet_is_active-1526020-39.patch, failed testing.

mparker17’s picture

Oops, apparently I fail at fixing merge conflicts during rebase. Let's try this new, better re-roll that doesn't accidentally duplicate FacetapiWidgetLinks::getItemClasses().

mparker17’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
1.31 KB

The attached patch addresses the issues pointed out in the code review in #37:

1. The comment has been removed.
2. This is no longer happening since the re-roll, so there is nothing to do.
3. I added a private function with the name you suggested (addShowActiveLabelJsSetting()) and called it.

Further reviews welcome!

B-Prod’s picture

Status: Needs review » Reviewed & tested by the community

The patch seems to work fine and do not break existing behaviours.

Internet’s picture

Patch failed on both the 7.x-1.5 module and dev version ( git and manually attempt ).
widget.links.inc broke site.

facetapi.js failed -

The dev version uses: $elem while the 7.x-1.5 version uses $link

if (active) {
checkbox.attr('checked', true);
// Add the checkbox and label, hide the link.
$elem.before(label).before(checkbox).hide();
}
else {
$elem.before(label).before(checkbox);
}
}

if (active) {
checkbox.attr('checked', true);
// Add the checkbox and label, hide the link.
$link.before(label).before(checkbox).hide();
}
else {
$link.before(label).before(checkbox);
}
}

Any guidance? Would really like to use this patch.

mparker17’s picture

Status: Reviewed & tested by the community » Needs work

I can confirm that the patch does not apply against the latest 7.x-1.x HEAD (and I will re-roll the patch).

However, the patch applies with no errors or warnings to 7.x-1.5 for me:

$ git clone --branch 7.x-1.x https://git.drupal.org/project/facetapi.git
Cloning into 'facetapi'...
remote: Counting objects: 3250, done.
remote: Compressing objects: 100% (2573/2573), done.
remote: Total 3250 (delta 2166), reused 508 (delta 395)
Receiving objects: 100% (3250/3250), 676.13 KiB | 847.00 KiB/s, done.
Resolving deltas: 100% (2166/2166), done.

$ cd facetapi/

$ git apply --index ~/Downloads/when_a_facet_is_active-1526020-42.patch 
error: patch failed: facetapi.js:74
error: facetapi.js: patch does not apply

$ git checkout 7.x-1.5
Note: checking out '7.x-1.5'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b <new-branch-name>

HEAD is now at 316b904... Issue #2159883 by Daemon_Byte, cpliakas: Date facets not displayed when the configured granularity is larger than the calculated granularity.

$ git apply --index ~/Downloads/when_a_facet_is_active-1526020-42.patch 

$ git status
HEAD detached at 7.x-1.5
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   facetapi.js
	modified:   plugins/facetapi/widget_links.inc

$

@Internet, do you have any other patches applied to Facet API 7.x-1.5?

***

Also, what do you mean by "widget.links.inc broke site."? How did it break the site / what error did you get? Did you flush caches before you got the error?

mparker17’s picture

Status: Needs work » Needs review
FileSize
4.73 KB

Here is an updated patch. This is a straight re-roll so no interdiff is needed. I have not had a chance to run it though.

Internet’s picture

Thank you, the Instructions worked well and the patch is applied (below). However the facet links appear to behave similarly to before (caches cleared and update run) as the link label / text does not form part of the (deselect) link - screenshots below. Is this the intended behaviour, if so is there perhaps a method to include the text?

HEAD is now at 316b904... Issue #2159883 by Daemon_Byte, cpliakas: Date facets not displayed when the configured granularity is larger than the calculated granularity.
Marks-MBP:facetapi markcullen$ git apply --index ~/Downloads/when_a_facet_is_active-1526020-42.patch
Marks-MBP:facetapi markcullen$ git status
HEAD detached at 7.x-1.5
Changes to be committed:
(use "git reset HEAD ..." to unstage)

modified: facetapi.js
modified: plugins/facetapi/widget_links.inc

Internet’s picture

#46 patch works against the current module, though as above, the text is not actually a link, which would be highly desirable as it allows for some very elegant styling solutions. If this is not currently possible I'll attempt a css fix to overlay the text and post here.
Very nice module though that we use on High Visibility house sales (real estate) website https://www.ibayhomes.com/properties

eugene.ilyin’s picture

I don't know why theme_facetapi_link_active could not help before, but now you can meet your requirements easy.

1. Overwrite function theme_facetapi_link_active in your theme, for example as my_theme_facetapi_link_active
2. Change logic a bit from

$variables['text'] = t('!facetapi_deactivate_widget !facetapi_accessible_markup', $replacements);
$variables['options']['html'] = TRUE;
return theme_link($variables) . $link_text;

to

$variables['text'] = t('!facetapi_deactivate_widget !facetapi_accessible_markup', $replacements);
$variables['options']['html'] = TRUE;
$variables['text'] .= $link_text;
return theme_link($variables);

3. Overwrite function Drupal.facetapi.makeCheckbox in JS to do not hide a link.

eugene.ilyin’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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