Zoom Commands #4488

UncleSnail · 2024-06-05T20:50:18Z

Description

Resolves the issues with chjj's original PR for zoom commands.

This implementation fixes the following issues:

Uses Chrome's zoom levels instead of a set increment.
Resets to the user's default zoom level instead of always 1.
Uses promises instead of callbacks.

It also uses a more similar code style to that of this extension. As expected, I have tested this manually in both Chrome and Firefox, ran the unit tests, and ran the formatter. If there are any suggestions for better implementations or code style, let me know.

Note that it would be very easy to add in an array of the Firefox default zoom levels and switch between the two based on the current browser. This might make the functionality more expected for Firefox users. If you would like me to do this, just let me know. I didn't do it here yet since in the original PR you mentioned to just use Chrome's values for now.

From the original PR:

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).

UncleSnail · 2024-06-10T12:54:58Z

Continuing the discussion about possible other default keyboard shortcut options, a friend recommended using "zz" as reset zoom, which is easier to press and has a president from some other programs like the open source Zathura document viewer (note that by default, Zathura supports both zz and z0). It also follows the pattern of double pressing the same key having a function. We can consider this, although z0 might be easier to remember for the general public.

philc · 2024-06-12T22:23:44Z

Excited for this. I'll look at it soon.

…

On Mon, Jun 10, 2024 at 5:55 AM, Caleb Marcoux < ***@***.*** > wrote: Continuing the discussion about possible other default keyboard shortcut options, a friend recommended using "zz" as reset zoom, which is easier to press and has a president from some other programs like the open source Zathura document viewer (note that by default, Zathura supports both zz and z0). It also follows the pattern of double pressing the same key having a function. We can consider this, although z0 might be easier to remember for the general public. — Reply to this email directly, view it on GitHub ( #4488 (comment) ) , or unsubscribe ( https://github.com/notifications/unsubscribe-auth/AAACDFUHSNB4O5F7PH5RWR3ZGWO3RAVCNFSM6AAAAABI3OVXJKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJYGI3TGMRZGY ). You are receiving this because you are subscribed to this thread. Message ID: <philc/vimium/pull/4488/c2158273296 @ github. com>

background_scripts/main.js

UncleSnail · 2024-07-13T18:08:00Z

Okay @philc, I implemented all the requested changes and I found some more things that you should look at.

I refactored the zoom commands to use nextZoomLevel.
I added unit tests to main_test.js in the Next zoom level context. Please let me know if there was a different place you wanted these.
I implemented the correct zoom levels for Firefox so that it matches Firefox users' expectations.
I performed much more thorough manual testing in Chrome, finding a few Chrome specific bugs.
Chrome allows having zoom set to below and above the maximum, so I fixed the code to properly handle zooming from these cases and added unit tests for this.
Chrome returns a zoom value with floating point errors in some cases, like 33% and 175%, making it impossible to zoom in past these values. I fixed the code to handle this and added unit tests.
Added setZoom value command. Now that we properly handle the edge cases in Chrome and Firefox, it is useful to have a way to let users set their own zoom values, allowing them to, for example, set z1, z2, z3, z4... to their preferred values. This is a feature that I personally want along with the zoom implementation and to me it made sense to round out the zoom feature set. If you would prefer it be in a different PR, or you don't like the feature/implementation, it can easily be removed. Thank you for your patience in reviewing my features. The implementation is simple and I added checks to avoid abuse/breaking when reasonable. If we accept this feature, I will add an FAQ about using it in the wiki.
I realized that tabs.setZoom has a "hidden" built-in behavior to set the zoom level to that tab's default zoom level by setting zoom to 0, making the resetZoom implementation simpler and more performant.

That summarizes the changes.

As usual, I have tried to do extensive testing in both Firefox and Chrome (previously I clearly hadn't tested thoroughly enough in Chrome). The unit tests all pass. I have formatted the code with deno fmt (and only committed the changes it suggested to the files I worked on. Note that there are suggested changes in many other files).

UncleSnail

Here are a few code explanations and question/suggestions. They are all also mostly covered in the summary message.

UncleSnail · 2024-07-13T18:10:00Z

background_scripts/main.js

@@ -868,6 +951,7 @@ Object.assign(globalThis, {
  HintCoordinator,
  BackgroundCommands,
  majorVersionHasIncreased,
+  nextZoomLevel,


I saw that this was how a previous coder had exposed majorVersonHasIncreased for testing purposes, so I did the same for nextZoomLevel. If you have a better preferred method, I'd love to use it.

UncleSnail · 2024-07-13T18:10:46Z

tests/unit_tests/main_test.js

+    assert.equal(0.25, nextZoom);
+  });
+
+  should("Test Chrome 33% zoom in with float error", async () => {


These two floating point error tests only apply to Chrome. Firefox does not seem to have the floating point error issues.

UncleSnail · 2024-07-13T18:12:30Z

tests/unit_tests/main_test.js

+    assert.equal(0.25, nextZoom);
+  });
+
+  should("Zoom in from below the minimum", async () => {


These next four tests ensure zooming works when we zoom from outside of Chromes min/max zoom range, because Chrome (not Firefox) allows the zoom number to be outside the default accepted range. Supporting this should help avoid issues with other Chromium based browsers that allow different zoom values.

UncleSnail · 2024-07-13T18:13:00Z

tests/unit_tests/main_test.js

+    assert.equal(1.00, nextZoom);
+  });
+
+  should("Zoom in past the maximum", async () => {


Zooming should stop on the min/max boundary.

UncleSnail · 2024-07-13T18:13:36Z

tests/unit_tests/main_test.js

+    assert.equal(0.80, nextZoom);
+  });
+
+  should("Zoom in from between values", async () => {


Zooming from between values should zoom up/down to the nearest value, not skipping.

UncleSnail · 2024-07-13T18:17:15Z

background_scripts/main.js

+  async setZoom({ tabId, registryEntry }) {
+    const zoomLevel = registryEntry.optionList[0] ?? 1;
+    const newZoom = parseFloat(zoomLevel);
+    // Chrome breaks on values below 0.01 and 10 or above.


Chrome allows setting the zoom between 1% and 999%, but it will not actually change the visual size of the page beyond 0.25 and 5.00. I decided to allow setting to any value Chrome allows, which seems the most sensible and should help with support for other Chrome based browsers.

UncleSnail · 2024-07-13T18:21:45Z

background_scripts/main.js

+    const newZoom = parseFloat(zoomLevel);
+    // Chrome breaks on values below 0.01 and 10 or above.
+    if (!isNaN(newZoom) && newZoom >= 0.01 && newZoom < 10) {
+      chrome.tabs.setZoom(tabId, newZoom);


Only try a zoom if we found a reasonable number.
We could also support parsing percents if you would like, so that users who try to enter:
map z1 setZoom 10%
will still have their config parse. That would not be difficult, but I think it's probably the right option might be to only have one correct method to set values.

UncleSnail · 2024-07-13T18:22:25Z

background_scripts/main.js

+    return currentZoom;
+  } else if (steps > 0) { // In
+    // Chrome sometimes returns values with floating point errors.
+    currentZoom += 0.0000001; // This is needed to solve floating point bugs in Chrome.


We need to see if adding a tiny value will boost the number up to a value in our list to cover floating point errors.

What does floating point error mean here? isNaN?

I added an example into the comments to make this meaning clear. There are also unit tests to cover the known values for which Chrome does not return the precise number.
For further clarification, if we run tabs.setZoom(tabId, 0.33) Chrome will show 33% in the UI. However, when we run tabs.getZoom(tabId) Chrome will give us the number 0.32999999999999996

UncleSnail · 2024-07-13T18:23:32Z

background_scripts/main.js

+    currentZoom += 0.0000001; // This is needed to solve floating point bugs in Chrome.
+    const nextIndex = zoomLevels.findIndex((level) => level > currentZoom);
+    // Properly handle index not found (we want not found to be after array, not -1).
+    const floorIndex = nextIndex < 0 ? zoomLevels.length : nextIndex - 1;


If we didn't find the value in our list, IE, it was bigger than all of them, we want to say the index is past the end, not -1. This helps handle several edge cases.

UncleSnail · 2024-07-13T18:24:23Z

background_scripts/commands.js

@@ -551,6 +562,11 @@ const commandDescriptions = {
  moveTabLeft: ["Move tab to the left", { background: true }],
  moveTabRight: ["Move tab to the right", { background: true }],

+  setZoom: ["Set zoom to the value given after the command, like setZoom 2.25", { background: true }],


I wanted to add an example here to make the usage more clear, but if there is a better place for this or a better way to make the usage clear, we should remove it here.

Assuming the zoom value must be given as part of the command definition, then documenting that here would be unusual, as this string is shown in the help dialog.

Where to document a command's options is an unsolved issue. I think the #2827 has the most substantial discussion about it. The tips and tricks wiki page contains various command options organized by use case.

For now, I'd suggest this docstring: "Set zoom level to a given value. E.g. map zz setZoom 1.5"

philc

Looking good. I plan to merge this once you've had a chance to look at my comments.

background_scripts/main.js

philc · 2024-07-15T23:13:25Z

background_scripts/main.js

+    return currentZoom;
+  } else if (steps > 0) { // In
+    // Chrome sometimes returns values with floating point errors.
+    currentZoom += 0.0000001; // This is needed to solve floating point bugs in Chrome.


What does floating point error mean here? isNaN?

philc · 2024-07-15T23:17:48Z

background_scripts/main.js

+    // Chrome sometimes returns values with floating point errors.
+    currentZoom += 0.0000001; // This is needed to solve floating point bugs in Chrome.
+    const nextIndex = zoomLevels.findIndex((level) => level > currentZoom);
+    // Properly handle index not found (we want not found to be after array, not -1).


I think this "properly handle" comment is unnecessary if you make the condition on the next line nextIndex == -1 (nextIndex < 0 is actually less clear/specific than nextIndex == -1), and removing it from both branches is a nice DRY cleanup.

Good point! Originally I needed to catch both -1 and -2, but that issue should be fixed now because I moved the subtraction (now nextIndex - 1) to this line instead.

philc · 2024-07-15T23:20:15Z

background_scripts/main.js

+    const newZoom = parseFloat(zoomLevel);
+    // Chrome breaks on values below 0.01 and 10 or above.
+    if (!isNaN(newZoom) && newZoom >= 0.01 && newZoom < 10) {
+      chrome.tabs.setZoom(tabId, newZoom);


philc · 2024-07-15T23:24:19Z

tests/unit_tests/main_test.js

+
+  should("Zoom in from above the maximum", async () => {
+    const count = 1;
+    const currentZoom = 9.99; // highest non-broken Chrome zoom level


What does non-broken mean? What happens, exactly, if it's > 9.99?

It still scales the page correctly to 500%, but it does not parse the number correctly and shows odd % numbers at the top of the page. Note that in the Chrome API documentation for setZoom it does not specify a range of values to be used. I determined the values it handles well from my own testing.
Firefox DOES specify that the value should be between 0.3 and 5, but it gracefully fails for numbers outside of this range. Chrome isn't quite so graceful. That's why I decided to add a limit where Chrome begins to act oddly, but didn't do anything for Firefox's range.

Okay, I don't know why I thought it was parsing the number incorrectly.
I think it does work correctly for bigger values, it's just that there isn't enough space to show the number, so it shows "1,0..." because the "1,000" gets truncated. And it looks like for smaller values it just does nothing, similar to Firefox.
I must have been thinking incorrectly before.
So this bounds checking may be unnecessary. I am going to remove it.

philc · 2024-07-15T23:34:31Z

background_scripts/commands.js

@@ -551,6 +562,11 @@ const commandDescriptions = {
  moveTabLeft: ["Move tab to the left", { background: true }],
  moveTabRight: ["Move tab to the right", { background: true }],

+  setZoom: ["Set zoom to the value given after the command, like setZoom 2.25", { background: true }],


Assuming the zoom value must be given as part of the command definition, then documenting that here would be unusual, as this string is shown in the help dialog.

Where to document a command's options is an unsolved issue. I think the #2827 has the most substantial discussion about it. The tips and tricks wiki page contains various command options organized by use case.

For now, I'd suggest this docstring: "Set zoom level to a given value. E.g. map zz setZoom 1.5"

philc · 2024-07-15T23:35:31Z

background_scripts/commands.js

@@ -551,6 +562,11 @@ const commandDescriptions = {
  moveTabLeft: ["Move tab to the left", { background: true }],
  moveTabRight: ["Move tab to the right", { background: true }],

+  setZoom: ["Set zoom to the value given after the command, like setZoom 2.25", { background: true }],
+  zoomIn: ["Increase zoom on current tab", { background: true }],


I think we can drop "on current tab." Our zoom works the same as Chrome's and the Chrome UI doesn't make that distinction.

Although now that I think about it, in Chrome when changing the zoom of a site, Chrome will also update all open tabs which are showing that same site. I haven't yet tested this PR, but presumably our implementation doesn't do that. That would be a nice future change (outside of this PR) to make the behavior consistent.

I agree, that wording was left from the original implementation, which tried to limit the zoom to the current tab. It is actually incorrect now, as I had already changed the implementation to match the default behavior. The behavior of all of these zoom commands should match the browser default, and it does zoom all pages of the current domain when you use zi or any other of these commands (I just tested in Chromium to make sure it is still working).

philc

Nice work. Looks great. Let's ship it!

Could you rebase this onto master (it looks like there's a merge conflict) and I'll merge it in.

UncleSnail · 2024-07-16T05:27:16Z

Nice work. Looks great. Let's ship it!

Could you rebase this onto master (it looks like there's a merge conflict) and I'll merge it in.

That's odd, I merged master in and don't see any conflicts. I will try to perform a rebase though. Give me a few minutes to see what I can do.

Fixes the issues with the previous PR and brings the implementation style closer to that of the existing project. Uses Chrome's zoom levels instead of a set increment. Resets to the user's default zoom level instead of always 1. Uses primises instead of callbacks.

UncleSnail · 2024-07-16T05:35:09Z

Okay @philc I performed a rebase onto master and force pushed. If there are still issues, let me know.

UncleSnail mentioned this pull request Jun 5, 2024

Add zoom commands #4367

Open

UncleSnail commented Jun 13, 2024

View reviewed changes

background_scripts/main.js Outdated Show resolved Hide resolved

philc requested changes Jul 5, 2024

View reviewed changes

background_scripts/main.js Outdated Show resolved Hide resolved

background_scripts/main.js Outdated Show resolved Hide resolved

background_scripts/main.js Outdated Show resolved Hide resolved

UncleSnail commented Jul 13, 2024

View reviewed changes

philc requested changes Jul 15, 2024

View reviewed changes

UncleSnail requested a review from philc July 16, 2024 03:30

philc approved these changes Jul 16, 2024

View reviewed changes

UncleSnail and others added 10 commits July 16, 2024 01:31

Add zoom commands

08f74af

Remove console log and correct formatting.

038d21e

Refactor zoom methods and add unit tests.

4f369dd

Fix floating point zoom in errors on Chrome.

e866394

Add setZoom command and better handle zooming edge cases.

869b076

Deno fmt changed this new line. I don't see the difference.

395efaa

Solve PR Suggestions: comments/docstring

d8b899b

Remove unneccessary setZoom bounds check.

fc60b13

Remove old comment about Chrome breaking.

a095899

UncleSnail force-pushed the zoom-commands branch from 4cc72f2 to a095899 Compare July 16, 2024 05:34

philc merged commit 2bae2f8 into philc:master Jul 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zoom Commands #4488

Zoom Commands #4488

UncleSnail commented Jun 5, 2024

UncleSnail commented Jun 10, 2024

philc commented Jun 12, 2024 via email

UncleSnail commented Jul 13, 2024

UncleSnail left a comment

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

philc Jul 15, 2024

UncleSnail Jul 13, 2024

philc Jul 15, 2024

UncleSnail Jul 16, 2024 •

edited

Loading

UncleSnail Jul 13, 2024

UncleSnail Jul 13, 2024

philc Jul 15, 2024

philc left a comment

philc Jul 15, 2024

philc Jul 15, 2024

UncleSnail Jul 16, 2024

philc Jul 15, 2024

philc Jul 15, 2024

UncleSnail Jul 16, 2024

UncleSnail Jul 16, 2024

philc Jul 15, 2024

philc Jul 15, 2024

UncleSnail Jul 16, 2024

philc left a comment

UncleSnail commented Jul 16, 2024

UncleSnail commented Jul 16, 2024

Zoom Commands #4488

Zoom Commands #4488

Conversation

UncleSnail commented Jun 5, 2024

Description

UncleSnail commented Jun 10, 2024

philc commented Jun 12, 2024 via email

UncleSnail commented Jul 13, 2024

UncleSnail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UncleSnail Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philc left a comment

Choose a reason for hiding this comment

UncleSnail commented Jul 16, 2024

UncleSnail commented Jul 16, 2024

UncleSnail Jul 16, 2024 •

edited

Loading