[#61546] Fix DangerDialog scroll behaviour w/form

[myabc]

⚠️ See #244 for system tests for this bugfix

What are you trying to accomplish?

Fix Danger Dialog scroll behaviour when the dialog contains a form.

Note on using Form Builders

N.B. this issue only occurs when the `form` element is rendered inside the dialog, which in turn only happens when an `action` rather than `builder` is passed as `form_arguments`.

https://qa.openproject-edge.com/lookbook/inspect/primer/open_project/danger_dialog/with_form (incorrect scroll behaviour)

https://qa.openproject-edge.com/lookbook/inspect/primer/open_project/danger_dialog/with_form_builder_form (behaves as expected)

Screenshots

Before

After

Integration

No changes necessary.

List the issues that this change affects.

https://community.openproject.org/wp/61546

Risk Assessment

• Low risk the change is small, highly observable, and easily rolled back.
• Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
• High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

This PR also applies display: contents to the form element when present.

See commit 10e03e7 (PR #234)

Anything you want to highlight for special attention from reviewers?

N/A

Accessibility

• Fixes axe scan violation - This change fixes an existing axe scan violation.
• No new axe scan violation - This change does not introduce any new axe scan violations.
• New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[changeset-bot]

🦋 Changeset detected

Latest commit: 79986c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@openproject/primer-view-components	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[myabc]

@HDinger in order to unblock this for review and merging, I've split the system tests out into a separate PR - #244.

[myabc]

I've noticed that this could be considered an anti-pattern. However I'm not sure if the note in the Catalyst docs refers to children that might be appended dynamically at a later point or those present in the DOM at the time connectedCallback() is invoked.

[HDinger]

I agree, that feels like a smell. I am not a fan of setting styles via JS anyway. Couldn't we set a class on the form and solve it in CSS?

[myabc]

I am not a fan of setting styles via JS anyway.

I generally agree - at least with regards to visual styling. Although when the style is more "behavioural", I think it can be ok to apply the styles directly - e.g. <scrollable-region> web component.

[HDinger]

Yeah, that makes sense. Fine for me then 👍

[HDinger]

Now that I think of it, we might run into problems because of what you quoted above.. We could avoid that by applying a class directly in the component. So we don't have to rely on the timing to be correct..

[HDinger]

I think I managed to run into the error that the form was not present yet when the connectedCallback was called. I guess, applying a class in the constructor via the @form_arguments is probably safer.