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.

Sign up for GitHub

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

Jump to bottom

md-menu/md-select overlay positions itself too far to the left side of the anchor element #5522

Open
Pupix opened this issue Mar 9, 2024 · 8 comments
Open

md-menu/md-select overlay positions itself too far to the left side of the anchor element #5522

Pupix opened this issue Mar 9, 2024 · 8 comments

Comments

@Pupix
Copy link
Contributor

Pupix commented Mar 9, 2024

What is affected?

Component

Description

When using md-menu with positioning popover or document the overlay goes 10ish? pixels too far to the left side of the anchor.

The expected behavior would be for the overlay to be left? aligned with the anchor button.

This is also a problem in md-select, which is where I initially noticed the problem.

image

Reproduction

The problems are visible on the material-web.dev website already
https://material-web.dev/components/menu/#popover-positioned-menus

image

https://material-web.dev/components/menu/#document-positioned-menus

image

And the select:
https://material-web.dev/components/select/#usage
image

Workaround

I have not found a workaround.

Is this a regression?

No or unsure. This never worked, or I haven't tried before.

Affected versions

1.3.0

Browser/OS/Node environment

Browser: Version 122.0.6261.112 (Official Build) (64-bit)
OS: Windows 11 Pro, version 23H2, build 22631.3155
Node version: v20.11.0
npm version: 9.6.7
@Pupix Pupix changed the title md-menu overlay positions itself too much to the left side of anchor with positioning="popover" and positioning="document" md-menu/md-select overlay positions itself too far to the left side of the anchor element Mar 9, 2024
@Pupix
Copy link
Contributor Author

Pupix commented Mar 11, 2024

In Firefox 123.0.1 (64bit) the menu behaves how it should, but the md-select is even worse. It opens the overlay in another country.

image

image

image

@Pupix
Copy link
Contributor Author

Pupix commented Mar 11, 2024

Further info:
Applying menu-positioning="absolute" on md-select seems to fix the issue.

The problem may be with how position: fixed; works? Which doesn't always position itself relative to the the viewport, rather any ancestor with CSS transform applied will become a containing block for all descendants, as covered in the spec: https://www.w3.org/TR/css-transforms-1/

There may be other limitations at play here, but I am not aware of them.

@asyncLiz
Copy link
Collaborator

Does this happen outside the catalog as well? If so, can you post a reproduction and the browsers it occurs on?

I wasn't able to reproduce in Chrome or Firefox on macOS at https://material-web.dev/components/select/stories/, so it may be an issue just with the inline demos.

@Pupix
Copy link
Contributor Author

Pupix commented Mar 12, 2024 •
edited
Loading

Does this happen outside the catalog as well? If so, can you post a reproduction and the browsers it occurs on?

Yes, I randomly encountered the issue while working on an unrelated project.

I wasn't able to reproduce in Chrome or Firefox on macOS at https://material-web.dev/components/select/stories/, so it may be an issue just with the inline demos.

I can also confirm that the storyboard works properly.

In regards to the initial issues in Chrome 122.0.6261.112 (Official Build) (64-bit), here's is a repro . Same repro seems to work fine in Firefox 123.0.1 (64bit), so maybe there are 2 unrelated issues. Both tests have been done in Windows 11.

Trying to pinpoint the problem was a pain, however from what I've seen so far it happens only under certain conditions. Perhaps something to do with overflows and scrollbars during position calculations? Setting scrollbar-width: none; on the root <html> completely fixes the positioning problem in the repro and in my personal project.

Changing the height of .wrapper will yield different results, so if you don't instantly see it, try different values.
If you can't reproduce the issue on your macOS then the width of the scrollbars may be the issue. From what I recall windows inlines them, pushing the content to the side, while macOS just overlays them on top of everything, not impacting the page's layout.

On my screen (Chrome 122.0.6261.112) it looks like this:
height: 200px
image

height: 400px You can notice the problem here
image

height: 600px
image

@Pupix
Copy link
Contributor Author

Pupix commented Jul 8, 2024

After further investigation I found the culprit but wouldn't know if the fix I have in mind would impact something else. Could you take a look please? @asyncLiz

The problem seems to be that showPopover() here

material-web/menu/internal/controllers/surfacePositionController.ts

Line 222 in 713f0a8

(surfaceEl as unknown as {showPopover: () => void}).showPopover();
impacts the results of getBoundingClientRect() a few lines below. Calling either anchorEl.getSurfacePositionClientRect() or anchorEl.getBoundingClientRect() before and after the showPopover() call will result in two different x values.

I have no idea why it does what it does. Again, it's in Chromium based browsers (tested Chrome and Edge, Firefox is fine) on Windows.

The fix would be to move the following lines above the showPopover() call:

material-web/menu/internal/controllers/surfacePositionController.ts

Lines 228 to 230 in 713f0a8

const anchorRect = anchorEl.getSurfacePositionClientRect
? anchorEl.getSurfacePositionClientRect()
: anchorEl.getBoundingClientRect();
as there shouldn't be any impact to the rest of the calculations.

The JS code from npm to show the problem:
image

results in the following:
image

@asyncLiz
Copy link
Collaborator

asyncLiz commented Jul 8, 2024

The fix would be to move the following lines above the showPopover() call:

material-web/menu/internal/controllers/surfacePositionController.ts

Lines 228 to 230 in 713f0a8

const anchorRect = anchorEl.getSurfacePositionClientRect
? anchorEl.getSurfacePositionClientRect()
: anchorEl.getBoundingClientRect();

as there shouldn't be any impact to the rest of the calculations.

@e111077 at a glance do you see any problems with doing the anchor rect calculations before calling element.showPopover() in surface position controller?

@e111077
Copy link
Contributor

e111077 commented Jul 8, 2024 •
edited
Loading

I vaguely remember a bug where an absolute (non native popover) surface may resize the document (e.g. introduce scrollbars) and not match the final location of the anchor in the case where the <body> element is quite small. Though, as we can see now, it's already not matching the location of the anchor. I'd be in favor of making the popover approach more robust than the absolute approach though. Though I'm sure there is an order of operations here that can guard for the popover use case vs the absolute use case

@Pupix
Copy link
Contributor Author

Pupix commented Jul 9, 2024

I vaguely remember a bug where an absolute (non native popover) surface may resize the document (e.g. introduce scrollbars) and not match the final location of the anchor in the case where the <body> element is quite small. Though, as we can see now, it's already not matching the location of the anchor. I'd be in favor of making the popover approach more robust than the absolute approach though. Though I'm sure there is an order of operations here that can guard for the popover use case vs the absolute use case

As far as I understand, that showPopover() call gets skipped anyway if any positioning other than popover is used, because the surfaceEl won't have the popover attribute/prop as seen here:

material-web/menu/internal/menu.ts

Lines 485 to 497 in 713f0a8

private renderSurface() {
return html`
<div
class="menu ${classMap(this.getSurfaceClasses())}"
style=${styleMap(this.menuPositionController.surfaceStyles)}
popover=${this.positioning === 'popover' ? 'manual' : nothing}>
${this.renderElevation()}
<div class="items">
<div class="item-padding"> ${this.renderMenuItems()} </div>
</div>
</div>
`;
}

So changing the order of those 2 calls should be insignificant unless, positioning="popover" is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Pupix @e111077 @asyncLiz