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

The New popup menu UI is not displayed in full screen #1795

Closed
zxuanhong opened this issue Dec 19, 2022 · 8 comments
Closed

The New popup menu UI is not displayed in full screen #1795

zxuanhong opened this issue Dec 19, 2022 · 8 comments
Assignees
Labels
bug Something isn't working

Comments

@zxuanhong
Copy link

Describe the Bug

The New popup menu UI is not displayed in full screen

  1. not full

image

2. full

image

Steps to Reproduce

  1. Set browser content to full screen
  2. clieck change type icon
  3. popup menu not displayed

Expected Behavior

Environment

  • Browser: [e.g. IE 11, Chrome 69] 108
  • OS: [e.g. Windows 7] mac
  • Library version: [e.g. 2.0.0] bpmn js 11.1.0
@zxuanhong zxuanhong added the bug Something isn't working label Dec 19, 2022
@nikku
Copy link
Member

nikku commented Dec 19, 2022

I can reproduce it if you focus a particular element (.bjs-container) for fullscreen:

const container = document.querySelector('.bjs-container');
container.requestFullscreen();

The issue is that the popup menu now lives outside the diagram canvas, outside the fullscreen element. This is a behavior we'd need to revise if we wanted to fix this.

CC @smbea.

@smbea
Copy link
Contributor

smbea commented Dec 28, 2022

Maybe I misunderstood the issue, but I couldn't reproduce it:

Screen.Recording.2022-12-28.at.18.42.29.mov

@nikku
Copy link
Member

nikku commented Jan 1, 2023

Reproduce using our test suite (npm start) and these steps.

capture ihIbDA_optimized

Note that there seem to be other things going on.

@smbea
Copy link
Contributor

smbea commented Jan 2, 2023

Assigning it to me so I can have a look at this

@smbea smbea self-assigned this Jan 2, 2023
@smbea smbea added the ready Ready to be worked on label Jan 2, 2023
@smbea
Copy link
Contributor

smbea commented Jan 5, 2023

I have looked into this and figured out that we don't necessarily need to pull it out of the bjs-container. We can simply keep using thedocumentElement for position calculations (in _ensureVisible); we already have a position: absolute in place.

I gave this a test, and here's how it looks on desktop modeler:

Screen.Recording.2023-01-05.at.10.56.53.mov

In this demo, I added the container for config.popupMenu in bpmn-js like we do for the canvas.

Wdyt @nikku ?

@nikku
Copy link
Member

nikku commented Jan 5, 2023

Just cross-checked #1795 (comment) in the in the Camunda Modeler. What we wanted to accomplish was more natural positioning in the viewport:

image

We may need to use position: fixed rather than position: absolute to accomplish that. Alternatively we could resort to applications properly using positioning attributes. You could argue that the Camunda Modeler shall not render the bjs-container with position: absolute in the first place, which it currently does.

@smbea
Copy link
Contributor

smbea commented Jan 5, 2023

We actually already use position: fixed on the direct parent - .djs-popup-backdrop-, and we use position: absolute on the .djs-popup (see here)

smbea added a commit to bpmn-io/diagram-js that referenced this issue Jan 5, 2023
smbea added a commit to bpmn-io/diagram-js that referenced this issue Jan 5, 2023
smbea added a commit to bpmn-io/diagram-js that referenced this issue Jan 5, 2023
nikku pushed a commit to bpmn-io/diagram-js that referenced this issue Jan 5, 2023
smbea added a commit to bpmn-io/diagram-js that referenced this issue Jan 5, 2023
@nikku nikku added the fixed upstream Requires integration of upstream change label Jan 6, 2023 — with bpmn-io-tasks
@nikku nikku removed the ready Ready to be worked on label Jan 6, 2023
nikku added a commit that referenced this issue Jan 6, 2023
@nikku
Copy link
Member

nikku commented Jan 6, 2023

Closed via ca4559f.

@nikku nikku closed this as completed Jan 6, 2023
@bpmn-io-tasks bpmn-io-tasks bot removed the fixed upstream Requires integration of upstream change label Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants