HTMLs for range filters don't conform to the HTML standards

[masasakano]

The range filter produced by Datagrid generates a HTML that violates the HTML-5 standards:

Attribute multiple not allowed on element input with type="text"

(See HTML5 standards about input for detail of the specification.)

The form in the HTML for a range filter of Datagrid tries to pass a (2-element) Array when submitted, and Rails does interpret them as an Array (and so the filter works). But the HTML standards basically do not allow such pair of fields of input with type=text.

It should be better if the generated HTML for a range filter conforms to the HTML standard.

I asked a question about it in Stackoverflow, and an answer suggests using forms of:

• my_model[my_attribute][from]
• my_model[my_attribute][to]

as opposed to the current way of multiple

• my_model[my_attribute][]

So, this may be a way to implement the (grammatically) correct pair of HTML-form fields for a range filter. Obviously, I suppose there are many other ways.

[bogdan]

I am open for the change to accept attributes values as Hash with {from: , to: } keys. Altering the default input names generation to these is definitely a good idea, but is also a breaking change. So, we need to think how we want to introduce that in a way that allows some to keep the old system.

I imagine the first step in supporting Hash to be assigned as attribute value and make sure built-in partials can be modified to support that Hash.

Besides that, I designed range filter attribute value as an array of two elements because Ruby didn't support infinite ranges (1..) at that moment. I think it needs to be changed at some point and this would be a good opportunity to do so. Obviously, it is impossible to pass any kind of range from frontend to backend because HTML forms are legacy. But I believe if you read the datagrid attribute value of a ranged filter it should return a Range, but not Hash or Array.

Let me know what you think or let me digest my own thoughts for a few days and get back.

[masasakano]

Thank you very much for taking this up!

Conceptually, I agree with you that a range filter should ideally return a Range, regardless of how it is actually dealt with in HTML forms and Rails built-in processing. Ruby 3 supports both (..5) and (5..), and so I guess it is well possible to implement it in Datagrid? Personally, I would welcome the change.

I understand such changes in Range filters are more or less backward-incompatible. A major-version upgrade in versioning may be necessary? Personally, I would accept and welcome the breaking changes with this matter.

A way to mitigate to minimise the disruption I can think of is this. This is not perfect, but I guess it should accommodate most use-cases.

I suppose two primary, potentially breaking changes for users are the default option and the block given to the filter method. The default option for filter(range: true) is expected to return a 2-element Array. So, I suppose Datagrid can be coded so it accepts both a Range and 2-element Array as the return value of the default option.

The parameter given to the block is more tricky. However, I guess most users access the given object (currently a 2-element Array) through methods of first, last, [0], [1], [-1], although few people might use other ways like values_at. If Datagrid passes to the block a Range-like object that accepts the three methods (first, last, and []), most of the existing user code using Datagrid should need no change. I note that Range accepts the method first (and last) except for begin-less (and end-less, respectively) cases. You may create a sub-class of Range and implement the methods in the sub-class, or alternatively may implement them as singleton methods for the object passed to the block.

Obviously, users may have other use-cases. In particular, their controller tests of Grid, where parameters are directly passed to GET, will fail. But a bright side is all W3C-validation tests (in Controller and System tests) that currently fail with Datagrid with Range filters will pass at last.

The above is just an idea of hopefully reducing (or minimising) the potential pain of Datagrid users associated with the breaking change. Please feel free to take it or leave it!

[bogdan]

I understand such changes in Range filters are more or less backward-incompatible. A major-version upgrade in versioning may be necessary? Personally, I would accept and welcome the breaking changes with this matter.

Phase 1 - minor version release:

Here is how it is usually done: we introduce a new configuration parameter new_ranges_behaviour which can be nil(default), false and true.
false and nil would make ranges behave the old way.
true converts them to new way.
When it is not set (aka nil) it triggers a warning on app boot that this new configuration parameter needs to be set as well as the link to instructions.

Additional optional convenience would be to the ability to override a global configuraiton on per filter basis with the same parameter name. It would help larger projects with gradual migration.

Phase 2 - major version release:

We make the parameter default to true and add a warning for both nil and false that the migration is needed.

Phase 3 - minor version release:

We remove the parameter, saying instead that it is deprecated and new behaviour is enforced.

In particular, their controller tests of Grid, where parameters are directly passed to GET, will fail

I suppose two primary, potentially breaking changes for users are the default option and the block given to the filter method. The default option for filter(range: true) is expected to return a 2-element Array. So, I suppose Datagrid can be coded so it accepts both a Range and 2-element Array as the return value of the default option.

Good point.

I believe we should maintain the ability to assign a range attribute as a 2 elements array. I don't see any problems with that.
Potentially it can be deprecated later, but I see not much reason for that. To me this functionality is built-in typecast: the same as when you would assign a "1" to an integer attribute either ActiveRecord or Datagrid - very helpful.

The other problem I see is serialization: in current version if you put datagrid attributes to database (like Job Queue for CSV export), it would serialize to JSON and deserialize normally. With ranges it would break:

> ActiveSupport::JSON.load((1..2).to_json)
=> "1..2"

