Overview

Currently when zooming in and out the annotations and labels provided by XB on top of the preview change size. This is not great because it makes it hard to see the annotations when zoomed out, and when zoomed in, they can take too much space.

Proposed resolution

A common pattern from Figma and other page builders is not to change the size of the annotations and labels when zooming in and out.

User interface changes

Before
After
CommentFileSizeAuthor
#30 after.png77.22 KBwim leers
#30 before.png77.57 KBwim leers
#19 Annotaions_zooming.mov16.81 MButkarsh_33
Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

lauriii created an issue. See original summary.

balintbrews’s picture

Some background on this:

When zooming happens, we zoom a <div> outputted by the <Canvas> component by adding transform: scale(...) in an inline style attribute. (Note that we also set the same attribute on the same element elsewhere, just search for style.transform in the codebase.)

The same <div> also outputs <Preview> > <Viewport> where we have the iframe and <Outline> laid out as adjacent elements. <Outline> is where all the annotations and labels live. They all get zoomed, because we scale the <div> which is their ancestor.

I had a chance to discuss this with @jessebaker, and his idea is the following:

  1. Move the <style> attribute with the transform: scale(...) further down the component tree to only zoom the contents of the iframe;
  2. Move <Outline> out of the <Viewport> component and lay it on top of the iframe with absolute positioning — this will require calculating its position;
  3. The <Outline> component needs to be aware of the current zoom level and calculate the dimensions and positioning of the borders it draws around components accordingly.

balintbrews’s picture

bhuvaneshwar’s picture

Assigned: Unassigned » bhuvaneshwar
bhuvaneshwar’s picture

Assigned: bhuvaneshwar » Unassigned

Unassigned it for now as per the priority of the issue @soaratul you can pick this as discussed.

utkarsh_33’s picture

Assigned: Unassigned » soaratul

Assigning it to Atul.

utkarsh_33’s picture

So I'll explain the current state of the MR.

1)

Move the attribute with the transform: scale(...) further down the component tree to only zoom the contents of the iframe;
is done and it disables the annotation of the buttons to be zoomed in. Regarding 2 and 3 in #2 I tried moving the <Outline> component out of <ViewPort> component but the problem is that it looses access to the iframe ref.To overcome that issue i tried 2 approaches:-
  • Use a state variable in the parent state and pass it as a prop to the child which then does all the magic
  • I also tried using forwardRef but was not completely able to get that working.
Also there's a question regarding current zoom levels mentioned in the 3 point of #2 that doesn't useSyncElementSize hook inside the Outline component deal with this problem(I might be wrong as i was not able to test this because if failing to pass the iframe ref)?
balintbrews’s picture

Assigned: soaratul » utkarsh_33
Status: Active » Needs work

Using forwardRef sounds like a good approach. Here is the example from the React docs on how to expose a DOM node to the parent component: https://react.dev/reference/react/forwardRef#exposing-a-dom-node-to-the-.... Is this how you tried making it work? Perhaps you could give it another try and show your code if it's still not working?

While continuing the work on this issue, let's keep an eye on #3469677: Remove flickering when preview is being updated, which will soon land with changes that will affect how we need to solve this issue. Until then, I think it's worth doing the exercise of making forwardRef work, so you clear that complexity first.

utkarsh_33’s picture

I'll create a separate MR for using the forwardRef approach.

utkarsh_33’s picture

I'll try to add the code that i tried for forwardRef.Before that the problem that i faced with the implementation of forwardRef was that we only want the irfameRef from the viewPort so that it can be passed to <Outline> component which is moved out from the ViewPort component, do we really need forwardRef for that?
Isn't there a better/Easy way to approach this?

utkarsh_33’s picture

Describing the current state of MR using forwardRef:-

What's working:-
- I am able to get the iframeRef for the outline component.The outline is place at a static positions using the hardcoded CSS values for now(which we might change subsiquently).
- The outline changes when we switch between the elements.
- The button's annotation are not getting Extra large when zooming(even though it's not correctly positioned).

What's not working:-
- The preview works only for one preview size, that is because we are using the same iframeRef for both the sm and lg previews. Since the ref is mutable and can only be attached to one element at a time, the Outline and Viewport components are both trying to access the same iframeRef, which may be causing the problem.
- I am not able to understand how do i handle the zooming part of the issue.

And as with what Balint mentioned in #10 does it make sense for us to wait for Remove flickering when preview is being updated to land or is there something more that need to explore or work on?

utkarsh_33’s picture

Since we will make changes in 3469672-adjust-XB-annotaion-zoom-using-forwardRef MR so closing the other 2 MR's to avoid confusion.

utkarsh_33’s picture

StatusFileSize
new16.81 MB

Uploading the video to make it clear how thing are working until this point.

