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

custom-zoom autohide: Use CSS :hover instead of getBoundingClientRect #7258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mybearworld
Copy link
Contributor

Changes

Currently, custom-zoom detects if the cursor is hovering over the area with getBoundingClientRect and mouseover event. This PR makes it just use the :hover selector.

Reason for changes

Removes most of the logic for this. It's also a bit more robust when the window is being resized.

Tests

Tested in Edge.

@Waakul

This comment was marked as abuse.

@Jazza-231

This comment was marked as off-topic.

@Waakul

This comment was marked as off-topic.

@mybearworld
Copy link
Contributor Author

No one's as stupid to not use ":hover" when needed, It's probably done for a reason. EDIT: woops!, it seems you made that addon! sorry.

I didn't make that addon, and I don't think the person that originally made it is stupid.
The way this addon worked before was entirely reasonable; I had to make some changes in order to get :hover to actually work in this context.

@Waakul

This comment was marked as off-topic.

@mybearworld
Copy link
Contributor Author

i didnt call him stupid, i said he most likely did it for some reasons. You'll need to do lots of testing to clarify everything works (cause using :hover might break other things)

Yeah, the reasons seem to be:

  • Moving the zoom area before the SVG makes it go behind the SVG, but is also necessary in order to select it with CSS.
  • Having :hover work also means that pointer-events can't be set to none, but it also has to be because otherwise, the zoom area will receive all clicks in its area, and the zoom buttons will stop working.

And I have addressed them.

@Samq64 Samq64 added the scope: addon Related to one or multiple addons label Mar 10, 2024
Copy link
Member

@Samq64 Samq64 left a comment

Choose a reason for hiding this comment

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

Tested on Chromium and Firefox with editor-stage-left and after resizing the window.

@Samq64 Samq64 added the status: needs review [Use when there's already 1 approval] Review needed on the PR label Mar 11, 2024
@WorldLanguages WorldLanguages added type: enhancement New feature for the project scope: performance Related to performance of the extension or an addon labels Mar 17, 2024
@WorldLanguages
Copy link
Member

We usually try to avoid mousemove events on the document, so this is also related to performance.

@mxmou
Copy link
Member

mxmou commented Apr 29, 2024

This change makes it impossible to scroll or zoom using the mouse wheel when the mouse is above the zoom buttons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: addon Related to one or multiple addons scope: performance Related to performance of the extension or an addon status: needs review [Use when there's already 1 approval] Review needed on the PR type: enhancement New feature for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants