fix(overlays): cap visualViewport width at clientWidth to handle scrollbar gutter

[hasegawa-101]

Closes #9199

✅ Pull Request Checklist:

• Included link to corresponding React Spectrum GitHub Issue.
• Added/updated unit tests and storybook for this change (for new code or code which already has tests).
• Filled out test instructions.
• Updated documentation (if it already exists for this component).
• Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

[nwidynski]

This should probably be rounded to account for sub-pixel differences when zooming.

[snowystinger]

shouldn't it be ceil? or floor? I don't quite follow what the concern is here though

also, this doesn't need to be an if, can be

width = Math.min(Math.round(width), documentElement.clientWidth)

[nwidynski]

Concern were false-positives in general. I did not take a deeper look at the underlying issue before, but in this case false-positives could result in a tiny gap between the overlay and scrollbar, right? Maybe that's acceptable though - certainly better than cutting content off 🤷

On Firefox & Chrome the clientWidth appears to correspond to Math.round, but Safari apparently uses some other heuristic, neither floor nor ceil.

[snowystinger]

This is sounding very similar to some previous iterations of work here https://github.com/adobe/react-spectrum/pull/8958/files#diff-78d190ce0d1388eb141efadb6cae01cfc47f354f8f3bc3a700405d0f74e606b6R24

it's now the weekend for me, I'll try to look again soon.

[snowystinger]

Thank you for your patience, we finished up our docs and release today, so I had a little time to look at this.

I tried reproducing it with this story in our S2 storybook and was unable to.

export const CloseToEdge: Story = {
  render: (args) => {
    return (
      <div
        className={style({
          display: 'flex',
          alignItems: 'start',
          justifyContent: 'end',
          width: '100vw',
          height: '[calc(100vh + 100px)]'
        })}>
        <MenuTrigger {...args}>
          <Button aria-label="Actions for selected resource"><NewIcon /></Button>
          <Menu {...args}>
            <MenuItem>Favorite</MenuItem>
            <MenuItem>Edit</MenuItem>
            <MenuItem>Delete</MenuItem>
            <SubmenuTrigger>
              <MenuItem>Share</MenuItem>
              <Menu>
                <MenuItem>SMS</MenuItem>
                <MenuItem>Email</MenuItem>
              </Menu>
            </SubmenuTrigger>
          </Menu>
        </MenuTrigger>
      </div>
    );
  },
  parameters: {
    layout: 'none'
  }
};

I assume I couldn't reproduce because the scrollbar wasn't on the body element.

I was able to reproduce "something" with this code in our s2-docs inserted after this line

react-spectrum/packages/dev/s2-docs/src/Header.tsx

Line 168 in 4c91299

<HeaderLink aria-label="npm" href={`https://npmjs.com/${npm}`} target="_blank" rel="noopener noreferrer"><NpmLogo /></HeaderLink>

            <MenuTrigger>
              <Button>
                Press
              </Button>
              <Menu>
                <MenuItem>
                  Docs
                </MenuItem>
                <MenuItem>
                  Releases
                </MenuItem>
              </Menu>
            </MenuTrigger>

However, the fix being suggested in this PR did not fix it.

Can you provide a storybook story or docs example to demonstrate the issue and the fix?

[lixiaoyan]

I think this might not work either. Chrome will return an incorrect document.documentElement.clientWidth value, which includes the scrollbar width, if the document has scrollbar-gutter: stable; but no scrollbar is present.

See also w3c/csswg-drafts#8099 and https://interop-2022-viewport.netlify.app/scrollbar-gutter/short-stable/

[snowystinger]

Comments would indicate this fix is not ready yet. I'm going to close for now. If you wish to keep working on it, please open a new PR with reproduction steps. Thank you!