🇧🇷Brazil @mabho

I am glad to see the latest feedback on this ticket. I will review the comments as soon as possible to identify potential and required changes.

Hey, do you have any plans to release the Drupal 11 version? Apparently, the only requirement is applying the change to the info file.

mabho → created an issue.

This is now ready for you again, @nod. Thank you!

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

@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.

@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.

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

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

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...

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.

@vishal.kadam,

Ok, done, I merged the changes in the issue queue into the main branch (1.0.x). It is ready for your review now.

Many thanks

Changes are now ready.

I removed branches main and feature/add-module-tests.

Here is the merge request to incorporate the changes you requested in the README.md:
https://git.drupalcode.org/project/partial_datelist/-/merge_requests/9

I am also incorporating a .gitignore file to the codebase.

I have this open thread where I plan to apply your change requests:
https://www.drupal.org/project/partial_datelist/issues/3525354 ✨ Changes for Drupal.org security advisory review preparation Active

Updated the issue status to reflect the fact that changes are actively being applied here.

@avpaderno,

This module contains only code I have written. GitLab CI is enabled and the code passed validation.

@vishal.kadam,

Thank you for the quick response here, I will review your requests and get back to you once it is ready.

mabho → created an issue.

Completed several improvements to complete the security advisory process.

mabho → created an issue.

All the pipeline tests were validated, and this task is considered to be complete now.

mabho → created an issue.

@ bertb76,

I appreciate you opening this ticket. I also installed the module under a Drupal 10 website, and can't reproduce the problem you are describing here. For now, I am changing the status of this ticket to Postponed )maintainer needs more info). Feel free to reopen it when you have additional information to provide.

Closing this ticket again after applying a PHPCBF fix.

Reopening to fix a PHPCS message introduced when working on this ticket.

mabho → created an issue.

mabho → created an issue.

mabho → created an issue.

Completed the work on this ticket and delivered the functional tests.

mabho → created an issue.

Bert, I don't see how is it possible for this module to cause the issues you are describing.

Please provide me with some additional data on the tests you are performing:

• What is the Drupal version you are using in your test (major and minor version, like 11.0.9)?
• Do you know what is the theme being used in the form instance you are mentioning?
• Is the problem you are raising associated with a node form, or is it another entity?
• Is this entity in a context of being added or edited?
• Is this field limited to one instance, or multiple instances?

Now, some questions associated with your particular testcase description:

• You mention date-list element in this sentence: "After installing date-list element (...)". The module's name is partial_dateist. Just to be on the safe side: are you sure you installed partial_dateist?
• When you say "add element to form", do you mean you created a date or datetime field?
• What do you mean by "Select Date part and order"? Did you open the settings form associated with the date field on this page admin/structure/types/manage/[date-type]/form-display? In case you did, did you save the form display at the end of the page?
• What date parts did you decide to hide? Was it the day only, leaving Year and Month unchecked?
• When you mention "Set Title Display and Description to invisible", do you mean that you moved both the Title and Description fields to the Disabled section of the form display?
• Aren't you adding two different date fields in your form?
• Can you provide me with a screen capture of your form configuration on this page (admin/structure/types/manage/[date-type]/form-display)?
• If you simply uninstall the module, does the problem persist or go away entirely?

Please, provide me this additional information so I can get more perspective on the issue you are documenting here. As I said before,

Thank you!

After creating this ticket I noticed there is an option Allow HTML in texts inside the Styling tab. Since this delivers the ability to add custom links within the text, it delivers the functionality I need. I am thus closing this ticket.

mabho → created an issue.

I am closing this as the information on the recommended Google Analytics approach is revealed in the README.md module documentation file.

Oh, perfect, thank you so much, I am sorry I didn't look enough at the module's documentation file.

Thanks, Jan, this is the link to the Google Analytics module I am using: https://www.drupal.org/project/google_analytics.

Do you suggest switching into a different GA module that is potentially compliant with Klaro?

mabho → created an issue.

mabho → created an issue.

This is a simple label fix, but I think it is relevant for coherence. The output of each instance corresponds to a single Font Awesome icon, not multiple; the label should reflect that, and be written in the singular, not plural.

mabho → created an issue.

This error is showing up to me when I set something that Drupal probably sees as an invalid value.

In order to reproduce it, please try this:

