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

Zoom in doesn't zoom into the selected block #65884

Closed
2 tasks done
jeryj opened this issue Oct 4, 2024 · 18 comments · Fixed by #67126 · May be fixed by #61465 or #66146
Closed
2 tasks done

Zoom in doesn't zoom into the selected block #65884

jeryj opened this issue Oct 4, 2024 · 18 comments · Fixed by #67126 · May be fixed by #61465 or #66146
Assignees
Labels
[Feature] Zoom Out [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@jeryj
Copy link
Contributor

jeryj commented Oct 4, 2024

Description

When zooming in to the canvas via double click or enter keypress on a block, it doesn't zoom to the selected block location.

Step-by-step reproduction instructions

  1. Enter zoom out
  2. Select a section at the top of the canvas
  3. Scroll down
  4. Double click to zoom in on a block
  5. Selected block will not be in frame

Screenshots, screen recording, code snippet

Screen.Recording.2024-10-04.at.2.07.29.PM.mov

Environment info

No response

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@jeryj jeryj added [Feature] Zoom Out [Type] Bug An existing feature does not function as intended labels Oct 4, 2024
@jeryj jeryj self-assigned this Oct 4, 2024
@jeryj
Copy link
Contributor Author

jeryj commented Oct 4, 2024

I think setting a transform-origin in the zoom out CSS transform and calculating a variable on the scaling animation that checks for the current scroll offset and calculating the central point could work well. It would need to work for both zoom-in and out.

@stokesman
Copy link
Contributor

It seems like this is what #61465 is meant to solve. It seems overkill to actually center the block because that’s adjusting the scroll position instead of maintaining it. Also, it can’t work for blocks that are near the end of the canvas unless spacing is added to the end of the canvas to make enough overflow/scrolling range to center the block. Not that it would have to work for such blocks.

I’ll mention as well that #63390 maintains scroll position for free (i.e. without code managing it).

@ndiego
Copy link
Member

ndiego commented Oct 7, 2024

I can confirm this is occurring in 6.7 Beta 1, so I am adding it to the project board. It would be great to come up with a solution before the release.

@ndiego ndiego moved this to 📥 Todo in WordPress 6.7 Editor Tasks Oct 7, 2024
@richtabor richtabor changed the title Zoom in doesn't center selected block Zoom in doesn't zoom into the selected block Oct 9, 2024
@richtabor
Copy link
Member

It seems like this is what #61465 is meant to solve.
I’ll mention as well that #63390 maintains scroll position for free

Which is the proposed path forward?

@draganescu
Copy link
Contributor

draganescu commented Oct 10, 2024

I have tried to get #61465 to work, still not there yet. I think #63390 needs a summary of where it's at.

@ndiego
Copy link
Member

ndiego commented Oct 14, 2024

Since RC1 is occurring later today and there is no current solution, I think we should punt this to 6.7.1 or 6.8. Thoughts @colorful-tones @getdave @kevin940726?

@colorful-tones
Copy link
Member

I think we should punt this to 6.7.1 or 6.8.

Yes 👍

@getdave
Copy link
Contributor

getdave commented Oct 14, 2024

I would love this to be solved for 6.7. Contributors have been heavily focused on the scaling bugs this week, but with that rounding out, may be able to pivot to solve this as a related issue. The "zooming in" experience was introduced during this cycle so it seems valid to pursue a fix through the RC.

Can we reassess as we move through RC?

@priethor
Copy link
Contributor

While not part of the release squad, I agree with Dave: the Zoom in/out experience is also the most prominent feature in the upcoming 6.7 release, so delivering an experience as polished as possible is even more critical.

@ndiego
Copy link
Member

ndiego commented Oct 14, 2024

While not part of the release squad, I agree with Dave: the Zoom in/out experience is also the most prominent feature in the upcoming 6.7 release, so delivering an experience as polished as possible is even more critical.

Sounds good!

@getdave getdave added the [Priority] High Used to indicate top priority items that need quick attention label Oct 21, 2024
@ndiego
Copy link
Member

ndiego commented Oct 29, 2024

How are we looking on this @jeryj @ajlende? Do we think a fix is achievable for 6.7?

@jeryj
Copy link
Contributor Author

jeryj commented Oct 29, 2024

I think we can. We have a POC route, and are working to apply it to Gutenberg. I hope we'll have a PR up for review this week 🤞🏻

@jeryj
Copy link
Contributor Author

jeryj commented Oct 31, 2024

Not ready for review, but here's a PR to watch on maintaining scroll position when toggling between zoom levels. #66618

@getdave
Copy link
Contributor

getdave commented Nov 4, 2024

Given that there is a lot of code in the proposed PR and we are now at RC3 (tomorrow) I'm not feeling very confident about a fix landing in 6.7. It doesn't allow for much time for any knock on effects to be observed/resolved before the final release next week (12th November).

I'll test out the PR now, but it might be that we have to punt this to 6.8. Appreciate a confidence check from @colorful-tones @ndiego @kevin940726.

Also cc'ing @annezazu and @richtabor who have been heavily involved in Zoom Out Design work and/or triage.

@ndiego
Copy link
Member

ndiego commented Nov 4, 2024

Given that there is a lot of code in the proposed PR and we are now at RC3 (tomorrow) I'm not feeling very confident about a fix landing in 6.7. It doesn't allow for much time for any knock on effects to be observed/resolved before the final release next week (12th November).

I agree. We could also punt to 6.7.1 since it would be great to fix this as soon as possible.

@annezazu
Copy link
Contributor

annezazu commented Nov 5, 2024

Agreed once more. Thanks for raising this.

@jeryj
Copy link
Contributor Author

jeryj commented Nov 6, 2024

We still have some refactoring to do, but the PR to fix this is ready for initial testing: #66618

Also, while there is a lot of code changed, it's pretty confined to only impacting the transition animation between zoom in/out. I'm fine with this being punted to 6.7.1, but I don't think we should equate PR size to risk. I think the PR risk is on the lower side, especially given it's fixing something that is definitely broken.

@getdave
Copy link
Contributor

getdave commented Nov 7, 2024

I don't think we should equate PR size to risk.

I would say it's one of the most important metrics we have to consider. The more code changes the more surface area for potential unexpected knock on effects. This happens a lot and it's very difficult to predict - for example I've witnessed it several times already during RC.

If this were at RC1 or even RC2 we'd have time for public testing to help catch any other problems. But we're looking for this to be included in a silent RC4 (which ideally shouldn't even be happening) which will be subject to no public testing period.

As a result the threshold for inclusion must necessarily be extremely high.

Release leads definitely appreciate all the work going on here and will look forward to including in 6.7.1 🙇‍♂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Zoom Out [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
10 participants