data-blacklight-modal=preserve doesn't work for forms

[ebenenglish]

In app/javascript/blacklight/modal.js, the documentation says:

Link or forms inside the modal will ordinarily cause page loads when they are triggered. However, if you'd like their results to stay within the modal, just add data-blacklight-modal="preserve" to the link or form.

This works fine for links, but it doesn't work for form submission. When a form is submitted, a page load is triggered.

This seems to be because the JS references Blacklight.modal.preserveFormSelector when trying to set the event handler for form submission:

https://github.com/projectblacklight/blacklight/blob/master/app/javascript/blacklight/modal.js#L173-L174

$('body').on('submit', Blacklight.modal.triggerFormSelector + ', ' + Blacklight.modal.preserveFormSelector,
    Blacklight.modal.modalAjaxFormSubmit);

But Blacklight.modal.preserveFormSelector is never defined in the code.

This should be an easy fix, just need to add a line like ...

Blacklight.modal.preserveFormSelector = Blacklight.modal.modalSelector + ' form[data-blacklight-modal~=preserve]';

... to the section where selectors are defined (L80-L99).

(PS. I'm unsure of whether this change would need to be replicated in app/assets/javascripts/blacklight/blacklight.js or whether there is some automated compilation?)

[jcoyne]

Is blacklight itself ever using this behavior? If not, maybe it should be for downstream applications to handle and we can remove preserveFormSelector entirely.

[ebenenglish]

As far as I know this behavior isn't actually used by Blacklight. However, this used to work on 6.*, so I'd call this a regression.

https://github.com/projectblacklight/blacklight/blob/release-6.x/app/assets/javascripts/blacklight/ajax_modal.js#L99

I'm not sure when/why that line got removed (harder to parse history since the file has been renamed a few times).

If we remove this, then the documentation needs to be updated to reflect the removal.

But I'd be in favor of keeping it. It's a useful feature to provide, and it doesn't add a huge amount of overhead to Blacklight to keep it? I've been using it in our app (Digital Commonwealth) for several years, as part of a "search inside this book" feature for items with OCR text.

[jcoyne]

It's hard to support a feature that we can't write a test for. We can't test it because blacklight itself doesn't use it. I believe this feature was intentionally removed in version 7 intentionally, but wasn't completely done.

[ebenenglish]

I can't really argue with that point (hard to support feature that can't be tested).

I guess my only argument is that this feature has been part of Blacklight for a long time (since 2014, it looks like), it's a nice added feature, the lack of a test doesn't represent a major flaw that prevents efficient development/implementation, and removing it represents a regression that is potentially disruptive for adopters attempting to upgrade to BL 7.*.

If that doesn't convince you, that's OK, I understand where you're coming from. Let's update the docs and remove all references to preserveFormSelector in that case.