Create Javascript library for searching rendered lists on the client.

Created on 28 January 2019, over 6 years ago
Updated 4 November 2023, over 1 year ago

Problem/Motivation

There are several places in core where the user searches in a text area to filter a list that is already rendered on the page. This filtering does not require any call back to the server.

  1. Current uses
  2. Place block on Block layout
  3. Views listing
  4. Extend(modules listing)
  5. In progress #2998862: The Layout Builder block listing should be filterable

All of the these instances use their own Javascript and don't use a shared solution.

Proposed resolution

Investigate and determine if these use cases could shared a javascript library to perform this filtering.

If they share a enough functionality and it could be generalised then let's make library for it.

Remaining tasks

Determine if it is a good idea, are they really the same use case
Create central solution
Determine if this library should be internal or available outside of core.

User interface changes

None

API changes

A new Javascript API for filtering rendered lists

Data model changes

None

Release notes snippet

TBD, If this a Javascript ApI that is not @internal then we will need release notes for how to use it.

Feature request
Status

Active

Version

11.0 🔥

Component
Javascript 

Last updated 3 days ago

Created by

🇺🇸United States tedbow Ithaca, NY, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇬🇧United Kingdom joachim

    I'm having a go at dabbling with this. I think I can see how to follow the same sort of pattern as tabledrag, which allows specific uses of it to customise it.

  • Status changed to Needs review over 1 year ago
  • 🇬🇧United Kingdom joachim

    There's still handling of table grouping to do, and then conversion of existing filtering to the library, but I'd really appreciate a sanity check of my totally amateur attempt at JavaScript :)

  • Pipeline finished with Failed
    over 1 year ago
    #47220
  • Pipeline finished with Failed
    over 1 year ago
    #47246
  • 🇬🇧United Kingdom joachim

    Found a bug when a DETAILS element starts off closed but an item is found in that element. The whole DETAILS gets hidden! I wonder if when the DETAILS is closed, the things inside it count as not visible?

  • Status changed to Needs work over 1 year ago
  • The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    over 1 year ago
    #47663
  • Pipeline finished with Failed
    over 1 year ago
    #47670
  • Pipeline finished with Failed
    over 1 year ago
    #48261
  • 🇬🇧United Kingdom joachim

    Something to discuss: the current filtering implementations are not consistent in their behaviour, on two axes:

    - filter on first character typed / filter only on a minimum of 2 characters typed
    - search from the start of words / search inside words -- in other words, will typing 'om' find 'comment' or not?

    1. Place block on Block layout - 2 chars, search in words
    2. Views listing - 2 chars, search in words
    3. Extend (modules listing) - 2 chars, search from start of words
    4. Layout builder blocks - 2 chars search in words
    5. Field reuse selection - 1 char, search in words

    Do we want to standardize as part of this issue, or make the new library configurable for these behaviours?

  • 🇩🇪Germany geek-merlin Freiburg, Germany

    @joachim: I'd suggest make it configurable.

    FTR: Some time i hacked down livefilter module, which exposes similar functionality as a block. Based on this library, we may move that block to core in a followup.

  • Pipeline finished with Failed
    over 1 year ago
    #51626
  • Pipeline finished with Failed
    over 1 year ago
    #51693
  • 🇬🇧United Kingdom joachim

    Notes to self, still to do:

    - announce functionality
    - fix details opening/closing - needs to remember state prior to filtering
    - options to word boundaries & minimum search string
    - set up options for all conversions to match current behaviour

  • Pipeline finished with Failed
    over 1 year ago
    #51771
  • Pipeline finished with Failed
    over 1 year ago
    #52313
  • Issue was unassigned.
  • Status changed to Needs review over 1 year ago
  • 🇬🇧United Kingdom joachim

    I think this is ready for review.

  • Pipeline finished with Failed
    over 1 year ago
    #55890
  • Pipeline finished with Failed
    over 1 year ago
    #56050
  • Pipeline finished with Failed
    over 1 year ago
    #56415
  • Pipeline finished with Failed
    over 1 year ago
    #56461
  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom joachim

    I'm stuck on the last two complaints the JS linting has:

    > error jquery/no-is Prefer matches to $.is https://git.drupalcode.org/issue/drupal-3028968/-/blob/4d82de62/core/mis...

    I don't think we can use matches() here, as for the user permissions table, the selector for groups has to be "tr:has(td.someclass)" and in Firefox, that doesn't work with matches().

    I don't know how to rewrite this to avoid using $.is.

    I have a jQuery object that is all the groups, and a jQuery object that is the current TR from the table, and I need to determine whether the TR is one of the groups or not.

    > fatal Parsing error: Unexpected token = https://git.drupalcode.org/issue/drupal-3028968/-/blob/4d82de62/core/mis...

    This is because we don't have ??= in our JS standards yet. What's the alternative?

  • 🇷🇸Serbia finnsky

    Gonna check js

  • Merge request !6309Resolve #3028968 → (Open) created by finnsky
  • Pipeline finished with Canceled
    over 1 year ago
    #81995
  • 🇬🇧United Kingdom joachim

    Rebased the branch on 11.x. Also applied a couple of fixes that got made to core/modules/system/js/system.modules.js in the meantime to the code that replaces that file.

  • Pipeline finished with Failed
    over 1 year ago
    #81996
  • Pipeline finished with Failed
    over 1 year ago
    #81998
  • Pipeline finished with Running
    over 1 year ago
    #82168
  • 🇷🇸Serbia finnsky

    Added vanilla JS only library.

    We need to check backend and tests.

  • Pipeline finished with Failed
    over 1 year ago
    #82169
  • Pipeline finished with Failed
    over 1 year ago
    #82170
  • 🇷🇸Serbia finnsky

    @joachim @nod_
    Actual version of that library in this MR https://www.drupal.org/project/navigation/issues/3412125#comment-15461104 📌 Convert jQuery to vanilla Javascript Needs work

    We will continue with adaptation this library in Navigation module and then backport it to core as it was disqussed in slack `admin-ui`

  • 🇬🇧United Kingdom joachim

    How much more work does it need? I didn't think it needed to be extensively developed in contrib - could we just improve the code here?

  • 🇷🇸Serbia finnsky

    Last variant of this library in Navigation module was here

    https://git.drupalcode.org/project/drupal/-/merge_requests/7474

    It is removed now from navigation module since it uses Layout Builder for now.

  • Pipeline finished with Failed
    about 1 year ago
    Total: 259s
    #155576
  • 🇫🇷France nod_ Lille

    to help with searchability in the queue

  • Pipeline finished with Failed
    11 months ago
    #223900
  • Pipeline finished with Failed
    11 months ago
    Total: 548s
    #223907
  • Pipeline finished with Failed
    11 months ago
    Total: 234s
    #223919
  • Pipeline finished with Failed
    11 months ago
    Total: 557s
    #223973
  • Pipeline finished with Failed
    11 months ago
    Total: 459s
    #223987
  • Pipeline finished with Failed
    11 months ago
    #223996
  • Pipeline finished with Failed
    11 months ago
    #224005
  • 🇷🇸Serbia finnsky

    Some weird functional js errors. Cannot reproduce them locally. And it is mostly ready.

    I suggest to postpone views popup filters cause it has extra selectbox filter.

  • 🇬🇧United Kingdom joachim

    @finnsky your MR seems to have dropped the ListFilter.php render element I'd added. I feel that having a dedicated render element gives clearer DX than just adding attributes. Also, it means that you get to choose where to position the search element.

  • 🇷🇸Serbia finnsky

    Yeah. Thank you. I meant ready on front side. Can be any adaptation on backend.

  • Pipeline finished with Failed
    11 months ago
    Total: 549s
    #224086
  • Pipeline finished with Failed
    11 months ago
    Total: 461s
    #224091
  • Pipeline finished with Canceled
    11 months ago
    Total: 176s
    #224096
  • Pipeline finished with Failed
    11 months ago
    Total: 581s
    #224101
  • 🇷🇸Serbia finnsky

    Really not sure what the reason of failures. It should work. Probably filterVisibleElements() works incorrect.

  • Pipeline finished with Failed
    11 months ago
    Total: 460s
    #224111
  • 🇬🇧United Kingdom joachim

    joachim changed the visibility of the branch 3028968-create-js-library-filtering-lists to hidden.

  • Pipeline finished with Canceled
    11 months ago
    Total: 157s
    #226886
  • 🇷🇸Serbia finnsky

    @andy
    thank you for review.
    gonna check now.
    next time you also can add direct comment here. since gitlab only comment not updates issue status.

  • Pipeline finished with Failed
    11 months ago
    Total: 539s
    #226894
  • Pipeline finished with Failed
    11 months ago
    Total: 534s
    #226983
  • Status changed to Needs review 11 months ago
  • 🇷🇸Serbia finnsky

    Fixed tests. Maybe not in good way.

    Now ready for review in terms of frontend.

    @joachim, please take a look if you want to adapt render elements to current backend.

  • Pipeline finished with Failed
    11 months ago
    Total: 450s
    #226991
  • Pipeline finished with Failed
    11 months ago
    Total: 489s
    #227000
  • Pipeline finished with Failed
    11 months ago
    Total: 488s
    #227021
  • Status changed to Needs work 11 months ago
  • 🇷🇸Serbia finnsky

    Last weird test failure. I give up

  • 🇬🇧United Kingdom joachim

    > @joachim, please take a look if you want to adapt render elements to current backend.

    I'm not sure when I'll have time.

    I'm feeling a bit discouraged about this also, as it feels like doing the work I did all over again.

    Can you tell me which properties of ListFilter are for functionality your JS supports, and which should be removed?

    - #library for example can be removed, as you said in Slack that your JS handles sibling groups and details.

    What about #minimum_filter_length and #search_start_of_words? I can't tell from reading drupal-filter.js whether it handles those -- the code could really do with more comments.

  • 🇩🇪Germany rkoller Nürnberg, Germany

    and one question. at the moment the filter field is using the browser based clear button. that has the downside that the behavior differs in between browsers and in firefox there is no clear button at all. would it make sense to include the suggestion from this issue Add a clear button to the fields ui Active , adding the clear button functionality recently added to project browser, already in this issue, or keep the scope tight and keep Add a clear button to the fields ui Active as a followup?

  • 🇩🇪Germany rkoller Nürnberg, Germany

    i've also manually tested the filter with a screenreader (voiceover in my case - see filter.mp4 - recorded the video a few days ago but managed to finish the write up just now). there are a few details to note:

    somehow the announcement is sort of cut when the initial string is entered. in the first example entered ( "basic block" ) you can only hear "6 permissions", the rest of the string "are available in the modified list" gets cut by some announcement about the styling, and then just "block" gets announced again even though "basic block" was entered. when the already entered string "basic block" gets adjusted to "basic" the full string "15 permissions are available in the modified list." is getting announced. but "basic" is not announced after the announcement of "15 permissions are available in the modified list." like with the entry of "basic block" and the additional announcement of "block" before. BUT in the video when the filter is cleared by pressing the ESC key "basic" is announced before "all available permissions are listed". That is potentially confusing. And the pattern is reproducible if you enter arbitrary characters ("bvgfh"), again "0 permissions" is announced interrupted by some styling announcement, the rest of the string "are available in the modified list" is getting dropped. after that the arbitrary string is announced again. if you clear the filter field with ESC "bvgfh" is announced another time for the empty filter field followed by " All available permissions are listed"

    In regards of the micro copy used, "modified list" in the context of the example "x permissions are available in the modified list." raises potential questions and make the user at least think. Me as the user could ask what does modified list mean in this context? Which list and what is "modified" implying; results added and or removed or even altered entries? Why not go with something like "x filter results"? That would be slightly more generic but shorter and more clear.

    There is also a problem with WCAG 2.2. SC3.2.2 (https://www.w3.org/WAI/WCAG22/Understanding/on-input.html) in the current MR as well as in Core in a few places. Discussed that aspect with @mgifford. In short the goal of that success criterion is that the interface can be operated predictably by the user. At the moment as soon as the user is entering any character in the filter field the results are getting updated based on that entry immediately. The user doesn't have to press return, nor is there a submit button, nor any "warning" in the filter label. The content gets re-rendered immediately based on the entered filter string . The option might be to either add a filter button and wait with filtering until the filter button/enter is pressed or at least add some sort of "warning" to the label of the filter field so the user is prepared that the list will be filtered based on the entry immediately.

  • 🇬🇧United Kingdom joachim

    > in the current MR as well as in Core in a few places

    Are there any existing filters in core which work OK with screen reader announcements? If so, the new script should take from those.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Hello, guys, do you want me to go over the code and try to remove jQuery, switching it to a pure Vanilla JS approach?

    On this ticket 📌 Improve the performance of the script delivering the permissions search filter functionality in admin/people/permissions. Needs review I was able to achieve an excellent performance enhancement (affecting the user permissions page only) by getting rid of jQuery on the user permissions script (core/modules/user/user.permissions.js). The code changes are available here: https://git.drupalcode.org/project/drupal/-/merge_requests/9433/diffs.

    I am putting my thread on hold since this issue creating a Drupal filtering library is a much more promising approach.

    If I can help adjust the code to a no-jQuery approach, should I commit and submit the changes on the main branch? Or on a separate branch? Let me know what would be your recommendation.

  • 🇬🇧United Kingdom joachim

    Yes, @mabho that would be great.

    The main branch is 3028968-filter-js-library, which has my attempt at JS (I am not a front-end dev, and jQuery is all the JavaScript I know!).

    There is vanilla JS on branch 3028968- but that AFAIK is not feature-complete.

    Please feel free to make changes to the JS on the 3028968-filter-js-library branch.

  • Pipeline finished with Success
    9 months ago
    Total: 523s
    #281587
  • Pipeline finished with Failed
    9 months ago
    Total: 155s
    #281608
  • Pipeline finished with Failed
    9 months ago
    Total: 191s
    #281619
  • 🇷🇸Serbia finnsky

    1. Added `Search only from start of words` and `Min length of search query` features.
    2. Applied library for config-translation

    @joachim
    Don't get me wrong. You've done a great job here. And I've been inspired by it in many ways. But I think building a frontend library should start with JavaScript. And then adapt it to the backend. And not the other way around. So I will continuing with my approach because it's ready to use right now.

    @rkoller
    In this task we solve the problem of multiple scripts for the same task, preserving what has already been done. But without adding new features. At least not complex ones.

    If there is something that worked before and does not work now, please write here. Otherwise, let's continue working on it in future iterations of this library.

  • Status changed to Needs review 9 months ago
  • Pipeline finished with Success
    9 months ago
    Total: 1417s
    #281620
  • 🇬🇧United Kingdom joachim

    > But I think building a frontend library should start with JavaScript. And then adapt it to the backend. And not the other way around.

    I disagree. Complexity should be kept away from users of a system. Here, the users will be developers writing PHP code to apply this filtering functionality to their page. So the API is that part. I don't think there's going to be much need for extending the JS.

    I think my approach of a dedicated render element is much better DX than adding attributes. It's easier to document, it's easier to understand, and it also allows developers to control where they place the search filter box on the page.

  • 🇷🇸Serbia finnsky

    Here, the users will be developers writing PHP code to apply this filtering functionality to their page.

    Users will be any people who will filter tables in the admin panel and possibly in custom modules/themes.
    Therefore, in this task, first of all, a simple and predictable JavaScript is needed. And the backend architecture should simply support it.
    JavaScript is not a facade here, but a foundation.
    As you can see, everything works without a complex API.

    In any case, the render element can be added to my MR, and it will be much simpler, as it seems to me. So let's let the community decide how to move forward with this task.

  • 🇬🇧United Kingdom joachim

    > Users will be any people who will filter tables in the admin panel and possibly in custom modules/themes.

    Ok, yes. But my aim at least was to keep the current behaviour, because I felt it had a better chance of this issue getting in that way.

    > Therefore, in this task, first of all, a simple and predictable JavaScript is needed. And the backend architecture should simply support it.

    I still disagree. The two need to work in tandem.

    > In any case, the render element can be added to my MR, and it will be much simpler, as it seems to me.

    That would be great. I don't know enough JS to have an opinion about how it should be written! :) I would really like to keep the backend code from my MR though.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Guys, I went over the frontend part of the 'problem', and started off from @innsky's code, which doesn't mean I have any opinion on the ideal approach discussed above. I think the ideal place is somewhere in the middle: keep the backend API allowing module/core developers to provide customization and override the defaults, but provide a set of conventions that would allow users to quickly plug the filter library by using the defaults (table selector, targets, etc.); this way, if no complexity is required, someone could easily and quickly plug the library and have the filters working immediately if no customization if a more complex/custom setup is not needed.

    That said, here is the explanation on the changes I applied on the code available in branch 3028968-:

    • Use a case-insensitive (this was working already), accent insensitive search (Latin languages use accent very often; I can tell you it is very frustrating when systems don't take accents into account; I am thus 'normalizing' strings before searching
    • Instead of searching straight into the DOM elements themselves, I am setting up an array of objects to iterate through. Each array item object carries a lower-case, no-accent version of the string being searched for, as well as a reference to the corresponding DOM element being manipulated.
    • I am setting a mechanism to accelerate the queries. Imagine 200 rows to search from. Every time you type a letter, the script searches among the entire 200 rows. Usually, though, users are incrementally typing a word, of multiple search words, and we don't actually need to run a lookup on the entire 200 rows for each keystroke. Example: type string 'con' for 'content' and you are already ruling out most of the potential rows not carrying that string sequence. The next time the script is triggered (let's say, when you completed typing 'conte'), there is no need to start from the 200 records, but we can use a temporary resultset including the ~ 20 search results for 'con'; this tends to deliver good performance improvement on larger tables (not so much on smaller sets, but it still improves the speed).
    • The approach with once() is indicating that we can potentially carry more than one input filter per page. If that happens, we don't want all the variables to be duplicated (one instance per input/table). I separated that portion of the code to prevent that type of problem and keep resources usage lower in that case.
    • Last but not least, probably the most important change: we don't need to activate the filters if users are not using it; I would say, most of the times users don't use the filter when reaching the users permissions page, or modules list page. I am thus keeping the filter dormant until the filter field is focused. When that happens, then the entire mechanism is activated.

    The code is ready on my local environment. @finnsky, let me know if I can push it over your branch as it applies many changes to the original code (the original concept and structure is preserved). Feel free to review, comment and improve from there, and of course, if you don't like it, it is always possible to revert my commits.

  • 🇷🇸Serbia finnsky

    Sure please commit anything. Thank you

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    @finnsky, I am getting a message stating that I am. not allowed to push code to this project. Is this something you can grant to me?

    git push origin 3028968-
    remote: 
    remote: ========================================================================
    remote: 
    remote: You are not allowed to push code to this project.
    remote: 
    remote: ========================================================================
    remote: 
    fatal: Could not read from remote repository.
    
  • 🇺🇸United States mlncn Minneapolis, MN, USA

    mabho, if you go all the way up to the summary of this issue do you see this button? (Or search this page for "Get push access")

    Looking forward to seeing your code!

  • 🇷🇸Serbia finnsky

    I had same problem.
    But now seems fine with git and not https

  • 🇬🇧United Kingdom joachim

    > the next time the script is triggered (let's say, when you completed typing 'conte'), there is no need to start from the 200 records, but we can use a temporary resultset including the ~ 20 search results for 'con';

    This and the other improvements in #55 sound fantastic!

  • Pipeline finished with Failed
    9 months ago
    Total: 249s
    #284594
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Guys, I just pushed the code. This is how I suggest testing this:

    Add console.time('timer') and console.timeEnd('timer') to measure the time it takes to execute the event attached to the table instance. I suggest applying the same change to the original code for comparison. It can be applied exactly at this position:

          e.target.addEventListener(
            'input',
            debounce((event) => {
              tables.forEach((tableElement) => {
                console.time('timer')
                tableElement.dispatchEvent(
                  new CustomEvent(FILTER_EVENT, {
                    detail: {
                      event,
                    },
                  }),
                );
                console.timeEnd('timer')
              });
            }, 200),
          );
    

    This performance test will measure the time it takes to run each keystroke. It won't take into account the time we are saving by not activating the filter script on each page load.

    Let me know your thoughts.

  • Pipeline finished with Failed
    9 months ago
    Total: 181s
    #284612
  • 🇷🇸Serbia finnsky

    Thank you! I gonna review in next few days.
    Please for now fix linter to pass CI js linter and run functional tests.

    cd core
    yarn
    yarn run lint:core-js-passing --fix
    
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Hum, I see the code fails the pipeline, I guess I need to run Javascript code linting against the Drupal recommended standards, I will check how to do that...

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Thank you for sharing that information, @finnsky, I will try these commands.

  • Pipeline finished with Failed
    9 months ago
    Total: 488s
    #284675
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    @finnsky, @oachim: I reran the failed functional test from the UI on this page (https://git.drupalcode.org/issue/drupal-3028968/-/pipelines/284678) and all went fine with no failed tests (https://git.drupalcode.org/issue/drupal-3028968/-/jobs/2775475). I wonder if a wait() or sleep() would be recommended at some point in the test script to prevent an intermittent, false-negative result. I also ran the tests locally, and didn't bump into any problems. I assume the script is thus passing the tests.

    I am looking forward to your comments/reviews, and let me know if you have any questions, thank you!

  • Pipeline finished with Failed
    9 months ago
    #295784
  • Pipeline finished with Failed
    9 months ago
    Total: 98s
    #295796
  • Pipeline finished with Failed
    9 months ago
    Total: 104s
    #295841
  • 🇷🇸Serbia finnsky

    @mabho
    i've reviewed and tested js. all works fine. Thank you

    @joachim
    I applied render element to core/modules/system/src/Form/ModulesListForm.php only for now. Same works fine. Please review. I'm not real PHP developer. I'm sure you can do it better ;)
    If all fine we need to update other places. They still works as before.

    Few notes.

    1. I still want to keep ability to use it without render array. For example if i have twig table template and i just want to have quick filter for this. We need to think how to document it well.

    2. We need to reapply tests from 3028968-filter-js-library and add Nightwatch tests.

    Only few steps left and we are ready to go!!

  • Pipeline finished with Canceled
    9 months ago
    Total: 722s
    #295842
  • Pipeline finished with Failed
    9 months ago
    Total: 450s
    #295847
  • 🇬🇧United Kingdom joachim

    I don't see how grouping of items is being handled now.

    We have cases where:

    - items are in a DETAILS element, and if all items are hidden by the filter, then the DETAILS element should also be hidden
    - items are in a table, and there are rows which act as subheadings. If all the rows beneath a subheading row are hidden, the subheading needs to be hidden too

  • 🇷🇸Serbia finnsky

    - items are in a DETAILS element, and if all items are hidden by the filter, then the DETAILS element should also be hidden

    we care about it here https://git.drupalcode.org/project/drupal/-/merge_requests/6309/diffs#d5...

    - items are in a table, and there are rows which act as subheadings. If all the rows beneath a subheading row are hidden, the subheading needs to be hidden too

    not really sure what it means. all current Drupal filters functional controlled in js.

  • 🇬🇧United Kingdom joachim

    > not really sure what it means. all current Drupal filters functional controlled in js.

    For example, the permissions page. The area to search is a table. There are item rows, and also heading rows.

    Items belong to a heading BUT the HTML elements are NOT children!

    So when all the items under 'Block' are hidden, the 'Block' table row must be hidden too.

  • 🇷🇸Serbia finnsky

    it was added as far as i remember.
    better to test for sure.
    it was added in January :)

    100% it was added into layout builder filter.

  • 🇬🇧United Kingdom joachim

    There's nothing in the ListFilter class docs or the code about grouping.

  • 🇷🇸Serbia finnsky

    Ah yeah, you are right:
    https://git.drupalcode.org/project/drupal/-/merge_requests/6309/diffs#a6...

    But not sure what to do with it, it will not work with render element.

  • 🇬🇧United Kingdom joachim

    What do you mean that it won't work with the render element?

    I had it working on my branch, albeit with the weird library switching thing I also put in.

  • 🇷🇸Serbia finnsky

    yeah. it `may` work` with couple of extra selectors. we need to think about it.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Guys, considering the latest state of the code put forth by @finnsky, what is the filter structure (user permissions, views, modules list, fields etc.) that would require the grouping functionality mentioned above? I tested the latest version of the code on the permissions filter (/admin/people/permissions), and it seems to work alright: only titles associated with the found permission rows are activated/displayed.

  • 🇬🇧United Kingdom joachim

    There are currently three kinds of structure for filterable lists in core:

    - no grouping
    - group is an element that contains the children, e.g. details element, or a DIV
    - group is an element that precedes the children

    If you look at my branch you can see which is which, as I handled this by adding different versions of the library (which I'm sure wasn't a good way of doing it!)

    I wasn't aware of the data-filter-label etc stuff, so if that is present on all the instances of preceding grouping, then it looks like that functionality is covered! :)

  • 🇷🇸Serbia finnsky

    Btw we can dramatically simplify everything by only using CSS :has() which is in current Drupal browserlist.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    @joachim, thank you for clarifying.

    From what I have seen, I would assume group is an element that contains the children, e.g. details element, or a DIV is the remaining structure that needs to be tackled/resolved, right? It seems to me the current version is implementing the grouping functionality via group is an element that precedes the children, e.g. a table TR.

    Do any of the Drupal core structures using the filtering library rely on group is an element that contains the children, e.g. details element, or a DIV, not covered on the current code?

    My question would be: do we really need to support/handle the parent/child approach for any of the core functionality using filters?

  • 🇬🇧United Kingdom joachim

    > I guess my actual question should be: do we really need to support/handle the parent/child approach for any of the core functionality using filters?

    Yes, why shouldn't we? The goal here is a reusable common library that satisfies the use cases.

    I've updated the IS with the details of which instances are which type.

    Also, I'm not sure why the IS says this:

    > Views Add Field popup https://gyazo.com/bd957018d522cf27725c505cfa0ffe7c -- This one seems can be ignored for now. It has extra selectbox for category

    We should think about how to support extensions to the library. I don't know how that's correctly done in JS.

  • 🇷🇸Serbia finnsky

    I thought to release first version.
    With basic features. And then after tests extend it with views with selectbox etc. Otherwise we will never finish it.

  • 🇬🇧United Kingdom joachim

    > With basic features. And then after tests extend it with views with selectbox etc. Otherwise we will never finish it.

    With that I'd be wary of finding after the first iteration that the ways we need to extend it in aren't doable without breaking the API. Obviously we can mark it as @internal to get round that :D

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    My 5 cents here. You have done a great job, and clearly reached a milestone with the new code: does it deliver everything the previous equivalent, scattered filter versions were doing? The answer, as I see it, is a sounding ‘yes’.

    But there is much more. Let’s see:

    • It is preventing redundancy (multiple modules implementing the same thing) with different approaches and methods. It thus delivers consistency.
    • Users will be loading a single Javascript file for this functionality instead of multiple Javascript files (one per module). Call it a marginal improvement, but we are preventing many Javascript files from being downloaded from the server because users now depend upon a single library for that functionality.
    • The new implementation is lean and fast, probably better than all the implementations on individual modules. It saves users' resources, and, considering Drupal's scale, it will help save some trees as well.
    • Module developers willing to use a filter can use this approach instead of developing yet another custom implementation.

    I would advocate to move this thing forward immediately, and write new individual tickets to improve the code (grouping with children DOM elements; comprehensive API to help module developers extending the code) after it has been merged.

    This ticket touches several different files. The more it is delayed, greater are the chances that breaking changes on individual files will require us to analyze the code again and fix conflicts, thus consuming precious time, and delaying things even further.

    Quoting Stevie Wonder, this is 'Signed, Sealed, Delivered', what should be the next steps?

  • 🇷🇸Serbia finnsky

    Please take a look and confirm or deny

    It seems to me that there is not much difference between:

        $form['filters']['text'] = [
          '#type' => 'search',
          '#title' => $this->t('Filter modules'),
          '#title_display' => 'invisible',
          '#size' => 30,
          '#placeholder' => $this->t('Filter by name or description'),
          '#description' => $this->t('Enter a part of the module name or description'),
          '#attributes' => [
            'class' => ['table-filter-text'],
            'data-table' => '[data-drupal-selector="system-modules"]',
            'data-items' => '.package-listing table tbody tr',
            'data-targets' => '.table-filter-text-source, .module-name, .module-description',
            'data-singular' => $this->t('1 module is available in the modified list.'),
            'data-plural' => $this->t('@count modules are available in the modified list.'),
            'data-full' => $this->t('All available modules are listed.'),
            'data-search-start' => 'true',
            'autocomplete' => 'off',
          ],
        ];
    

    AND

        $form['filters']['text'] = [
          '#type' => 'list_filter',
          '#title' => $this->t('Filter modules'),
          '#title_display' => 'invisible',
          '#size' => 30,
          '#placeholder' => $this->t('Filter by name or description'),
          '#description' => $this->t('Enter a part of the module name or description'),
          '#list_container_selector' => '[data-drupal-selector="system-modules"]',
          '#list_item' => '.package-listing table tbody tr',
          '#list_text' => '.table-filter-text-source, .module-name, .module-description',
          '#search_start_of_words' => TRUE,
          '#announce' => [
            'singular' => $this->t('1 module is available in the modified list.'),
            'plural' => $this->t('@count modules are available in the modified list.'),
            'all' => $this->t('All available modules are listed.'),
          ],
          '#attributes' => [
            'autocomplete' => 'off',
          ],
        ];
    

    for a developer.

    And this render element seems to be just an extra layer. Which appeared without any real need. What does this give us?

  • 🇬🇧United Kingdom joachim

    Because that's the design of FormAPI. We abstract away from the specifics of HTML. It's the same reason we have the #options property, or the #label property and so on.

    Also, that abstraction is a protection for changes in the HTML and front-end implementation. You might want to change those data-foo keys at some point. Or there might come along a totally new paradigm for setting data on HTML elements for JavaScript to consume.

  • 🇷🇸Serbia finnsky

    You might want to change those data-foo keys at some point

    It can be managed with setting data attributes with json _encode. It exists in lot of places in core in dialog options. And read them with JSON.parse() on frontend.

              'data-dialog-options' => Json::encode([
                'width' => 555,
                'classes' => [
                  "ui-dialog" => "ui-corner-all top-1",
                ],
              ]),
    

    After we release we should never change attributes without deprecation messages.
    Technically it is still input type="search". with some extra attributes. Still need reason to have that abstract layer.

  • 🇬🇧United Kingdom joachim

    > Technically it is still input type="search". with some extra attributes

    With extra *functionality*.

    Same way we have an abstraction for type=radios, checkbox and so on.

  • 🇬🇧United Kingdom scott_euser

    Thank you all for all the hard work on this. Challenging problem and sounds like we need some sort of round table or vote or something perhaps from a slightly wider group. Is #admin-ui slack channel maybe appropriate to have it discussed as part of the next meeting to agree on a direction?

    I have been working on Improve the Search API admin UI for adding/editing fields Active which is nearly there (just coordinating final steps via slack with solr and search api maintainers). As a next step I would love to be able to leverage this as I am sure you have probably all tried search API add fields and know how unweildy that list can become. With search api getting into Drupal Cms (and AI Search leveraging that as a submodule of AI) it'll be a great improvement for both.

    So I would love to help test this out within the context of core and test extensibility of it over there (if it's not @internal), but sounds like a bit of a huddle is needed first to know which direction to test.

    In any case this is a really great chunk of work that could benefit a lot of areas I'm sure! Thank you!

  • Pipeline finished with Failed
    4 months ago
    Total: 113s
    #439932
  • Status changed to Needs work about 1 month ago
  • 🇧🇷Brazil joaopauloc.dev

    joaopauloc.dev made their first commit to this issue’s fork.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 152s
    #502083
  • Pipeline finished with Failed
    about 1 month ago
    Total: 7222s
    #502092
  • Pipeline finished with Success
    about 1 month ago
    Total: 526s
    #502246
  • Pipeline finished with Failed
    14 days ago
    Total: 347s
    #518077
  • Pipeline finished with Failed
    14 days ago
    Total: 284s
    #518100
  • Pipeline finished with Failed
    14 days ago
    Total: 142s
    #518123
  • Pipeline finished with Failed
    14 days ago
    Total: 921s
    #518128
  • Pipeline finished with Failed
    14 days ago
    Total: 519s
    #518156
  • Pipeline finished with Success
    13 days ago
    Total: 2006s
    #519074
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Hi, guys, I would like to call your attention to a very relevant update we recently completed on this thread (this is a “four-hands” effort by me and @joaopauloc.dev).

    Since we are introducing several relevant changes, we decided it would be better to fork from an earlier effort that involved @finnsky and @joachim. We introduced a new branch (3028968-new-filter-library) starting from 3028968-.

    In a nutshell, here is what we are doing here:

    • Rewrite the code as a Javascript library that can be extended. The concept is to add flexibility: a custom module can rely on this library to quickly implement a search filter using all the defaults, or it can override specific methods to achieve different results if needed (e.g.: an implementation can potentially change the behavior to make the not found rows change opacity for 0.3 instead of being hidden and disappearing).
    • It is important to say the underlying code within the `listFilter` function itself hasn’t changed much from what had been crafted by @finnsky originally with the aide of other developers. Let’s say the new approach is providing the wrappers needed to make the code more flexible and its parts overridable.
    • Following a suggestion by @joachim, we are keeping the new `list_filter` field type and standardizing its use across all occurrences covered in this ticket. We like the fact that it establishes a structured standard for setting up the parameters being passed into the script.
    • Finally, this version is introducing numerous new tests to confirm the changes being introduced work as desired/expected.

    We would love to see this solution going forward, as the new approach is preventing code repetition; introducing a solution that can be reused both by existing and new modules; improving frontent performance by activating the search filter upon user input filter focus only (thus saving browser resources) and improving search performance as well.

  • Pipeline finished with Success
    13 days ago
    Total: 746s
    #519851
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ
  • 🇬🇧United Kingdom joachim

    Thanks for getting this rolling again!

    Does this new MR handle all different types of grouping?

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    Hi, Joachim, I just tested it again to confirm. Yes, grouping is working for the structures discussed on this thread. The group name is showing both on the modules list and permissions list (each one has different grouping structures). But don't take my word for it, I encourage you to test it. The library code is mostly based on the latest version by @finnsky on fork 3028968-, and the feature you mention was already working there...

  • 🇬🇧United Kingdom joachim

    I've tested it too, and it works fine, but I'm not sure how it works -- how does UserPermissionsForm for instance specify that there's a TR that's a group header?

  • 🇫🇷France nod_ Lille

    few comments in the MR

  • Pipeline finished with Failed
    12 days ago
    #520892
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    mabho changed the visibility of the branch 3028968-filter-js-library to hidden.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    mabho changed the visibility of the branch 3028968- to hidden.

  • Pipeline finished with Failed
    12 days ago
    Total: 1776s
    #520898
  • 🇬🇧United Kingdom joachim

    I need to think of a better example than 'cake'/'ke', where the substring is also a word so it doesn't need to be added to the dictionary.

  • Pipeline finished with Failed
    11 days ago
    Total: 8344s
    #520920
  • Pipeline finished with Canceled
    11 days ago
    Total: 287s
    #521035
  • Pipeline finished with Canceled
    11 days ago
    #521038
  • Pipeline finished with Success
    11 days ago
    Total: 1349s
    #521040
  • 🇫🇷France nod_ Lille

    I like how much things we delete, really nice to see!

    It's a good start and I have some more comments.

    1. Speaking of comments there are not enough useful comments, the places where I need comments to understand what the code does, there are none.

      Declaring a function then an object member with a similar name creates 2 comments and makes the object member comment useless since we can't get the function signature (defaultSearchMethod, and listFilter.searchMethod for example) we don't know the function signature to implement if we want to override this. More on overriding below

    2. In the docs inside the JS file I'd like a list of all the data attributes used and the events triggered.
    3. The name is not great, we call that list_filter but the code and varibles names are all about table. This is not actually the list it's a seach input with filter an existing list. Something like search_filter_list list can't be the first thing in the name, the render element is not a list.
    4. There are a bunch of extension points, but they're not used. For example the only relevant one (in core) would be to override defaultSearchMethod in system.modules.js with the regex, but it's built-in the default function and controlled by an option.

      The point here is that we don't need the extensions points "just in case", this will create BC headaches down the line. This is an internal library, not a package we publish in npm so we can make the api surface as small as possible, so let's do that. The features we want to add to the library and the API surface we want to expose in Drupal core is a problem we already ran into with the autocomplete replacement work, the time it took to figure out the different priorities between the lib and core everyone was burnt-out on it and that killed the effort, let's not replicate that this time.

      So we should only add the extension points we actually need in core. If the regex is built-in the default search method, don't make it extensible. If we want to keep it extensible, let's remove the fromWordStart option.

    5. It's really old school JS :) I'd still like to separate the library code and the initialization code. Can you create a subfolder in misc and add that
    6. Seems like if there are multiple instances of the filter on the page, things gets shared. Like listFilter.matchedRows, or the two filters can't have different methods overridden, if one overrides the search it would impact the other (another reason to limit extension points). I don't see how we can have different searchingFinished callbacks on the same page. Easier with an event to deal with that.
    7. If you don't want to make a class, keep track of the various created instances on the page and all that, save the state in the DOM in some data attributes, for example matchedRows can be a data attribute, makes things simpler.
    8. I'm not saying to do it, just asking: couldn't that be a SDC?
  • 🇬🇧United Kingdom joachim

    > I need to think of a better example than 'cake'/'ke', where the substring is also a word so it doesn't need to be added to the dictionary.

    - content / tent
    - node / ode
    - taxonomy / ox
    - permission / mission

    Somebody pick one :)

  • Pipeline finished with Success
    11 days ago
    Total: 569s
    #521668
  • Pipeline finished with Failed
    10 days ago
    Total: 701s
    #521948
  • Pipeline finished with Failed
    8 days ago
    #523702
  • Pipeline finished with Failed
    7 days ago
    Total: 9327s
    #524523
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    @nod, thank you for your comments. I incorporated your suggestion to remove the transitional search array. You were right that the benefit was minimal for the additional code complexity. I tested with 2,000 dummy permissions and the performance difference was negligible.

    When looking into that I came across an important optimization oportunity, and the latest version of the code is now using requestAnimationFrame() before performing visibility changes that affect the DOM elements. The code is now performing all the hiding and showing of elements in a single shot with much better performance.

    I am looking into converting this into a class. If we tackle that task, what names do you suggest we use?

    • Keep drupal-list-filter.js as the file name for the behavior that will instantiate the class...
    • Add a new folder listFilter inside core/misc.
    • Add a single file inside it (for now): listFilter.js with the class to be instantiated.

    Finally, allow me to disagree with you regarding the semantics of the new field type: list_filter sounds good to me because it does not seem to refer to the list, but actually to the filter. It is as if you were saying The filter of the list. The search field in that context is working as a filter, and list seems to be a generic enough word to refer to the dataset.

  • 🇫🇷France nod_ Lille

    Much better thanks! the two loops and requestAnimationframes is a good call

    Seems like we lost the ability to open closed details elements during search though.

    For the class, it could even be a webcomponent now that I think about it. Some previous webcomponent work can be seen here: 📌 Use webcomponents for dropbutton Needs review

    I don't feel strongly about the naming, so list_filter is fine with me if that's what people want.

  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    @nod, there is a new branch for the class conversion of the code. I pursued the suggestion in your original comment to separate it in a different file inside a specific folder for this class/library.

    João Constantino (@joaopauloc.dev) will adjust the tests so they pass again, and then I will create a new pull request for it.

    The new code is restoring the original open/close behavior of the details groups, thank you for pointing to that missing functionality.

    I wanted to write more up-to-date Javascript, but it seems like the linter doesn't like private methods in Javascript classes (with the #methodName notation), not to mention other limitations that required me to declare these methods as statics. That said, the code seems to be working alright, all the functionality is kept, there are methods being triggered to override the class options and when the search completes.

    I will write an update here when we consider the code is ready, and the merge request is available for your review again, but feel free to share any comments you might have about the updated version.

  • Merge request !12424Convert List Filter into a class → (Open) created by mabho
  • Pipeline finished with Success
    4 days ago
    Total: 8487s
    #527259
  • Pipeline finished with Success
    4 days ago
    Total: 682s
    #527414
  • 🇧🇷Brazil mabho Rio de Janeiro, RJ

    mabho changed the visibility of the branch 3028968-new-filter-library to hidden.

  • Pipeline finished with Success
    about 17 hours ago
    Total: 507s
    #529404
Production build 0.71.5 2024