Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popup Menu positioning breaks inside "containing blocks" #2156

Closed
deje1011 opened this issue May 3, 2024 · 8 comments
Closed

Popup Menu positioning breaks inside "containing blocks" #2156

deje1011 opened this issue May 3, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@deje1011
Copy link

deje1011 commented May 3, 2024

Thanks for your work on this library!
We are currently trying to integrate it but we are facing an issue when embedding a bpmn diagram in a dialog.
I'm not sure if the issue should be solved in bpmn-js or in diagram-js but I think it's a general problem with the positioning of popovers:

Describe the Bug

If a diagram is nested within a "containing block" that is not full screen, the positioning of the popup menu breaks.
Imagine the following DOM structure:

<div class="content" style="padding: 100px;">
    <div class="canvas">
        <!-- diagram is rendered here -->
    </div>
</div>
Screenshot image

As long as there is no containing block around the diagram, this works as expected. But if we turn the canvas element into a containing block by adding a transform, the fixed position of the popup-menu will be relative to the position of that container, instead of the entire document.

<div class="content" style="padding: 100px;">
    <div class="canvas" style="transform: scale(1)">
        <!-- diagram is rendered here -->
    </div>
</div>

Therefore, the popover menu will be offset to the right and to the bottom by 100px (which is the offset of the .canvas element, caused by the padding on the .content).

Screenshot image

Steps to Reproduce

Steps to reproduce the behavior:

  1. Open https://demo.bpmn.io/s/start
  2. Add 100px of padding to the .content element
  3. Add transform: scale(1) to the .canvas element
  4. Open a popover menu

Expected Behavior

The position of the popover should not be affected by this.

Possible Solutions

I think this could be solved by either appending the popover menu to the body or by subtracting the offset of the closest containing block before applying the position to the popover-menu element.
What do you think?

Environment

Please complete the following information:

  • Browser: Chrome 123
  • OS: MacOS 13.6
  • Library version: 14.4.2
@deje1011 deje1011 added the bug Something isn't working label May 3, 2024
@nikku nikku transferred this issue from bpmn-io/diagram-js May 6, 2024
@nikku
Copy link
Member

nikku commented May 6, 2024

@deje1011 I moved this over to bpmn-js. Will have a look and see if I can reproduce your issue.

@nikku
Copy link
Member

nikku commented May 6, 2024

Could you elaborate why you need a transform outside of the diagram in the first place? I'd be hesitant to support it.

@barmac
Copy link
Member

barmac commented May 6, 2024

Thanks for reporting this! I'd be happy to review a contribution providing a fix for this. It could probably use the offsetParent property for the positioning of the popup menus: https://polypane.app/blog/offset-parent-and-stacking-context-positioning-elements-in-all-three-dimensions/#how-to-find-the-offset-parent

@deje1011
Copy link
Author

deje1011 commented May 6, 2024

Could you elaborate why you need a transform outside of the diagram in the first place? I'd be hesitant to support it.

Sure! In our case, the diagram is shown inside a dialog. And the dialog has an opening animation, which uses transform to scale the dialog while it fades in.

There are some other css properties that can cause the same issue if they were to be used on the .canvas:
transform, perspective, will-change: transform/perspective, filter, contain, container-type, backdrop-filter.
https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_block#identifying_the_containing_block

And I'm afraid we can't use the offsetParent property to find the containing block because it doesn't work for some of those cases.
https://bpceee.github.io/posts/10

I couldn't find an implementation of a function to detect the closest containing block, yet. If I find the time later today, I can implement it and post it here.

@nikku
Copy link
Member

nikku commented May 6, 2024

Sure! In our case, the diagram is shown inside a dialog. And the dialog has an opening animation, which uses transform to scale the dialog while it fades in.

Pragmatic solution: Remove transform attribute when scale turns 1?

@deje1011
Copy link
Author

deje1011 commented May 6, 2024

Yes, removing transform when scale is 1 would work :) Currently we work around it by disabling the animation completely for dialogs that contain diagrams. If you decide that supporting this edge case isn't worth it, we could improve that workaround.

Btw, we discovered today that the form-js editor has a similar same issue, if it's embedded in such a containing block.
When dragging form controls into the form, they have an offset.

I tried to implement a function that finds the containing block for a given element but I must say that I'm not super confident that this works in all cases. And the MDN docs make some distinctions between content-box vs padding-box and continuous media vs paged media, which I didn't consider in my implementation. But maybe it can be a starting point for someone else:
https://codepen.io/jjd/pen/xxevBKN?editors=0010

An easier solution might be to append the popover menu to the body. That way, it can't accidentally be nested inside a containing block and the top and left properties would always be relative to the entire viewport as expected.

@nikku
Copy link
Member

nikku commented May 7, 2024

@deje1011 I think you do yourself a favor to get rid of transform: ... on any standard HTML components, outside of transitions.

An easier solution might be to append the popover menu to the body. That way, it can't accidentally be nested inside a containing block and the top and left properties would always be relative to the entire viewport as expected.

The other option indeed is for us to move such position: fixed elements to the document.body, but then again that poses the risk to break custom integrations; in fact we were there before, and reverted that change. We'll not pursue that direction.

@nikku
Copy link
Member

nikku commented May 7, 2024

I'll close this as wontfix. It is an edge case, and your usage of transform will break any tools that do position: fixed inside a dialog. Workaround is available.

@nikku nikku closed this as not planned Won't fix, can't repro, duplicate, stale May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

3 participants