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

chrome: Decide what to do with SetZoomLevel #3284

Closed
magreenblatt opened this issue Mar 18, 2022 · 7 comments
Closed

chrome: Decide what to do with SetZoomLevel #3284

magreenblatt opened this issue Mar 18, 2022 · 7 comments
Labels
chrome Related to the Chrome runtime enhancement Enhancement request Framework Related to framework code or APIs

Comments

@magreenblatt
Copy link
Collaborator

Original report by me.


This issue involves the following functionality currently supported by the Alloy runtime:

  • Zoom settings (SetZoomLevel; currently configurable via the Chrome UI).

@magreenblatt
Copy link
Collaborator Author

  • edited description

@magreenblatt
Copy link
Collaborator Author

See issue #2969 for background on the Chrome runtime.

@magreenblatt
Copy link
Collaborator Author

This would be chome::Zoom (which uses constants for zoom in/out/reset instead of taking a double value).

@magreenblatt
Copy link
Collaborator Author

magreenblatt commented Oct 4, 2023

We can call the functions from browser_commands.h:

bool CanZoomIn(content::WebContents* contents);
bool CanZoomOut(content::WebContents* contents);
bool CanResetZoom(content::WebContents* contents);
void Zoom(Browser* browser, content::PageZoom zoom);  // PAGE_ZOOM_OUT, PAGE_ZOOM_RESET, PAGE_ZOOM_IN

Unfortunately this doesn't align well with the existing SetZoomLevel/GetZoomLevel API.

@magreenblatt
Copy link
Collaborator Author

magreenblatt commented Oct 4, 2023

It looks like Chrome's ZoomController (used by the browser_commands functions) adds additional functionality on top of the content::HostZoomMap used by Alloy. ZoomController has SetZoomLevel/GetZoomLevel, so we could just call those methods directly.

The implementation of PageZoom::Zoom can be a useful reference for how Chrome expects these methods to be used. The interesting detail here is the clamping to preset zoom levels.

An API using constants (IDC_ZOOM_PLUS, IDC_ZOOM_NORMAL, IDC_ZOOM_MINUS) may also come with issue #3282.

Given how error-prone the SetZoomLevel/GetZoomLevel usage is currently (example), I would be in favor of deprecating/removing these methods in favor of the more explicit API used by browser_comments. To implement the same API for Alloy we would either borrow some of the Chrome implementation or potentially switch from HostZoomMap to ZoomController usage.

@magreenblatt
Copy link
Collaborator Author

Alloy we would either borrow some of the Chrome implementation or potentially switch from HostZoomMap to ZoomController usage.

Alloy already creates a ZoomController for extensions, so that should be fine to use here as well.

@magreenblatt
Copy link
Collaborator Author

For Chrome, it might also be good to expose configuration of ZoomController::SetShowsNotificationBubble (whether the zoom notification bubble can be shown when the zoom level is changed).

magreenblatt added a commit that referenced this issue Oct 5, 2023
Add a simpler CanZoom/Zoom API as an alternative to the more error-prone
SetZoomLevel/GetZoomLevel API. Both APIs are now implemented for both runtimes.
With the Chrome runtime a zoom notification bubble will be displayed unless
CefBrowserSettings.chrome_zoom_bubble is set to STATE_DISABLED.

To test:
- Run cefclient and select zoom entries from the Tests menu.
- chrome: Run cefclient with `--hide-chrome-bubbles` command-line flag to hide
  the zoom notification bubble.
magreenblatt added a commit that referenced this issue Oct 9, 2023
magreenblatt added a commit that referenced this issue Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chrome Related to the Chrome runtime enhancement Enhancement request Framework Related to framework code or APIs
Projects
None yet
Development

No branches or pull requests

1 participant