• Navigate to /admin/structure/events/series/settings
• Update the value of Event Series Minimum Time to something in the 0 hour range (e.g.: 00:00am or 00:30am).
• Save the settings form.
• Navigate to /events/add/default.
• Check the WSOD being displayed with the exact message on this thread.

It appears 0:00am is not a valid format; apparently I should use 12:00am instead. I changed the configuration on /admin/structure/events/series/settings to 12:00am and it resumed working, now the starting time on the range is midnight (0 hour). Ok, I wonder if there should be a form validation to confirm the date entered by the user in Event Series Minimum Time is a valid format to be processed by Drupal\Component\Datetime\DateTimePlus::createFromFormat() or not.

Actually, the problem still occurs for me. But it is intermittent, and I can't determine the cause.

The fix I mentioned on #90 was helpful, it dramatically decreased its frequency, but the problem is it still happens.

The same page might fail in production, and still work in DEV or STAGE, which makes things very hard to debug.

I will post here if I find a solution, or if I can get a new insight for a solution.

I am currently experiencing the exact same issue @mibstar mentioned above (#88) on a website. External files hosted in Adobe Fonts' Typekit and Google Fonts were triggering the problem.

fonts:
  css:
    theme:
      https://use.typekit.net/[css-file-path]:
        type: external
      https://fonts.googleapis.com/css2[font-params]:
        type: external

I moved the references to these external assets into @import statements within the CSS and swerved the problem without actually resolving the underlying cause.

The fact that the <figure /> tag is showing within CKEditor5, and not outside it (when the image is rendered on the page) seems to be the problem to me. My take on this is: when the image is rendered, no matter what, the <figure /> wrapper should be applied. I wonder what would be the unintended consequences of such a change on legacy websites...

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?

mabho → created an issue.

@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?

mabho → created an issue.

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.

@chris burge: yes, I did clear cache, but the code is still broken after that.

mabho → created an issue.

Sure thing, @aaronchristian, I am happy you agree with the solution, and that I could contribute back to the module's codebase :-)

After applying the DIFF made available here (https://git.drupalcode.org/project/oembed_providers/-/merge_requests/25....), I get this exception message:

The website encountered an unexpected error. Try again later.

Error: Class "Drupal\oembed_providers\Helper" not found in oembed_providers_form_media_type_edit_form_alter() (line 61 of modules/contrib/oembed_providers/oembed_providers.module).
Drupal\Core\Extension\ModuleHandler->alter('form', Array, Object, 'media_type_edit_form') (Line: 834)
Drupal\Core\Form\FormBuilder->prepareForm('media_type_edit_form', Array, Object) (Line: 285)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 73)
Drupal\Core\Controller\FormController->getContentResult(Object, Object) (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult(Object, Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 638)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 181)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 53)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 50)
Drupal\ban\BanMiddleware->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 741)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

Here is a static patch file out of the changes delivered in commit 2a5f14b79fd68d6c2d92c96eaffad63728558d0a.

mabho → created an issue.

@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!

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

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...

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.

I moved my activity into issue thread ✨ Create Javascript library for searching rendered lists on the client. Active because it is proposing a single library to be used everywhere a filter is needed. It promotes code reuse and provides a pluggable mechanism that can more easily be reused.

@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.

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.

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.

My only concern about this permissions table is its mechanism to hide/show the parent of the found record (in this case, the module name associated with the found permission). I wonder if the implementation being carried out here (https://git.drupalcode.org/project/drupal/-/merge_requests/6309/diffs#58...) is taking care of this. This is speculation, though, and I need to look into that implementation.

Thank you @_nod and @nicxvan. I can see that initiative is pretty advanced and removes the dependency on jQuery... Maybe the suggested approach here (taking advantage of a subset of the records being searched based on the text previously typed) can be ported into that script for improved performance... or not. I will take a look at it.

Real-time commenting and adjusting ;-) Just fixed the typo, many thanks for the quick feedback!

I am introducing some enhancements with the purpose of making the code run faster.

First things first, removing jQuery delivers relevant performance enhancement. It is amazing how slow the code can get on some operations triggered by jQuery. This becomes more dramatic on a script being activate on each keystroke by the client.

Apart from that, the code is also introducing transitional variables storing information on the most recent text typed and sample search results extracted. By comparing the previous search string with the current, the program can determine if the user is incrementally typing a word or phrase. If so, it will incrementally run the new row filtering from the sample results on the previous round, instead of doing it from scratch every time.

This dramatically reduces the number of rows the program needs to analyze, thus improving performance.

A merge request is already available.