So I believe, to make it smoother, we need the ability to assign range in the string format as well.

The parameter given to the block is more tricky. However, I guess most users access the given object (currently a 2-element Array) through methods of first, last, [0], [1], [-1], although few people might use other ways like values_at.

Yeah that is a problem.

If Datagrid passes to the block a Range-like object that accepts the three methods (first, last, and []), most of the existing user code using Datagrid should need no change. I note that Range accepts the method first (and last) except for begin-less (and end-less, respectively) cases. You may create a sub-class of Range and implement the methods in the sub-class, or alternatively may implement them as singleton methods for the object passed to the block.

That seems too crazy: changing built-in object behavior even in a scope of the library doesn't look good to me.

[masasakano]

The proposed procedure of phases 1--3 sounds definitely ideal!
(if some extra amount of work is needed for developers)

If you introduce a configuration parameter new_ranges_behaviour and if it is accepted also as an optional parameter in each filter, then the block parameter in the associated block to each filter can (and perhaps should) reflect it. That is, if new_ranges_behaviour is nil or false, the block parameter is an Array as it has been, and if it is true, it is a Range. For example,

filter(:year, :integer, range: true, new_ranges_behaviour: false){ |record| }
  # record is a 2-element Array.
filter(:year, :integer, range: true, new_ranges_behaviour: true){ |record| }
  # record is a Range.

This seems to me the cleanest way with the 3-phase gradual migration.

I didn't think of serialization… Very good point!

[bogdan]

@masasakano I've addressed all the issues with discussed here.
I would apreciate if you try #328 version-2 branch in your project and provide feedback on how difficult is the upgrade and weather it solves all your issues.

[masasakano]

@bogdan Marvellous!! Thank you so much. You have adressed all the issues in the best way possible and modernized DataGrid very well!

I have trid the branch (ff5193f) in my project, and here is my feedback. Specifically, my environment was Ruby-3.1.2, Rails-7.0.8, and I upgraded Datagrid from 1.7.0 (as in the Gem version) to the version-2 branch.

First of all, once I have followed the instrucition on Github of upgrading to Ver.2, including changing the base class model from BaseGrid to ApplicationGrid, pretty much everything, incluring Range filters for Integer, work very well (except for the date filter, which I describe below). Migration worked like a charm.

Admittedly, there are a fair amount of points to upgrade, especially the test suite, as CSS class names have changed. But I very much understand that is a necessary price to pay to modernize, and I am happy to follow it.

minor points

• New I18n-Translation of **.datagrid.filters.range.separator was newly introduced? It may be worth mentioning in the guide in case users use a different locale from English. Depending on the user's setting, overlooking it may result in something undesirable, including a complete failure with unprocessable entity.
  • I understand it appears in https://github.com/bogdan/datagrid/blob/version-2/version-2/views.diff but asking users to have a look may be too much...
• CSS Class for the Datagrid table tag used to contain "<SNAKE_CASE_PLURAL_MODEL_NAME>_grid", for example, articles_grid for the Article class, but no more. Unlike other CSS classes that are renamed to data-column etc, it has completely disappeard. Maybe worth mentioning in the doc?
• In the Manual, this sentence does not make sense: "Serialization/Deserialization of Range is help correctly"

significant point

• The Range filter fails in validator.w3.org with an error message of

  The value of the “for” attribute of the “label” element must be the ID of a non-hidden form control.

  • A Range filter creates something like this, where the for attribute contradicts the name attributes:

    <label for="musics_grid_year">Year</label>
    <input step="1" class="datagrid-range-from" name="musics_grid[year][from]" type="number">
    <span class="datagrid-range-separator">〜</span>
    <input step="1" class="datagrid-range-to" name="musics_grid[year][to]" type="number">

  • Answers for this Stackoverflow question mention a few potential measures.

major points

An integer Range filter works fine and needs no extra work in default in upgrading to Ver.2; for example, this simple filter needed no work: filter(:birth_year, :integer, range: true)

However, I found the date range filter does not work anymore, filter(:release_date, :date, range: true), in the sense inputting a value(s) for the filter makes no effect on filtering. It seems the filter creates a correct (I mean, expected) HTML and so the correct GET request is made when submitted. Here is an example for the release_date column in Article model:

• HTML: <input class="datagrid-range-from" value="" name="articles_grid[release_date][from]" type="date">
• GET request (part): &articles_grid[release_date][from]=2024-08-12&articles_grid[release_date][to]=2024-09-05

For your reference, this is the output from Datagrid Ver.1, which worked as a filter as expected:

• HTML: <input class="release_date date_filter from" multiple="multiple" type="text" name="articles_grid[release_date][]">
• GET request (part): &articles_grid[release_date][]=2024-08-12&articles_grid[release_date][]=2024-09-05

I notice that the filter columns are now in version-2 equipped with the GUI date picker. After submitting with the Range filter with values, the range filter fields in the refreshed screen revert to NULL, the initial values, in version-2 unlike all the other fields and selections, which keep the submitted filter values as expected.

