add hide_by_default attribute option

[Bubballoo3]

Fixes #3346 by passing a boolean hideByDefault parameter to addHideHandler and updateVisibility. Due to the caching, this will only allow one of either data-hide or data-show actions to apply to an trigger element, automatically avoiding conflicts by using the first one passed.

There is a slight issue where if the data-show directive is not on the initial setting for the trigger element, the hidden element flashes briefly on the form before it is hidden. The only way to fix this is to have it be hidden by default on html render, which would require parsing the implications of form.yml server-side. I don't think this is a bad idea, and could be a good way to check form viability and warn if any dynamic form conventions are not being followed (eg. data-hide and data-show in same trigger element, data-label not set on all options, etc) but will be a much greater change that should likely be a PR of its own.

Testing for data-show will be added shortly, but wanted to get this up to discuss the previous point.

[Bubballoo3]

It should also be noted when this is added to the docs that this is only supported on select trigger elements, since data-show-when-checked would be equivalent to data-hide-when-unchecked, and we should prefer data-hide

[johrstrom]

Looking at this expression after all this time - does data-hide-something: false just work here?

[johrstrom]

There is a slight issue where if the data-show directive is not on the initial setting for the trigger element, the hidden element flashes briefly on the form before it is hidden.

Without pulling this down, just reading through it, I wonder if it solves the original issue. The OP mentions adding a hide_by_default attribute to set the initial page. I wonder if that coupled with data-hide-something: false will solve this if something has hide_by_default: true.

[Bubballoo3]

I wonder if that coupled with data-hide-something: false will solve this if something has hide_by_default: true.

That certainly would be a direction we could take this, I guess it depends how much value we see in making a server-side form processing component. Robin mentions that automatic detection would be preferable, but if we want to stop this from launching other significant changes, the hide_by_default and data-hide-something: false combination would work.

[robinkar]

Thank you for this. I just pulled this down and tested it a bit. Functionality wise, this seems to do what it needs to do in our use case.
Both this, and hide_by_default option would be perfectly fine for the original issue. The hide_by_default option was mostly an idea in case it would simplify implementation or make it easier to maintain (explicit > implicit in some cases).
The hidden element flashing would be nice to get rid of, but it's not too big of a concern (we have some other flashing elements already).

[Bubballoo3]

The hide_by_default approach should be implemented now, it took a lot of little tweaks to get the wrapper div hidden properly, probably a sign that we would want to refactor this to get a single form.form_group wrapper for every widget. The current approach converts the hide_by_default: true attribute into wrapper: style: 'display: none' which hides the correct wrapper for the basic bootstrap form items. The custom items all had to implement this themselves to some degree. As described above, this uses data-hide-something: false in place of data-show

[johrstrom]

We shouldn't apply inline styles. Can we just add the d-none class?

[Bubballoo3]

It has to do with the way data-hide actually hides the elements, which is by going in and adding the inline display:none style through the jquery hide(). I also explored another option (seen in the commented out css) to do .mb-3:has([hide_by_default=true]) which works more cleanly but has some browser support concerns detailed http://caniuse.com/css-has. Since jquery show() just removes the inline style, I worried about consistency and if some things would remain hidden. So this comes as close as possible to what the dynamic forms create after hiding, to ensure that the same javascript can show it later without issues. I can explore the class option though and report back

[johrstrom]

That makes sense, no need to investigate I think we likely have to follow jQuery to be compatible with everything else.

[Bubballoo3]

Yeah I just tested real quick and d-none breaks as I expected. The CSS is still a valid option though, do we have a set list of browsers we support? It looks like :has is standard post-2022/23 but display:none isn't going anywhere and is about as safe as you get.

Still writing tests as well so those should be coming soon

[johrstrom]

I'm not in love with wrappers in this and IIRC another PR. Can we just embed a data-hide-by-default: true attribute in the existing smart attribute and it's resulting HTML?

[Bubballoo3]

Testing finished with coverage over all the form widgets, so should be ready to merge after a final review.

