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

Add zoom commands #4367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add zoom commands #4367

wants to merge 1 commit into from

Conversation

chjj
Copy link

@chjj chjj commented Nov 19, 2023

Implements zoom commands in a manner similar to cVim (which I recently migrated away from as it seems unmaintained).

The three new commands are zoomIn, zoomOut, and zoomReset. These are combined with a configurable zoomStep option which defaults to 0.10 (10%).

The default key mappings for the above commands are zi, zo, and z0 (cVim defaults) -- may be worth bikeshedding over.

Fixes #1866 and #4161 (also see #2978).

@philc
Copy link
Owner

philc commented Dec 9, 2023

This is awesome and I think this should get added to Vimium. Thank you @chjj for implementing.

I think the simplest and least surprising implementation is to make this work the same as Chrome and Firefox's built-in zoom behavior.

Chrome's zoom has these properties:

  • All tabs from the same origin are zoomed, and any time you change the zoom, it becomes the origin's default zoom level.

  • In Chrome, the zoom factor can only be one of this fixed range:

    25%
    33%
    50%
    75%
    80% (interesting that they have both an 80% and 75%)
    90%
    100%
    110%
    125%
    150%
    175%
    200%
    250%
    300%
    400%
    500%
    
    • (You can see them all listed in Settings > Appearance > Page zoom)
    • In Firefox, it's a slightly different list. I propose we ignore this difference for now and use one list of factors.
  • "Zoom in" and "zoom out" begin from the site's current zoom level.

  • "Zoom reset" should ideally reset to the zoom level that the user has defined in their settings (Appearance > Page Zoom).

I think the non-linear range of zoom factors is helpful in practice, and presumably someone on the Chrome team thought hard about which specific ones to use.

This PR is pretty close to Chrome's implementation already, but for these changes:

  • Remove the Vimium setting for a fixed zoom step
  • Remove the per-tab scope when calling setZoomSettings
  • Select the next zoom factor from the fixed list of factors above, based on the tab's current zoom level (chrome.tabs.getZoom). This is how it works in Chrome; in this PR, z0 sets the zoom to 100%, no matter what the user's default is.

What do you think?

About key mappings:

zi, zo, and z0 are the mappings I would expect for these commands. You mentioned others; are you aware of any popular alternatives?

[1] Firefox's zoom levels are 30%, 50%, 67%, 80%, 90%, 100%, 120%, 133%, 150%, 170%, 200%, 240%, 300%, 400%, 500%.

@@ -159,6 +159,14 @@ const mkRepeatCommand = (command) => (function (request) {
}
});

const setZoom = (tabId, callback) => {
chrome.tabs.getZoom(tabId, (factor) => {
Copy link
Owner

@philc philc Dec 9, 2023

Choose a reason for hiding this comment

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

Let's use await for all of these chrome zoom APIs, rather than callbacks.

@UncleSnail
Copy link
Contributor

@philc If you would like, I would be willing to make your suggested changes to see this feature merged, as it appears the original author might not be making them.

@philc
Copy link
Owner

philc commented May 30, 2024

@UncleSnail Yes, that would be great!

@UncleSnail
Copy link
Contributor

@UncleSnail Yes, that would be great!

Cool, I'll look into making the changes. How would you like me to submit them, since I can't push directly to this PR? Would you like me to create a new branch/PR?

@philc
Copy link
Owner

philc commented May 30, 2024 via email

@UncleSnail UncleSnail mentioned this pull request Jun 5, 2024
@UncleSnail
Copy link
Contributor

Yes

On Thu, May 30, 2024 at 8:11 AM Caleb Marcoux @.> wrote: @UncleSnail https://github.com/UncleSnail Yes, that would be great! Cool, I'll look into making the changes. How would you like me to submit them, since I can't push directly to this PR? Would you like me to create a new branch/PR? — Reply to this email directly, view it on GitHub <#4367 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAACDFXMF72WIHISHTEZI7LZE46S5AVCNFSM6AAAAAA7RLX36KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZZHA3TCOBYGA . You are receiving this because you were mentioned.Message ID: @.>

I created a new branch and PR for the suggested changes. #4488
This PR can probably be closed (I used chjj's branch as a base so that he gets credit for the original implementation commit).

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.

Feature Request - Adjust zoom/fontsize via keypress
3 participants