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: update position on visual viewport scroll and resize #7279

Merged

Conversation

vursen
Copy link
Contributor

@vursen vursen commented Mar 27, 2024

Description

The PR adds listeners for visual viewport scroll and resize events to update the overlay's position accordingly. Based on my testing, the visual viewport events are triggered in all scenarios in which the corresponding window events are triggered, so I completely replaced the window scroll and resize listeners with the visual viewport ones.

The Visual Viewport API has good support across all browsers supported by Vaadin 24:

Screenshot 2024-03-27 at 16 13 45

Fixes #7214

Type of change

  • Bugfix

Copy link

sonarcloud bot commented Mar 27, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@vursen
Copy link
Contributor Author

vursen commented Mar 27, 2024

This snippet can be used for testing the fix (for example, on iOS simulator):

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="UTF-8" />
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
    <title>Combo box</title>
  </head>
  <body>
    <vaadin-combo-box></vaadin-combo-box>

    <div style="height: 350px"></div>

    <vaadin-combo-box></vaadin-combo-box>

    <div style="height: 500px"></div>

    <script type="module">
      import '@vaadin/combo-box';

      document.querySelectorAll('vaadin-combo-box').forEach((comboBox) => {
        comboBox.items = ['Item 1', 'Item 2', 'Item 3'];
      });
    </script>
  </body>
</html>

@vursen
Copy link
Contributor Author

vursen commented Mar 27, 2024

There is now a question whether we still need to call updatePosition in requestAnimationFrame after opening:

// Schedule another position update (to cover virtual keyboard opening for example)
requestAnimationFrame(() => this._updatePosition());

I'm leaning toward keeping it since it might also cover template rendering for someone.

Copy link
Member

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

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

Confirmed that the fix works (the overlay is positioned correctly after a short delay).

Before
combo-bug.mp4
After
combo-after.mp4

@web-padawan web-padawan removed the request for review from tomivirkki April 3, 2024 12:50
@web-padawan web-padawan merged commit 542a6ec into main Apr 3, 2024
9 checks passed
@web-padawan web-padawan deleted the fix/update-overlay-position-on-visual-viewport-resize branch April 3, 2024 12:51
web-padawan pushed a commit that referenced this pull request Apr 3, 2024
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.

combo-box: Overlay positioning not working as expected
3 participants