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: account for page zoom factor in getBoundingClientRect #234970

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

deepak1556
Copy link
Collaborator

Fixes #233692

@deepak1556 deepak1556 force-pushed the robo/enable_standard_browser_zoom branch 2 times, most recently from 6147fed to b52ebaf Compare December 2, 2024 12:12
@deepak1556
Copy link
Collaborator Author

@sbatten can you take a look at this, the required changes are mostly in titlebar, menu, contextmenu and hover widget which you have touched. Let me know if I missed something.

I confirmed the change to not regress

#149741
#149957
#151803

@deepak1556 deepak1556 added this to the January 2025 milestone Dec 3, 2024
@deepak1556
Copy link
Collaborator Author

Also looping in @benibenj, there is no urgency on this PR we can adopt it for January milestone.

Looking at https://chromestatus.com/feature/5198254868529152 the standardization has only completed in Chromium since 128, firefox and safari have open bugs with positive signals. So we will need to feature check and keep the workaround for those cases. I will update the PR.

@deepak1556
Copy link
Collaborator Author

And based on the spec PR we also need to cover getClientRects and IntersectionOserver calls. I haven't accounted for them yet.

@benibenj benibenj self-assigned this Dec 3, 2024
@@ -230,8 +230,7 @@
margin-left: 4px;
}

.monaco-workbench .part.titlebar > .titlebar-container.counter-zoom .menubar .menubar-menu-button > .menubar-menu-items-holder.monaco-menu-container,
.monaco-workbench .part.titlebar > .titlebar-container.counter-zoom .monaco-toolbar .dropdown-action-container {
.monaco-workbench .part.titlebar > .titlebar-container.counter-zoom .menubar .menubar-menu-button > .menubar-menu-items-holder.monaco-menu-container {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for removing this? This was added to fix the zooming for the dropdown action to the right of the command center. Without this, the zooming is wrong for that action. This line counter zooms the position because the title bar does not support negative zoom values (due to WCO).

Ref: #231825

@benibenj
Copy link
Contributor

benibenj commented Dec 3, 2024

I've tested the changes, all is good except for the dropdown to the right of the command centre

Copy link

@JakeDavila210 JakeDavila210 left a comment

Choose a reason for hiding this comment

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

Working

@deepak1556 deepak1556 force-pushed the robo/enable_standard_browser_zoom branch from b52ebaf to 21c615e Compare January 27, 2025 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetBoundingClientRect zoom adjustments
5 participants