When using checkbox widget to display facets and check at least 2 items only the last one is enabled.
Checkbox widget only allow to enable one item at a time.

Also items deselect makes strange behaviours. You uncheck one element, but after submit other element is unchecked.

Tests in #31 still required to merge into patch if this goes in.

Comments

Evaldas Užkuras created an issue. See original summary.

michiellucas’s picture

When having multiple checkbox facets there is also the following problem

FACET 1
- Cats
- Dogs
SUBMIT

Facet 2
- Funny
- Stupid
Submit

If you select Cats and Funny and press Submit of facet 1 the value Funny will not be submitted.

michiellucas’s picture

Question:
Using a form as widget, does that not bypass the cache every time you submit?

borisson_’s picture

Drupal's cache system disables cache for all post requests, that's true. But that doesn't really matter for facets, because they are uncacheable anyway. See http://cgit.drupalcode.org/facets/tree/src/Plugin/Block/FacetBlock.php#n107

Evaldas Užkuras’s picture

The problem is with this part of code:
http://cgit.drupalcode.org/facets/tree/src/Plugin/facets/widget/Checkbox...

After submit the url of the last active item is used. And the redirect to this url is made
That url only contain information with eneble/disable that facet item.
This url is used in link widget, where you just click on one item at once.

michiellucas’s picture

If you debug you will notice that only the last facet will pass the submitForm function, so that is a problem when you don't have autosubmit and you select multiple facets at one

Evaldas Užkuras’s picture

For autosubmit, maybe this widget should be replaced with Links with checkboxes.
And instead of +/-, it shows checkbox with checked/unchecked state.
This type of widget is used In D7 Facets API.

borisson_’s picture

I think the solution for this will be the same as the direction #2665400: InvalidArgumentException: The URI 'datasets' is invalid. is going in.

OlgaRabodzei’s picture

Tell me please does one checkbox facet work correctly? Because I have a problem with it. After submit it doesn't redirect to filtered result. The facet disappears and I stay on the page with all items.

borisson_’s picture

Issue summary: View changes

A facet with one checkbox should work as expected. Let's use this issue to expand the Widget Integration Test to include multiple facet values.

borisson_’s picture

I think this tests exactly what's wrong for this issue?

Status: Needs review » Needs work

The last submitted patch, 11: checkbox_widget_does-2658678-11--test-only.patch, failed testing.

The last submitted patch, 11: checkbox_widget_does-2658678-11--test-only.patch, failed testing.

Leksat’s picture

Another bug with the checkboxes:
- I have two different facets displayed as checkboxes
- selecting one item on the first one, submitting
- selecting one item on the second one, submitting
- all good for now
- unselecting item on the first facet, submitting
Expected: the second facet is not changed.
Actually: the second facet is unselected as well.

Do we need a new issue for that?

borisson_’s picture

@Leksat: no, I think this is fine to be solved in this issue. That's exactly what's going on in this issue.

borisson_’s picture

I think this should resolve the problem @Leksat had in #14. This code is very ugly and the test still disagree but this is taking steps in the right direction. I think.

Status: Needs review » Needs work

The last submitted patch, 16: checkbox_widget_does-2658678-16.patch, failed testing.

The last submitted patch, 16: checkbox_widget_does-2658678-16.patch, failed testing.

borisson_’s picture

Attached patch should improve the behavior of the checkbox widget, I'm very unhappy with the code but it seems to be working. There's a new testcase that should be specific enough to resolve those issues.

Status: Needs review » Needs work

The last submitted patch, 19: checkbox_widget_does-2658678-19.patch, failed testing.

The last submitted patch, 19: checkbox_widget_does-2658678-19.patch, failed testing.

borisson_’s picture

This needed to be fixed as well, still need to figure out how to fix the remaining failure.

Status: Needs review » Needs work

The last submitted patch, 22: checkbox_widget_does-2658678-22.patch, failed testing.

The last submitted patch, 22: checkbox_widget_does-2658678-22.patch, failed testing.

borisson_’s picture

Hopefully back to 1 fail now. I don't see how we can resolve that one.

Status: Needs review » Needs work

The last submitted patch, 25: checkbox_widget_does-2658678-25.patch, failed testing.

The last submitted patch, 25: checkbox_widget_does-2658678-25.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 28: checkbox_widget_does-2658678-28.patch, failed testing.

The last submitted patch, 28: checkbox_widget_does-2658678-28.patch, failed testing.