utkarsh_33’s picture

The Zooming work fine for the width and height calculation but it is not working correctly for the left and top.

jessebaker’s picture

I’ve been taking a stab at this issue. My brain is melting though, it's late on Friday and I’ve not managed to get it working after putting a few hours into it.

I think the solution is pretty complex but it would go some way towards allowing the refactor I have just proposed in #3475759: Implement a different approach to the iFrame preview interactions - at least laying some groundwork for it.

My theoretical solution is roughly as follows:

We need to be able to have the array of

components that show the site at different viewport sizes rendered inside the component so that they scale when zoomed. But we need to have the (and maybe some other components) rendered outside . However both of those components need to access the correct iFrame element as a ref.
  • At the top level we need an array of refs - I’m thinking this should be stored in a React Context provider because I don't know if storing refs in Redux is a good idea?
  • For each viewport size we need to add a ref to that array probably in or above Canvas.tsx
  • For each iFrame we render we need to set the appropriate ref in the array to point to the “current” iframe in iFrameSwapper
THEN
  • We can move out of Viewport.tsx and instead render it as a sibling of the .
  • Outline can access the correct iFrame ref via the context
  • That will ensure that when the canvas is scaled the Outline is not. But the Outline still knows which iFrame it's supposed to be interacting with because it can get the iFrame ref from the context.
wim leers’s picture

This is now the last issue for #3454094: Milestone 0.1.0: Experience Builder Demo! 🥳

effulgentsia’s picture

#21 sounds like a nice robust solution, but also potentially a lot of work. I'm wondering if a quick and dirty solution would be to either:

  • Use portals to render the not-to-be-scaled components outside of the div that we're scaling, but without moving them out of their current location in the React component tree, or
  • Scale the not-to-be-scaled components by the inverse amount, so that when the ancestor div is scaled, it cancels out.

It's possible that both of the above ideas are bad, or perhaps were already tried and failed, so feel free to ignore them. Just throwing them out there in case one of them might work and isn't too much work to implement.

jessebaker’s picture

Oh wow, yeah portals is a fantastic idea! I blame my Friday evening brain for not thinking of that!

balintbrews’s picture

Brilliant ideas! While using portals would be a solid approach, I was thinking we could try the second idea first — scaling the not-to-be-scaled components by the inverse amount —, given that if we can make it work, it's probably easier, and we may do a rewrite anyway if we decide to go with #3475759: Implement a different approach to the iFrame preview interactions.

balintbrews’s picture

I have a working MR (!325) for scaling the not-to-be-scaled XB annotations by the inverse of current scale. I think this approach will work!

Here is what's left:

  1. 🧪 More manual testing that the solution indeed works.
  2. 🪲 The "Add section" / "Add component" button's width starts to deteriorate at large zoom levels for some reason.
  3. 🧪 Adjusting an existing test to assert that the outer <div> outputted in Outline.tsx has the inline style attribute that cancels out the scaling when zoom is used. This can go into the test case named "Can zoom the canvas with the Zoom Controls".
  4. 🧪 Writing test that ensures that the dimensions of the outline (border shown around selected/hovered component in the preview) are adjusted when zoom is used. This can be as simple as looking at what getBoundingClientRect() returns for the outline's DOM element when zoom level is at 100%, and checking how that changes at different zoom levels.

@utkarsh_33, anything you can tackle from the list above as you start your day on Monday would be super helpful. Let's make this happen before DrupalCon! 💪🏻

utkarsh_33’s picture

Hi @balintbrews, the new implementation seems to work fine, except that Add section/component issue described in #27.2.
I was able to write the test coverages according to what's mentioned in #27.3 and 27.4.
I will try to get the work done related to "Add section" / "Add component" button's .

balintbrews’s picture

Great work on the tests and everything, @utkarsh_33, thank you! I left you some comments on the test code. I also would love us to explore if there's a better way to address the button width issue.

wim leers’s picture

Issue summary: View changes
StatusFileSize
new77.57 KB
new77.22 KB

Tested on Tugboat (see Tugboat link at the top of the issue — yay, #3472103: Add Tugboat integration is already helping! :D) — it seems to work great:

Before
After

I still have concerns about the tests though. See details on MR.

soaratul made their first commit to this issue’s fork.

  • balintbrews committed 845cc231 on 0.x
    Issue #3469672 by utkarsh_33, soaratul, balintbrews, wim leers,...
balintbrews’s picture

Assigned: utkarsh_33 » Unassigned
Status: Needs work » Fixed
wim leers’s picture

Version: » 0.x-dev

Now that this is fixed, I think #3473222: [test-only] Need to update the position of contextual edit component menu while zooming out/in in the canvas is worth looking at next by the same people 😇

Status: Fixed » Closed (fixed)

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