I figure out /lib/datagrid/filters/date_filter.rb seems to contain bugs; the substitution statements to value do nothing in the code. I am attaching a patch for it. Mind you, I still haven't made it work right... It may be because the fix is insufficient, or possibly I have yet failed to apply the patched code to my environment (I haven't checked the integration properly...).

date_filter.masa.20241113.patch

That is it for now.

Thank you so much again for putting effort to this and modernizing DataGrid. I am extremely grateful!

[bogdan]

New I18n-Translation of **.datagrid.filters.range.separator was newly introduced

Updated.

Serialization/Deserialization of Range is help correctly

Typo: help => held

CSS Class for the Datagrid table tag used to contain

Mentioned.

The Range filter fails in validator.w3.org with an error message of

Very good point. I've addressed it making sure label[for] for range filter matches the "from" input[id] so that the click on it results in first input of the two being focused. Second input will not have id by default.

I found the date range filter does not work anymore

I can not reproduce that.
Please try the following in console and show the output:

require 'datagrid/version'

class TestGrid < ApplicationGrid
  scope { Article }
  filter(:release_date, :date, range: true)
end

g = TestGrid.new(release_date: {from: '2024-08-12', to: '2024-09-05'})
puts Datagrid::VERSION
puts g.release_date.inspect
puts g.assets.to_sql

[masasakano]

Thank you for your update and thorough response! Here is my feedback, including new points.
I have confirmed all the points that you mentioned are now fixed or improved.

(another) W3C validation failure

When you specify column_names_filter(), the <label> tag ("Column names" in default) for the set of checkboxes does not pass the W3C validation:

The value of the for attribute of the label element must be the ID of a non-hidden form control.

I note each <input> tag for a checkbox in the column filter has an ID attribute and correspondent label, which is perfect. But the (only) label for the entire set of choices has no correspondent ID, hence the error.

doc

In README,

Introduced range filter separator localization

A separator symbol between range filter inputs is now a part of localizations to avoid hardcore.

Did you mean "hard-coding" instead of "hardcore"?

date-filter

Let me summarize first. In a word, my assessment was found to be wrong, and Range filters for Date actually work fine! Here is the detail.

First of all, your suggestion of producing outputs was most helpful, and I greatly appreciate it! Here is the output (with the latest commit d488792), which actually looks fine. I put it here for record.

irb> require 'datagrid/version'
irb> class TestGrid < ApplicationGrid
irb*   scope { Article }
irb*   filter(:release_date, :date, range: true)
irb> end
=> #<Datagrid::Filters::DateFilter:0x000000010df51258 @block=nil, @grid_class=TestGrid(release_date: date), @name=:release_date, @options={:range=>true}>
irb> g = TestGrid.new(release_date: {from: '2024-08-12', to: '2024-09-05'})
=> #<TestGrid order: nil, descending: nil, release_date: Mon, 12 Aug 2024..Thu, 05 Sep 2024>
irb> puts Datagrid::VERSION
2.0.0
irb> puts g.release_date.inspect
Mon, 12 Aug 2024..Thu, 05 Sep 2024
irb> puts g.assets.to_sql
SELECT "articles".* FROM "articles" WHERE (articles.release_date >= '2024-08-12') AND (articles.release_date <= '2024-09-05')
irb>

This (= the fact it seems to be working OK) prompted me to make further investigation, and I found my mistake... Basically, none of the Range filters were working(!) for my app, but I wrongly asserted some of them were working and only date-filter was not working, partly because my system tests accidentally passed (as the result of the conditions being met unintentionally for the specific situation).

I dug deeper and found that the cause of the filter not working AND the cause of the input values not appearing in the corresponding fields on the refreshed screen after submission were both related to the handling of Rails params and its permission in my application.

In the end, I successfully made it (Datagrid Range filters) work with this:

 # In Controller:
@grid = ArticlesGrid.new(params[:articles_grid].merge(order: :release_date, descending: true)) do |scope|

 # In View:
datagrid_table @grid 

For your information, what I was caught and ended up spending some time was the following.
This was my original code in Controller.

prms = params[:articles_grid].permit!
@grid = ArticlesGrid.new(order: :release_date, **prms)

I found this did not work with the new Range filters. I had to modify it as follows, basically needing to explicitly convert params to a Ruby Hash:

@grid = ArticlesGrid.new(order: :release_date, **(prms.to_h))

In the official doc of DataGrid, the given example Hash#merge the extra parameters to params as the main argument as in my first (working) example above. I was not aware of it, and instead I used to pass everything as optional parameters to ApplicationGrid.new. My way used to work fine in Version-1 but not in Version-2 for whatever reason.

This may be just me, but I mention it in case you are interested... (To be honest, I don't think you would need any modifications because of this potential.)

a little suggestion for a piece of code

The apply method in /lib/datagrid/filters/date_filter.rb (in the lates commit d488792) surely does nothing? It always simply returns super because a potential change in the local variable value affects nothing outside the method. Seems it is related only to a timestamp Range, though, which I haven't had a look at.

def apply(grid_object, scope, value)
  if driver.timestamp_column?(scope, name)
    value = Datagrid::Utils.format_date_as_timestamp(value)
  end
  super
end

---

That's it. Thank you again!