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

fix: wait for updateComplete before setting positionTarget #7524

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Jul 9, 2024

Description

Fixes vaadin/flow-components#6368

This fixes a regression introduced by #7015 where setting positionTarget was delayed. It turns out we only need to wait for updateComplete when using Lit so that overlay rect is measured (no need to wait until vaadin-overlay-open).

With this approach, the logic setting positionTarget to undefined and removing style attribute turns out to be unnecessary and even problematic (as it's what caused blinking at the bottom of the screen), so I removed it.

Note: I couldn't find an easy approach to test this. However, existing tests for sub-menu position do cover the new code.

Type of change

  • Bugfix

@DiegoCardoso
Copy link
Contributor

I can see that the initial blink in the bottom right corner is gone, however, there's still one minor issue when a sub-menu is opened after another one closes:

menu-bar-blink.mp4

As seen in the recording, it briefly opens in its last position and then it moves to the right one.

@web-padawan
Copy link
Member Author

As seen in the recording, it briefly opens in its last position and then it moves to the right one.

Right, thanks. I think that's what I originally attempted to prevent when converting to Lit. Will investigate it further.

@web-padawan
Copy link
Member Author

@DiegoCardoso updated to use a different approach, now the flickering seems to be gone. Could you please check it?

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any flashing now. I checked the Lit element and also didn't see any issues there. Even after removing the await overlay.updateComplete line, but that makes some tests fail, so it could be something so quick I wasn't able to notice, so it might be better to keep it.

@web-padawan web-padawan removed the request for review from sissbruecker July 10, 2024 08:47
@web-padawan web-padawan changed the title fix: set sub-menu overlay position target before opacity fix: wait for updateComplete before setting positionTarget Jul 10, 2024
Copy link

sonarcloud bot commented Jul 10, 2024

@web-padawan web-padawan merged commit b46746e into main Jul 10, 2024
9 checks passed
@web-padawan web-padawan deleted the refactor/sub-menu-overlay-position branch July 10, 2024 08:53
web-padawan added a commit that referenced this pull request Jul 10, 2024
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.5.0.alpha5 and is also targeting the upcoming stable 24.5.0 version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MenuBar - SubMenu item flickers open in bottom right corner after upgrading to Vaadin 24.4.1
3 participants