borisson_’s picture

Removed debug code, still failing though. I have no idea how to resolve this.

Status: Needs review » Needs work

The last submitted patch, 31: checkbox_widget_does-2658678-31.patch, failed testing.

The last submitted patch, 31: checkbox_widget_does-2658678-31.patch, failed testing.

borisson_’s picture

Priority: Normal » Critical

Discussed this with @StryKaizer on our way back from Drupalcamp Granada, we figure this could potentially lead to some bigger changes. Marking this as critical. If someone else wants to give this a go, it should probably be sufficient to copy out the testcases and start again until the WidgetIntegrationTest passes.

StryKaizer’s picture

At this moment we have 2 options:
- either make checkbox fields links (link d7 did)
- provide an extra function in the urlprocessor interface to build multiple items at once.

Attached is an example for option 2

StryKaizer’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: checkbox_widget_does-2658678-35.patch, failed testing.

The last submitted patch, 35: checkbox_widget_does-2658678-35.patch, failed testing.

StryKaizer’s picture

Tests from #31 still needs to be merged with this patch

StryKaizer’s picture

Status: Needs review » Needs work

#39 was a test to see if we can fix the drupal form.

I discussed this a bit further with borisson_ and we think we should not use a real drupal form for this widget but use links similar to what d7 did.

Using a drupal form generates a POST every request, while we only change urls. Facets does not explicitly support caching (yet?!?) as certain datasources can not be cached, but this does not mean users can not throw custom caching (or a varnish) over their resultset. We should not break those cachehits by always using a POST request.

So I suggest we use the same approuch as D7 facet_api did (using links with checkboxes in css)

PROs
- Cachable page requests.
- Simpler code, no need to extend the urlprocessor interface to support multiple items at once.
CONs
- separate page request for each selection

borisson_’s picture

I agree with the approach described in #40. It'll give us less headaches down the road.

StryKaizer’s picture

This patch rewrites the checkboxwidget to use the D7 approuch, js and gracefull degration into links for non-js sites.

No tests has been added/changed/fixxed yet, will be red ;)

Status: Needs review » Needs work

The last submitted patch, 42: checkbox_widget_does-2658678-42.patch, failed testing.

The last submitted patch, 42: checkbox_widget_does-2658678-42.patch, failed testing.

borisson_’s picture

Attached patch fixes the unit tests, so that's one thing. Let's see how many integration tests break.

Status: Needs review » Needs work

The last submitted patch, 45: checkbox_widget_does-2658678-45.patch, failed testing.

The last submitted patch, 45: checkbox_widget_does-2658678-45.patch, failed testing.

borisson_’s picture

The only integration test that failed was not needed anymore. I removed that. All integration tests should pass as well.

StryKaizer’s picture

Changed the links id to a data-attribute to prevent duplicate id's in the DOM.

And some cleanup...

StryKaizer’s picture

FileSize
2.54 KB

Interdiff from #49

StryKaizer’s picture

borisson_’s picture

Small code style cleanup, overall this looks pretty great and I think we can get this in. I'd love to get an extra review if possible.

Evaldas Užkuras’s picture

I my opinion we should add checkbox element with php, not with javascript.
Now it would we harder to style checkbox elements. Also, it will not get same markup as other themed checkbox elements used in forms.
And if themer will need to change style for those facets links, he will have to rewrite JS files.

borisson_’s picture

@Evaldas Užkuras: This is the same way it works in d7, while the markup is different, I don't think the JS would need to be rewritten to theme the links, I'm pretty sure that we provide enough classes to style the links / checkboxes as needed.

However, this issue is resolved in an easier way, it also simplifies the codebase a bunch and makes resolving #2692027: "All" link in facets?, #2711149: Using either List of checkboxes or Dropdown Widget Fails on submit , #2710625: Use one of the Drupal RedirectResponse classes in Checkbox & dropdown widgets., #2611198: Give widgets the ability to require settings, #2656856: Checkboxes - having multiple facets provide option to have 1 submit button easier / not relevant.

borisson_’s picture

Status: Needs review » Fixed

Committed and pushed, we discussed this at the hangout and decided that this was the easiest route forward, thanks for all the hard work everyone!

  • borisson_ committed 832ba3a on 8.x-1.x
    Issue #2658678 by borisson_, StryKaizer, Evaldas Užkuras: Checkbox...

Status: Fixed » Closed (fixed)

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