Update modal.R

[BajczA475]

Currently, the modal title is hard-coded to be an h4 element. This makes sense as a default, but ideally, it would be toggleable to respect a user's heading hierarchy for a11y. This creates an input that allows toggling to whatever level is desired.

[gadenbuie]

Adding an argument is a relatively large public API change; it's best to open an issue first to discuss the need, feature and approach before opening a PR.

That said, I'd prefer to explore options that don't add new arguments. A few occur to me:

1. title is optional; users can include a heading with whatever level they want in the UI elements passed to ....
2. I strongly suspect we could use better, possibly more semantic, HTML that avoids using hX tags at all
3. We could have title accept a character string (default, current treatment) or a tag, in which case we could inspect the tag and make sure its an h1, h2, ..., h6 element and we could also append the appropriate Bootstrap class to it.

I'd prefer these over adding a new argument.

[BajczA475]

Adding an argument is a relatively large public API change; it's best to open an issue first to discuss the need, feature and approach before opening a PR.

Yep I get that, thanks for letting me know. It's my first time trying to do something like this. Didn't want to feel like I was coming to the party empty-handed. That said, I think I designed this one to be fairly backwards-compatible/invisible--the current behavior in the previous version should be the new default in the proposed version, so nothing should break (unless, I guess, if folks are specifying inputs positionally, which they could be...we could move the title_level argument to the end to circumvent that?)

That said, I'd prefer to explore options that don't add new arguments. A few occur to me:

1. title is optional; users can include a heading with whatever level they want in the UI elements passed to ....
2. I strongly suspect we could use better, possibly more semantic, HTML that avoids using hX tags at all
3. We could have title accept a character string (default, current treatment) or a tag, in which case we could inspect the tag and make sure its an h1, h2, ..., h6 element and we could also append the appropriate Bootstrap class to it.

I'd prefer these over adding a new argument.

Yeah these options, especially 2 and 3, seem good too. Right now, the current hard-coding means that an app with only h2s would skip to h4 for this title, which a11y trackers then flag. In my case, I was providing an h3() as an input to title, which meant I was getting an h3 inside an h4, which was pretty goofy-looking! Since supportive technologies do take advantage of the header tags, I think they are sufficiently semantic, but just need to be more flexible to match the hierarchy the user has constructed elsewhere in their app.

[gadenbuie]

Thanks for talking through your reaction to the options. I think the best way forward is to side-step the heading level issue entirely with better ARIA attributes 😄

[gadenbuie]

Looks great! Just a few tiny comments below.

A couple follow up tasks to finish this up:

• Can you please create a tiny demo and include the code in the PR description? (You can borrow from any existing modal example.)
• If you can, can you compare the appearance between the CRAN release of shiny and this branch? It'd be awesome if you could include screenshots of each in the PR description
• We'll need a news bullet, which you could add at the top of NEWS.md the dev version section under a ## Breaking changes section. Include your GitHub handle and the PR number in parenthesis at the end -- (@BajczA475 #4331).

[gadenbuie]

This doesn't quite work as written, we need to assign to the argument when has_title is TRUE:

Suggested change

	if (has_title) `aria-labelledby` = "shiny-modal-title",
	`aria-labelledby` = if (has_title) "shiny-modal-title",

This relies on a nice htmltools behavior that it will drop NULL attributes (and if (FALSE) "foo" returns NULL in R).

[gadenbuie]

small style nit: Let's put this on one line

Suggested change

	div(class = "modal-title",
	id = "shiny-modal-title",
	title)
	div(class = "modal-title", id = "shiny-modal-title", title)

[BajczA475]

Hi, Thanks for checking this out and providing feedback+next steps! I wanted to acknowledge receipt of it--I intend to do it, but I've gotten busy at work. I'll revisit it when things slow down. Cheers, A