Problem/Motivation
When you add a NEW filter to a View from the list of filters and you click on "Expose this filter to visitors, to allow them to change it" it will display the list of filters you can add again instead of the filter settings. If you try to expose the filter when you add it, you will continuously be sent back to the filter selection list.
Video: https://www.dropbox.com/s/x93ccp238xgd8qo/definition_of_insanity.mp4
If you save it and edit it to make it exposed it works. Just not the first time when you're adding it. (I.e., the bug is not reproducible with existing filters, exposed or not.)
How to reproduce:
- Edit the Content view
- Add "Content: Node ID" filter
- Click on "Expose this filter to visitors, to allow them to change it"
- You'll be popped back to the list of filters you can add
- Click on apply(to all displays) button to save the filter.
- Click on the filter you've just created
- Click on "Expose.."
- Now it works
Proposed resolution
Make a patch that fixes this problem.
Remaining tasks
1. Manually test (it's a JavaScript problem, so automated tests will not detect it). The scenarios to test are:
- Test the defect in this issue as stated above: Add a sort/filter to a view, click 'Expose' while adding. This should now work.
- Test the defect in the related issue 2499837: Add multiple sorts/filters to a view. This should work with settings dialogs for each added item, regardsless of setting the option 'Expose' or clicking the 'Delete' link.
- Generic single dialog test: edit a simple view setting (such as the Title): the modal dialog should still work (including changing, saving and deleting).
- Generic multi-dialog test: edit a view setting that forwards you to a second form before closing the modal dialog (such the Format): change the Format, and next you should see the settings form for the newly selected format.
User interface changes
Presumably it works again. ;)
API changes
Hopefully none.
Beta phase evaluation
Issue category | Bug because user is directed to an unintended place |
---|---|
Issue priority | Major because of the confusion caused by the redirection when trying to do a pretty common task in Views |
Prioritized changes | The main goal of this issue is a usability, specifically a usability bug fix, so it is prioritized. |
Disruption | Most of the patch is limited to UI code in Views and Views UI module, so that part should not be disruptive. Disruption analysis needed here! |
Comment | File | Size | Author |
---|---|---|---|
#85 | Screenshot from 2016-03-14 11:54:56.png | 404.33 KB | johnchque |
#78 | views_filter_loop-2248223-78.patch | 10.18 KB | Lendude |
#78 | interdiff-2248223-71-78.txt | 724 bytes | Lendude |
#71 | views_filter_loop-2248223-71.patch | 10.15 KB | Lendude |
#71 | interdiff-2248223-48-71.txt | 3.27 KB | Lendude |
Comments
Comment #1
xjmI was able to reproduce this in HEAD. If you keep adding your new filter and checking expose you can go in a nice little infinite UI loop. :)
Comment #2
xjmEnjoy:
https://www.dropbox.com/s/x93ccp238xgd8qo/definition_of_insanity.mp4
Comment #3
xjmComment #4
dawehnerDid anyone tried it without enabled javascript? We had a lot of issues which changes in the JS.
Random related issue:
#2168893: Views filters groups adding and removing is broken
Comment #5
dawehnerAnother related: #1967800: 'Select all' in exposed filter settings should be checked if all values are checked
Comment #6
dawehnerAd probably even more related: #2130205: Unable to "Expose this filter to visitors, to allow them to change it"
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedWithout javascript enabled, it seems to work properly. This is probably caused by an unexpected call to a change/click/.. event.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedThe issue is caused because a handler that simulates a button click/submit event is added on every exposed filter checkbox/radio button click.
The patch attached excludes the checkbox from the regular checkbox behavior, thus preventing the handler to be added. This is by no means a decent fix, but merely to demonstrate my findings.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedForgot to upload.
Comment #10
jhodgdonI just ran into this today. it is still an issue.
I see that there's a patch, and I'll test it shortly.
Comment #11
jhodgdonI tested the patch. But... it doesn't work. If I check the box now, I don't get the added edit fields to let me set up for the exposed filter (the label, etc.), whether I'm creating a new filter or editing an existing filter, so it effectively breaks the ability to configure exposed filters.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedI closed a newly reported issue, since it's an exact duplicate of this one: #2455903: When adding new filter, checking Expose this filter returns to .
Comment #13
idebr CreditAttribution: idebr commentedComment #14
lanchez CreditAttribution: lanchez commentedLooking this now.
Comment #15
lanchez CreditAttribution: lanchez commentedSo just a quick update. The js part is not responsible of the bug although the js also is somewhat bugged.
The problem is that with js enabled the form ajax is using wrong controller. It posts(?) to AddHandler when it should use ConfigHandler. I'm trying to figure out how it should work, but it might be too tricky for me to solve. Lets see.
Comment #16
lanchez CreditAttribution: lanchez commentedComment #17
mikeker CreditAttribution: mikeker commented@lanchez: I'm not sure... It think this is limited to the JS side of things, at least for HEAD today. I tried adding an exposed filter with JS turned off and it worked correctly.
Comment #18
mikeker CreditAttribution: mikeker commentedSpent some time this morning poking around in the Views UI code and how it works with Drupal core's Ajax code. Lots o' fun...
As far as I can tell:
action
attribute. (See /core/misc/ajax.js@67)/admin/structure/views/{nojs|ajax}/add-handler/...
.../add-handler/...
instead of just.../handler/...
.The last item makes for an especially bad UX for items normally hidden in the "Advanced" fieldset (relationships, contextual filters, etc.) as you hit Remove and when you (eventually) get back to the main view config screen, the Advanced fieldset is closed so you can't see that the item is still there.
I think the correct solution is to rewire the Views UI routing so that both
.../handler/...
and.../add-handler/...
are at the same endpoint. I'll ping folks in IRC to get some opinions.Comment #19
dawehnerTalked with mikeker on IRC and we agreed to try out the approach from #2168893: Views filters groups adding and removing is broken to set the form action.
Comment #20
metzlerd CreditAttribution: metzlerd as a volunteer and commentedTriage notes
Comment #21
mikeker CreditAttribution: mikeker as a volunteer commentedRerolled @olli's patch from #2356259-8: Can not expose a filter immediately after adding a filter and in my very preliminary testing, fixes the problem. More manual testing is needed and I don't know if there is a way to write automated tests since this is limited to JS interactions.
Comment #22
mikeker CreditAttribution: mikeker as a volunteer commented@todo: Incorporate issues raised in #2356259: Can not expose a filter immediately after adding a filter into this issue. Add those that worked on that issue to the suggested commit message.
Comment #23
ohthehugemanatee CreditAttribution: ohthehugemanatee as a volunteer commentedI merged the patches from the two issues into the attached patch. They were remarkably similar.
Works for me.
Comment #24
mikeker CreditAttribution: mikeker as a volunteer commented@ohthehugemanatee: Can you provide an interdiff between #21 and #23? #21 is a reroll of the patch in the linked issue. Or were you referring to a different patch that was merged?
Thanks.
Comment #25
nod_eslint complains.
You should probably have a .once here too.
Comment #26
olli CreditAttribution: olli commentedFixed #25, thanks @nod_.
Comment #27
xjm(Saving proposed issue credit for discussion and triage participants at LA, as well as earlier reviewers/testers).
Comment #28
olli CreditAttribution: olli commented#2168893: Views filters groups adding and removing is broken landed.
Comment #29
jhodgdonThanks for reviving this issue!
I took a look at the patch, and it looks like pretty good code to me. There were a couple of minor docs issues:
What does "setting a form" mean?
(In other words: Class docs should tell me what the class does. This is the one-line doc for class SetFormCommand, but since I don't know what "setting a form" means, I really don't understand this. Can we add a line or two to explain what "setting a form" is and where/why it's used?)
This JS function needs a proper doc block.
I also tested the patch, using the Reproduce in the issue summary (edited it slightly for clarity and so it matches what you actually see in the UI). It works!
The patch appears to be changing quite a bit more than just this one place in the Views UI code though. What else do we need to manually test to make sure the JS/PHP changes didn't break anything? Can someone make a list of these spots that need testing, and add it to the Remaining Tasks section of the issue summary please?
I'm also concerned about this change, which looks like it has a fairly widespread effect:
What do we need to test manually to verify that this change didn't break anything? It looks as though it could have a pretty widespread effect, changing the order of triggers or something?
I also updated the beta eval section in the summary. Since this is a Prioritized and Major bug fix, we need to evaluate disruption vs. impact. I'm not sure of the implications of the JS change in dialog.ajax.js, so can someone update the Disruption section of the Beta eval to comment on that?
Comment #31
olli CreditAttribution: olli commentedThanks for the review @jhodgdon. Filed #2500723: Ajax dialog triggers click before mouseup for dialog.ajax.js because I think we could do that with or without this patch.
That SetForm command's code and comments originate from 7.x where it used to more than just make the form submit using ajax to correct url. I added
Html::setAjaxHtmlIds();
because we still use some id selectors in js. Rest of the patch removes '#ajax' and 'use-ajax-submit' css classes from buttons in the php side because that is already done in js (viewsSetForm).The patch fixes the expose button/checkbox, remove and cancel button behaviors when adding a new filter. It fixes the same problems for sort criteria. It also fixes a problem when adding multiple filters/sorts described in #2499837: Endless loop of modals when adding multiple sorts/filters to a view.
As an alternative, keeping '#ajax' and/or 'use-ajax-submit', we could try to set the button #ajax url (or form #action url) in php and find a different fix for adding multiple filters/sorts.
Comment #32
jhodgdonOK, can you update the issue summary with a clear, simple list of what parts of the Views UI we need to check manually for the next patch then? Where it says:
TBD - make this list and test it
Thanks!
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous at Druid commentedrerolled, lets see if it failed :P
Comment #34
jhodgdonNo interdiff here and this was listed as a reroll, so I'm assuming this issue is still Needs Work for previous comments?
Comment #35
jhodgdonWe still need to fix this... although for me the current behavior with Beta 12 is that it is not popping all the way back to the list of filters. It is still broken though.
Comment #36
thejacer87 CreditAttribution: thejacer87 at Acro Commerce commentedJust checked this bug with a fresh install and the latest commit - cc17945f3d80cff1620dc3f275cab625fe530154.
Bug is still there. When I applied the patch from #33, it was applied cleanly.
But when trying to reproduce the bug, in step 2, the ajax loader wheel pops up for a second, disappears then nothing happens. Clicking on any link or button in the views section (ie. breadcrumbs and admin links work) that seems to have some ajax fails in this manner.
Comment #37
olli CreditAttribution: olli commented#33 seems to apply but
does not exist/is not needed anymore.
Removed dialog.ajax.js from the summary because #2500723: Ajax dialog triggers click before mouseup landed. Not sure if this still needs a disruption analysis.
I don't have a simple list (I think this affects every dialog in views ui) but the ones that do not work in HEAD:
- expose filter/sort when adding filter/sort (this issue)
- add multiple filters/sorts (#2499837: Endless loop of modals when adding multiple sorts/filters to a view)
- click the "Remove" link when adding a new anything (filter, field, relationship, etc.) (#18)
Comment #38
Nikolay ShapovalovRerolled.
Comment #42
peterg.griffin CreditAttribution: peterg.griffin as a volunteer and at KWD Digital commentedI'll work on this at the DrupalCon Barcelona sprints.
Comment #43
peterg.griffin CreditAttribution: peterg.griffin as a volunteer and at KWD Digital commentedRemoved the Html::setAjaxHtmlIds(); as the ids should be reset automatically by Drupal earlier.
Comment #44
peterg.griffin CreditAttribution: peterg.griffin as a volunteer and at KWD Digital commentedCleaned up the patch -file by removing the commented line from ViewsFormBase.php on line 99 as it it's not needed.
Comment #45
jhodgdonAs far as I can tell, previous review comments (see #29 and later comments) have not yet been addressed, so I am setting this back to Needs Work. If they *have* been addressed, please attach an interdiff. Also the last time the patch was tested, it did not actually fix the problem... no interdiff so far has changed the code so I am assuming this is still not working.
Comment #46
LendudeJust manually tested this, it's still broken in HEAD and with the patch in #44 applied it works for the given scenarios. Will look at making a list as requested and doing some more manual testing with the patch applied.
Edit: thought it didn't work but needed more cache clearing and switching off of aggregation to clear everything then i first thought.
Comment #47
finneComment #48
finneCleaned up patch #44: It now has more descriptive commenting and docblocks. Checked with @nod_ and updated the manual tests list. Conformed to new class naming as championed by @mortendk: prepended class name with js-.
So this is what the patch does, explained (for my self and any newcomers to this issue):
The patch moves the option to have a custom submit URL on buttons in a modal views UI form into a separate object/function:
- remove from ViewUI.php and ViewsFormBase.php and RearrangeFilter.php and ConfigHandler.php
- remove expose button ajax class from form in SortPluginBase.php and FilterPluginBase.php
- add separate object in new file SetFormCommand.php
- handle the command in js in views file ajax.js
also changed the jQuery class from .views-ui-dialog to .js-views-ui-dialog to make clear we are (mis)using a class to target javascript functionality.
All issues raised in comment #29 and further are now addressed.
The manual tests after applying the patch now all pass.
Comment #49
finneChanged the remaining tasks:
- there is now a working patch. All issues raised in comment #29 and further are now addressed.
- The patch also makes changes in any modal views ui form dialog, so I added some generic tests to the list of manual tests.
Comment #50
finneComment #51
dawehnerManually tested the patch, this really fixes the problem. Nod_ seems to be okay with the javascript.
Comment #52
nod_Yep, all good with the JS.
I have a feeling something is fishy but I can't put my finger on it. In the meantime this fixes the issue.
Comment #53
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAssigning to myself to review this for commit.
Comment #54
juampynr CreditAttribution: juampynr at Lullabot commentedIs there a reason why this did not get committed? I just tested the patch at #48 and verified that it still applies to HEAD and fixes the issue.
Comment #55
dawehnerWell, I doubt there is beside the lack of time ...
Comment #56
juampynr CreditAttribution: juampynr at Lullabot commentedGood! I thought that there could be a problem with the patch not being semver compliant.
Comment #57
dawehnerWhat kind of API is in there? I just see a UI fix by adding a bit more code. This is not at all a feature, IMHO.
Comment #59
jhodgdontestbot glitch
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedThe patch #48 worked properly for me to. Good :)
Comment #61
dawehnerComment #62
tic2000 CreditAttribution: tic2000 at Intellix commentedHere the events should be namespaced. IE
click.views_set_form mousedown.views_set_form
Comment #63
dawehnerPing @effulgentsia
IMHO this is a really important / annoying issue for actual site builders. Can we please fix this issue and maybe perfectionize if needed later?
I've seen multiple people running into that issue outside of here already.
Comment #64
tic2000 CreditAttribution: tic2000 at Intellix commentedI didn't change to NW for that reason.
Comment #65
dawehner@tic2000
Is this common in other places in core?
Comment #66
tic2000 CreditAttribution: tic2000 at Intellix commentedSome files do it (details-aria, dialog, filter.admin, node.preview, toolbar.menu, user.permissions, views-admin, seven's nav-tabs), others don't (ajax doesn't for example #2660272: Namespace ajax event).
There is an issue for adding this to the standards. #674716: We need guidelines on usage of DOM events and jQuery's namespaced events
So is not enough to stop the commit of this issue, but if the patch is changed till then it would be nice.
Comment #67
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFirst of all, apologies that it took so long for me to review this.
I think the patch looks great for 8.1. I have some minor nits that I'll post following this comment, but in this comment, I just want to express that I'm hesitant about this going into 8.0. While I understand that the bug is annoying for the people who run into it, and it's easy to run into, I'm concerned that the change from
#ajax
/use-ajax-submit
applied on individual buttons to the form-level Ajax binding that's done in the patch's addition ofDrupal.AjaxCommands.prototype.viewsSetForm
is a change that might have unintended consequences. I couldn't find any undesired consequences in anything that I tested, but I just have an uneasy feeling (and per #52, I'm not the only one) that some contrib/custom module or some other environment-specific setup will run into some issue with this. While I think that risk is a perfectly acceptable one to introduce into 8.1, I'm not yet convinced it's worth introducing into 8.0, as opposed to just letting site builders live with the existing bug for a few more months until 8.1 comes out.So meanwhile, assigning this to 8.1, but after it lands in 8.1, if anyone wants to make a case for getting it into 8.0 despite my hesitation, please do so then.
Comment #68
effulgentsia CreditAttribution: effulgentsia at Acquia commentedShould we put this into the views_ui namespace instead? I realize there are other ajax commands in the
\Drupal\views\Ajax
namespace already that are only ever used by views_ui, but I kind of think that's unfortunate, and that we shouldn't expand that problem. Also, for me, adding this into theviews
namespace makes it feel more like an API addition (it implies that it's available for contrib to use outside of views_ui), which is fine for 8.1, but less fine if we still think we'll want to try to get this into 8.0.Would be nice to make this fit on one line. Also, "this is done on a per-button basis" seems entirely incorrect to me, or am I missing something?
Should we change this to "This command is implemented in Drupal.AjaxCommands.prototype.viewsSetForm." to match the other Views ajax commands?
Comment #69
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOh, I do want to also add that I really do like this patch a lot. For processing modal forms, binding the Ajax behavior (including which URL to submit to) to the form directly is much more logical and elegant than HEAD's approach of binding each button within the form individually. In addition to its own elegance, it also means that after this patch, contrib handler forms that add some button but forget to add an ajax binding to it will work correctly in a modal context, which is really nice.
Comment #70
dawehnerThank you @effulgentsia to come back to the issue.
I totally agree with you, let's just put it in views_ui. I prefer sanity over consistency, but well, other people have other thoughts about that.
Well, at som epoint we need to ask ourself, which parts we can actually change and which we don't want touch anymore. I'm happy when it gets committed to 8.0.x, but IMHO when we care about users primimarily, then getting it into 8.0.x is kind of needed.
Comment #71
LendudeUpdated per #68.
Comment #73
dawehnerThank you lendude!
Comment #74
droplet CreditAttribution: droplet commentedadd scoped context if it's possible. Ideally a selector of that form
don't find input / button, use class.
what is it ? and what is clk ?
Comment #75
catchLooks like CNW.
Comment #76
dawehner@droplet Do you mind clarifying it a bit?
Well, this is about all buttons. Isn't that exactly the selector you want to have?
Well, this is jquery form: https://github.com/malsup/form/blob/master/jquery.form.js#L449
Of which form? I mean the form is what we select at that point in time. It is also a ajax command so it doesn't have any kind of context.
Comment #77
droplet CreditAttribution: droplet commentedIt's fine if we always made some assumption for ONLY one available at the time. But always be good to add it if we can.
It's same as above. But here's more uncertain than above one. therefore, the patch matched extra "button" here. what if we adding another "button" for other purpose. For a new API we should take care of it. At this case I believe there's a ".js-form-submit" we can use ?
(Not just for code side, it's also violate CSS code standard. Although I know drupal didn't following that strictly in the past, I hope we can do it in all new API)
Comment #78
LendudeNeeding scoped context for the form would mean multiple forms inside the dialog, this sounds like something we don't need to bother with.
Adding a class selector to the button/submit sounds ok since we have that class anyway.
Reroll for class selector on the button
Comment #79
dawehnerCool
Comment #80
droplet CreditAttribution: droplet commentedIdeally, it should be `.js-form-submit` only (in SMACSS suggestion) but i don't bother too much here :)
Thanks.
Comment #81
alexpottNice work everyone. Committed 29962dd and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks! I've committed to 8.0.x because the bug is major and the patch does not appear to make any API changes.
Comment #85
johnchqueThis seems to cause a problem, I tried to edit a view, add an exposed filter to it, whether I click Cancel or try to Add a filter I get something like this:
Also I get the same if I want to add a new item on an exposed filter but when I go back to edit the view, the new item is already added.
Comment #86
gnugetThis already has been commited, Can you please provide a list of steps to reproduce this problem? so we can try to replicate the problem and even revert the commit if necessary.
Comment #87
johnchqueTo reproduce this problem:
- Go to views
- Edit any view (e.g. Content)
- Try to add a new filter criteria
- Press cancel
And you get the same screen as the last screenshot.
Comment #88
Lendude@yongt9412 Can't reproduce this on 8.1.x or 8.2.x branch (didn't try 8.0.x).
I've seen the symptoms you describe many times but this usually happens to me when I git pull and don't clear cache after that. Please try a fresh install and see if you still have issues then.
Marking this fixed again, if you run into more problems please open a new issue for that.
Comment #89
edurenye CreditAttribution: edurenye at MD Systems GmbH commented@Lendude You are right, it doesn't happen in 8.1.x, nor 8.2.x. It just happens in 8.0.x, and in Ubuntu with chrome, I tested in MAC (chrome, safari and firefox) and it doesn't happen, neither in Ubuntu with Firefox.
So I could just reproduce in Ubuntu with Chrome and with the version 8.0.x of Drupal.
Comment #90
droplet CreditAttribution: droplet commentedget a screen with JSON ? using same server to test?
Comment #91
dawehnerThese kind of JSON errors always happen when there is some JS error before that.
Comment #92
edurenye CreditAttribution: edurenye at MD Systems GmbH commentedIt's strange, I tested a bit more and seems that this just happens when I'm accessing the local server in my machine, if I access to it through a public address it does not happen.
Moreover I checked with the browser inspector and there is no JavaScript error, It's just that it redirects to this path: "admin/structure/views/ajax/add-handler/content/page_1/filter" when you click cancel.