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

[lexical-utils] Bug Fix: Add feature detection to calculateZoomLevel #6864

Merged
merged 4 commits into from
Nov 26, 2024

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Nov 25, 2024

Description

Add feature detection to calculateZoomLevel in situations when we are not sure if the browser implements standardized CSS zoom. We still assume that Firefox does, but since only Chrome >= 128 implements it and Safari doesn't implement it yet we use feature detection (creating a div in the document but off-screen, measuring it with and without zoom, then removing it). It's measured twice in case there's a zoom on the body which could cancel out a static measurement.

Closes #6863

Test plan

Before

Insert relevant screenshots/recordings/automated-tests

After

Insert relevant screenshots/recordings/automated-tests

Copy link

vercel bot commented Nov 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 10:21am
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 26, 2024 10:21am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 25, 2024
Copy link

github-actions bot commented Nov 25, 2024

size-limit report 📦

Path Size
lexical - cjs 30.91 KB (0%)
lexical - esm 30.79 KB (0%)
@lexical/rich-text - cjs 39.62 KB (0%)
@lexical/rich-text - esm 32.65 KB (0%)
@lexical/plain-text - cjs 38.21 KB (0%)
@lexical/plain-text - esm 29.93 KB (0%)
@lexical/react - cjs 41.34 KB (0%)
@lexical/react - esm 34 KB (0%)

@ivailop7
Copy link
Collaborator

This looks reasonable. Was originally thinking, since the method is used only in 3 places in 'lexical-react', if we have a way to get it out and have at playground level-only, but besides passing an extra argument to the 3 plugins (draggable block, checklist, context menu), dont have anything better. Chrome 128+ already has a pretty decent market share, so Ideally in 6-7 months we can remove the solution we come up with.

@etrepum
Copy link
Collaborator Author

etrepum commented Nov 25, 2024

I think we still need it for Safari

@ivailop7
Copy link
Collaborator

I think we still need it for Safari

it's in the official spec now, I believe it already works in Safari and according to https://caniuse.com/css-zoom seems ok

@etrepum
Copy link
Collaborator Author

etrepum commented Nov 25, 2024

That's just the zoom CSS property and according to that table it's been supported in Chromium since 4 (2010). Running this code in Safari 18.1.1 (macOS) sets NEEDS_MANUAL_ZOOM to true where Firefox and Chromium set it to false.

    const div = document.createElement('div');
    div.style.cssText =
      'position: absolute; opacity: 0; width: 100px; left: -1000px;';
    document.body.appendChild(div);
    const noZoom = div.getBoundingClientRect();
    div.style.setProperty('zoom', '2');
    NEEDS_MANUAL_ZOOM = div.getBoundingClientRect().width === noZoom.width;
    document.body.removeChild(div);

@etrepum
Copy link
Collaborator Author

etrepum commented Nov 25, 2024

Safari

Screenshot 2024-11-25 at 10 22 54

Chrome

Screenshot 2024-11-25 at 10 24 28

@ivailop7
Copy link
Collaborator

Safari

Screenshot 2024-11-25 at 10 22 54

Chrome

Screenshot 2024-11-25 at 10 24 28

Fair enough. I do remember trying it in WebKit a year+ ago and it worked. Thanks for checking.

@etrepum
Copy link
Collaborator Author

etrepum commented Nov 26, 2024

We don't have any tests that cover this but I think it should be fine. I'm not impacted or interested enough to write e2e for this myself, but if anyone else wants to do that it would be appreciated.

@etrepum etrepum marked this pull request as ready for review November 26, 2024 00:02
@potatowagon potatowagon added the test-needed Merged PRs that could do with more test coverage label Nov 26, 2024
@potatowagon
Copy link
Contributor

We don't have any tests that cover this but I think it should be fine. I'm not impacted or interested enough to write e2e for this myself, but if anyone else wants to do that it would be appreciated.

have added tests-needed label

Copy link
Contributor

@potatowagon potatowagon left a comment

Choose a reason for hiding this comment

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

is this related to browser zoom? ie ctrl + shift + +/- to zoom in and out

@ivailop7
Copy link
Collaborator

is this related to browser zoom? ie ctrl + shift + +/- to zoom in and out

The browser-level zoom behaves correctly already. The problem this fixed is when you embed the editor in anything that uses CSS Zoom and the clickable regions would respect the browser-level zoom, but not the CSS zoom, despite resizing correctly, which is used for content reflow rather than transform, also the CSS Zoom didn't propagate to iframe tags prior to the Chromium-level fix.

Copy link
Collaborator

@ivailop7 ivailop7 left a comment

Choose a reason for hiding this comment

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

Appreciate the quick help on this!

@ivailop7 ivailop7 added the extended-tests Run extended e2e tests on a PR label Nov 26, 2024
@etrepum etrepum added this pull request to the merge queue Nov 26, 2024
Merged via the queue into facebook:main with commit 3c78cbb Nov 26, 2024
66 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR test-needed Merged PRs that could do with more test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor calculateZoomLevel to be an editor setting
4 participants