I will say I am a little concerned about the size of test/system/batch_connect_test (2400 lines after these changes, with even more in #4611), and may consider opening an issue to organize it better and separate off the dynamic forms actions specifically. This would also help us make sure dynamic forms behaviors have tests for all the widgets, because at the moment this causes a significant interruption to the pace of the other tests.

[Bubballoo3]

I think the issue is that the widgets are very flexible as far as how much we rely on bootstrap-form vs our own code. Ideally we don't have to manually handle the wrapper key nearly this much, but for the ones that we don't define the form.form-group the wrapper key in the passed hash is the only way. However if we define the form-groups ourselves, as is the case with checkboxes and radios, we have to also mimic how bootstrap-form would handle the wrapper option.

The easiest fix IMO would be to add a single universal wrapper on top of every widget with class visibility_scope_{attrib.id} (or something like that), and then control that instead for the data-hide activity. That way we have consistency across widgets, full control over the element being hidden, and don't have to handle all these settings slightly differently for all 9 widget types.

This would require changing the operation of data-hide though, and may work better as a future refactor

[Bubballoo3]

Can we just embed a data-hide-by-default: true attribute in the existing smart attribute and it's resulting HTML?

The issue is getting the attribute into the right location in the html, since the element we need to hide isn't always defined by us. The hide_by_default: true attribute is included in the field_options, and does work it's way into the HTML, and is used for detecting the setting in the Javascript. The problem is converting that attribute into proper display and functionality, which is what the whole wrapper mess solves.

[johrstrom]

I feel like this is on hold due to the need to refactor. Is that right? If so, I'd say let's just close it because it'll be open for some time then likely needs a major conflict resolution.

[Bubballoo3]

It is very well tested against all the widgets, and there is only one conflict so far and it shouldn't be too bad I don't think (all of the unwanted complexity is server-side not javascript, which we have had to mess with much less). It definitely is something that will be cleaner once we get to that refactor, but if it works and is well tested, I think we should get it out so people can start using it, and then clean up the implementation later. We could still push it back if we run into any major issues though.

[johrstrom]

I think we should get it out so people can start using it, and then clean up the implementation later.

What's worrisome to me is the updates in app/helpers/batch_connect/session_contexts_helper.rb. I feel like it's going to be hard to revert all of these.

I agree with the sentiment that this could be a nice addition, but I just get the sense that it'll be hard to extract and revert. At least cleanly.

Now I am in-fact wondering if we should to do the refactor now or at least consider it. I wonder how long it'd take? It seems like we just need to wrap every element which seems easy enough. New features like this can utilize it and older features can continue to do what they do.

Can you take a couple hours to hack and see if it's easy & simple to just do now while maintaining compatibility with the existing features?

[Bubballoo3]

I think doing the full refactoring, which to do right should involve looking into all 19 extant issues in #2476 and how we can make those things easier at all levels, is best reserved as a task of its own. I can definitely look into ways we can make this closer to what we think the refactoring will be.

My first impression is that I do think we could wrap each widget with hide-specific divs (maybe incorporate the widget info divs from #4594), and that would clean up the session_contexts_helper quite a bit. However this comes at the expense of HTML simplicity (slightly) and will require a more complex implementation for hiding elements than the built in jquery method.

The only other downside I see is that we may need to get rid of bootstrap-form for accessibility soon, and since working around the inconsistencies of bootstrap-form and the custom widgets is the real pain point of these changes, we may end up building our own widget helper functionality out to do what we want bootstrap-form to do (reading a hash of wrapper attributes and applying this consistently). This would allow us to handle every widget both even-handedly and cleanly. If we went that direction every widget would look like the select widget does in these changes, and it would all be pretty straightforward.

Anyway just my first impressions of the problem, I will take a closer look and see how the wrapper approach looks. Assuming no major complications, I'll add those changes to this PR after I resolve the current test conflicts.

[johrstrom]

I'll add those changes to this PR after I resolve the current test conflicts.

I'd really prefer a separate PR. It's very hard to evaluate changes when multiple things are happening.