Keyboard commands to zoom in and out should only impact the preview canvas

Created on 22 August 2024, 3 months ago
Updated 20 September 2024, 2 months ago

Overview

The zoom commands (CMD/CTRL and + CMD/CTRL and -) are currently zooming the whole interface due to default browser behavior.

Moved to 🐛 Pinch gesture zooming sometimes invokes OS zoom behavior Active

Proposed resolution

Given that the we expect users to be zooming in and out within the preview, these commands should be redirected for this. This is common behavior for Figma and other page builders. See #19

User interface changes

🐛 Bug report
Status

Fixed

Component

Page builder

Created by

🇫🇮Finland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • Assigned to fazilitehreem
  • Issue was unassigned.
  • 🇫🇮Finland lauriii Finland

    Tested manually and seems to work as expected! 🤩

    @fazilitehreem Is this something we could add automated Cypress tests for?

  • Pipeline finished with Failed
    3 months ago
    Total: 322s
    #268288
  • Not sure @laurii, we can add some test to check

  • Status changed to Needs work 3 months ago
  • 🇫🇮Finland lauriii Finland

    @fazilitehreem Is this something you'd like to do?

    Moving to needs work to indicate that we have some progress here 😊

  • First commit to issue fork.
  • Pipeline finished with Success
    3 months ago
    Total: 506s
    #273093
  • Assigned to soaratul
  • Status changed to Needs review 3 months ago
  • Status changed to Needs work 3 months ago
  • 🇮🇳India soaratul

    Changing status to Needs work to write test case for zoom reset and to fix a bug that I found while writing test case.

  • Issue was unassigned.
  • Status changed to Needs review 3 months ago
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Tests are present 👍

    Bumping to since @lauriii added this to 🌱 Milestone 0.1.0: Experience Builder Demo Active .

  • Assigned to soaratul
  • Status changed to Needs work 2 months ago
  • 🇬🇧United Kingdom jessebaker

    I'm afraid I have quite a bit of feedback!

    The zoom commands (CMD/CTRL and + CMD/CTRL and -) are currently zooming the whole interface due to default browser behavior.

    This seems to be fixed but the new test introduced in this MR is not actually testing that. TBC if it's even possible to test that the default browser behaviour is being prevented in a Cypress test!

    There seems to a bug that means that pinch-zooming on a trackpad on Mac can also sometimes zoom the whole browser, this should also be addressed here.

    This is not addressed yet. (mouse moving between iframe and parent during the zoom action causes it) - lets raise another issue for this specifically.

    The MR is adding the ability to press the 0 key to reset the zoom but

    1. If you focus in the iframe and then press (specifically) NUMPAD0 it doesn't pass that event up to the parent. (The other 0 key works)
    2. The new test introduced does not test for the above usecase.

    If you zoom to an irregular zoom amount (like 83% instead of one of the zoom amounts listed in the dropdown) using pinch zoom (or ctrl + mouse wheel) and then press +, the zoom snaps to 25%. I don't know if this was introduced in this MR.

  • 🇮🇳India omkar-pd

    @jessebaker,

    If you zoom to an irregular zoom amount (like 83% instead of one of the zoom amounts listed in the dropdown) using pinch zoom (or ctrl + mouse wheel) and then press +, the zoom snaps to 25%. I don't know if this was introduced in this MR.

    For this, we already have an issue that needs to be reviewed.
    Zoom Behaviour Issue: Clicking Plus Button Resets Zoom Level Before Gradually Increasing 🐛 Zoom Behaviour Issue: Clicking Plus Button Resets Zoom Level Before Gradually Increasing Needs review

  • 🇮🇳India soaratul

    @jessebaker!

    There seems to a bug that means that pinch-zooming on a trackpad on Mac can also sometimes zoom the whole browser, this should also be addressed here.

    I know for this ☝️ we are about to create an separate issue still I tried to fix this using touch-action: none; and other JS/React ways but no luck.

    Apart from this ☝️ I tried to write a test case 👇 in cypress to validate default browser behaviour on zoom in/out but again found nothing related to this in cypress.

    This seems to be fixed but the new test introduced in this MR is not actually testing that. TBC if it's even possible to test that the default browser behaviour is being prevented in a Cypress test!

    For this 👇 I cannot check as my external keyboard having numeric pad is not working with my Mac by the way are the other keys plus/minus(+/-) for zoom in/out using numeric pad are passing the test?

    If you focus in the iframe and then press (specifically) NUMPAD0 it doesn't pass that event up to the parent. (The other 0 key works)

  • Assigned to jessebaker
  • 🇬🇧United Kingdom jessebaker

    This is a challenging issue due a number of overlapping factors. Attempting to write a clearer breakdown here to reduce some of the cognitive load!

    There are further complications here when discussing different OS/browser/user input device considerations so for now I'm going to focus on a Mac Laptop with Chrome and a Mac touchpad.

    Outside of Experience builder, just talking about regular web pages/browser interactions zoom is handled in two different ways:

    Browser Zoom (Cmd + and Cmd -)

    When you use the keyboard shortcuts Cmd + and Cmd -, you are invoking Chrome's built-in browser zoom functionality. This changes the zoom level of the entire webpage, including text, images, and other content. It's a discrete step-by-step zoom and is visually indicated by a magnifying glass icon appearing in the top right of the address bar. This type of zoom is remembered by the browser for each website, meaning if you revisit the site, it will retain the zoom level you set previously.

    macOS Zoom (Trackpad Pinch Gesture)

    When you use the pinch gesture on the trackpad, you are using macOS's native pinch-to-zoom capability. This zoom method generally provides a smoother and more fluid zooming experience. Instead of changing the zoom level in discrete steps, it allows for continuous and gradual zooming. This method is handled by the operating system and provides a different user experience compared to the browser's internal zoom function.

    XB Canvas zoom (=, + and -)

    Pressing +/= or - while in XB will zoom the canvas. This is handled in two different places because we have to handle the keyboard shortcuts different depending on if the user has focused inside the preview iFrame or the parent document.

    1st Issue

    Pressing cmd + is simultaneously invoking the Browser Zoom and the XB Canvas Zoom. This occurs because there is no check for the event.metaKey boolean when we handle the keydown event. This bug appears to only affect our handling the event when the user's focus is inside the preview iFrame in useIframeKeyHandlers.ts. I guess the useHotkeys hook that we use in Canvas.tsx when the event occurs outside the iFrame is correctly stopping this behaviour.

    1st Issue solution

    There are two options here.

    1. Stop both events happening simultaneously. So cmd + will invoke Browser Zoom but not XB Canvas Zoom. + will invoke XB Canvas Zoom but not Browser Zoom. This is possibly required for a11y to allow a user to zoom the XB UI.
    2. Prevent the user from using Browser Zoom entirely.

    The first can be done by: When handling the keypress events in useIframeKeyHandlers.ts check for the value of the event.metaKey and don't invoke the XB Canvas Zoom if it is true.

    The second can be done by: Adding an event listener (both inside and outside the iFrame) that does something like

    if (event.metaKey) {
        // Check if the Plus key (which is also the equals key) is pressed
        if (event.key === '=' || event.key === '+' || event.key === '-') {
          event.preventDefault();
        }
    }
    

    At this point - a decision needs to be made on which solution is best (taking into account A11y and user expectations based on competing products.)

    2nd Issue

    Given the complexity of issue 1, I'm going to move the other issue to it's own ticket. 🐛 Pinch gesture zooming sometimes invokes OS zoom behavior Active

  • Issue was unassigned.
  • Status changed to Needs review 2 months ago
  • 🇬🇧United Kingdom jessebaker

    I have made MR !320 which will stop both events happening simultaneously as that is actually a bug.

    I propose that we code review and merge that if we are happy that it addresses that bug. Once that is merged we need to take a look at @soaratul's MR and see what changes in there we would like to carry on (for instance the keyboard shortcut to reset the zoom level).

    I have raised a follow up issue to explore the UX and a11y implications of @lauriii's originally proposed solution (preventing users from using Browser Zoom with keyboard shortcuts) 🌱 Consider a11y impact and/or competitor analysis for preventing browser zoom Active

  • First commit to issue fork.
  • Pipeline finished with Skipped
    2 months ago
    #288392
  • Status changed to Fixed 2 months ago
  • 🇳🇱Netherlands balintbrews Amsterdam, NL

    Given the complexities, I think we landed this issue at a very good place, with great analysis on where to go from here, and a clean solution for an incremental improvement. We have two follow-up issues:

    1. 🐛 Pinch gesture zooming sometimes invokes OS zoom behavior Active
    2. 🌱 Consider a11y impact and/or competitor analysis for preventing browser zoom Active
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    👍

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024