feat: pass extra alert_message keyword arguments through to div as attributes

[G-Rath]

This makes it possible to customize the alert message div, such as to add attributes for accessibility

[karaaslanz]

Small review note: this changes the rendered alert_message markup, but the existing alert_message assertions in test/bootstrap_form_test.rb still expect the

without tabindex. assert_equivalent_html compares the full Nokogiri fragments and its only loose exception is data-disable-with, so the current expectations should be updated and/or a focused regression should explicitly assert tabindex="-1".
That would both protect the intended accessibility behavior and make the change self-documenting in the test suite.

[lcreid]

@G-Rath I'm afraid I haven't dug into accesibility enough to understand why your fix fixes the problem. In fact, I don't understand how the div is even taking focus, as it doesn't have a control in it. Or am I missing something?

[G-Rath]

Focus is important for those using assistive technology such as screen readers and keyboard-only navigators, as it relates to changes on a page.

For example, consider submitting a form with a validation error - that would take you back to the form with an alert message summarizing the errors. To a non-sighted person if there's no change-in-focus they'll be told a generic "you're on <page you were already on>", and they'll have to hunt around for the alert message.

The ideal is when the page loads the focus is put on the form error summary, so the non-sighted person is told "focus is on: ".

Doing this requires making the alert focused, and to do that it has to be focusable, and you've said by default div elements cannot be focused at all - which is what tabindex: "-1" enables.

Note this does not put the element in the tab order, it just means that you can programmatically call focus on it.

As a more complete example, I'm working with the NZ govt on a "SaaS" version of the CWAC accessibility checker; we have a snippet of JS that looks for bootstrap alerts and calls focus on the first one on the page.

You can see this in action on our sign in page (just use any username + password, its a closed beta thing right now), where the "that email or password is incorrect" alert will get focused when the page is loaded, and once it loses focus you cannot tab it back into focus.

[G-Rath]

Alternatively (and tbh probably the better solution here) I could add generic support for e.g. wrapper, to allow you to apply any attribute to the element.

That would actually us remove our patch completely because we're also needing to set role="note" on the message, but I didn't do that here as my understanding its that is far more situational whereas this change should always be appropriate given this is an "alert message"

[lcreid]

@G-Rath Thanks for the excelent and clear explanation. You joined the dots nicely.

I like your idea of a more general purpose mechanism. Let me think about it a bit more.

[G-Rath]

I liked it at first, but then realized it'd mean we would have to pass these options through on every form 😅 maybe some kind of global default option initializer thingy..?

Just to reiterate a bit of the above: this particular change should be good to land imo because ultimately however it's styled, it is meant to be an alert and that an alert is something that should be expected to be capable of receiving focus programmatically, which is all this particular enables

[lcreid]

I get what you're saying that your original fix should be okay, but in the name of backwards compatibility, I think other solutions (like the idea you floated above) might win out.

Your points are valid. Let me speak to them one at a time:

• Suddenly adding tabindex to something that's been in the output of bootstrap_form for years might break someone else's form design. Maybe if we (you) just splat any keyword arguments to alert_message onto the div, we'd have a nice, flexible, and backwards-compatible way for one to add whatever they want to the alert_message.
• I absolutely agree with you that it's a pain to have to add something like tabindex: -1 to every place you want it, when it's really a global standard of your UI/UX. I've been feeling for a while that bootstrap_form really needs some way to define some options at the form, and/or, application level.

Would you be interested in modifying your PR to do something like what I said in the first bullet, add at least one test case, and maybe add something to the README page to give people a hint about how to provide a better ARIA experience?

Then, if you're keen, in another PR propose a mechanism to make this a form-level, or application level configuration option? I'd be happy to discuss this part with you, if you want to take it on. I might even do it myself if you don't want to take it on, although if you punt it to me I'd sure appreciate your feedback on whatever I come up with. But to be clear, I'm more than happy to see what you would propose.

How does that sound?

[lcreid]

P.S. I deliberately made some of my previous comments at a pretty high level. If you have questions or want more detail, just ask!

[G-Rath]

yeah I'm happy to do that - I think in a library like this making it generic is the right move anyway :)

[G-Rath]

@lcreid let me know what you think - happy to add more like an example to the README, but this should be the core change

[lcreid]

This looks great. Thanks! I think it does merit being highlighted in the README, so if you don't mind adding something there, I'd appreciate it. I won't mind if you just piggy-back it on the existing alert_message helper documentation, but if you're keen you can make a separate example. That way you can highlight your contribution a bit better. 😄

[lcreid]

🔥

[G-Rath]

@lcreid done, though I didn't add a screenshot as it didn't feel useful given there's no visual difference - happy to fold into the existing example if you'd prefer

[lcreid]

I may know what those test failures are, but I might not get them fixed tonight. If you figure it out, feel free to add the fix to your PR.

[lcreid]

If you rebase on the upstream, the tests should pass.

[G-Rath]

@lcreid done!

[lcreid]

I was afraid we might have an issue with how the screenshots and the examples work. I didn't actually set that up, so I wasn't sure what would happen. Otherwise, everything looks great in the PR. Thanks again!

If you can figure out how to satisfy CI, feel free to update the PR. Otherwise, I'll try to find time this weekend to take a look. I would probably just merge your PR with failing tests and then fix up CI in another PR.

[G-Rath]

@lcreid I can just merge my example into one of the existing ones - that seems like the easiest thing to do