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

LibWeb: Fix MouseEvent position values #2364

Merged

Conversation

Psychpsyo
Copy link
Contributor

The clientX and clientY values are, as per the spec, the offset from the viewport.
This makes them actually be that and also fixes up the calculations for offsetX, offsetY, pageX and pageY.
I assume all of these got messed up in some sort of refactor in the past.

The spec comment from the now-removed compute_mouse_event_client_offset() function sadly has no convenient place to be anymore so, for now, it is just gone as well.
Personally, I think it'd make sense to refactor a lot of this file so that not every mouse event repeats a large chunk of (almost) identical code. That way there'd be a nice place to put the comment without repeating it all over the file.
But that is out of the scope of this PR.

Also: I know, offsetX and Y are not fully fixed yet, they still don't ignore the element's CSS transforms but I am working on that in a new PR.

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

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

could you add a test for this change?

@Psychpsyo
Copy link
Contributor Author

could you add a test for this change?

I've added a test over in #2372 (essentially part 2 of this commit) that takes care of all the mouse position values at once.
I hope that's ok.

@gmta
Copy link
Collaborator

gmta commented Nov 20, 2024

I've added a test over in #2372 (essentially part 2 of this commit) that takes care of all the mouse position values at once. I hope that's ok.

I'd rather have a (partial) test in this PR so we don't depend on merging another PR for this code to be tested.

@Psychpsyo
Copy link
Contributor Author

I've split the test into two now and added the relevant half here.

@kalenikaliaksandr
Copy link
Member

could you include the text from the PR description in the commit message? the current commit message is a bit sparse, and since the commit message is more important than the PR description, it would be better to provide a detailed explanation there.

The clientX and clientY values are, as per the spec, the offset from
the viewport.
This makes them actually be that and also fixes up the calculations
for offsetX, offsetY, pageX and pageY.
I assume all of these got messed up in some sort of refactor in the
past.

The spec comment from the now-removed
compute_mouse_event_client_offset() function sadly has no convenient
place to be anymore so, for now, it is just gone as well.
Personally, I think it'd make sense to refactor a lot of this file so
that not every mouse event repeats a large chunk of (almost) identical
code. That way there'd be a nice place to put the comment without
repeating it all over the file.
But that is out of the scope of this PR.

Also: I know, offsetX and Y are not fully fixed yet, they still
don't ignore the element's CSS transforms but I am working on that
in a new PR.
@Psychpsyo
Copy link
Contributor Author

could you include the text from the PR description in the commit message? the current commit message is a bit sparse, and since the commit message is more important than the PR description, it would be better to provide a detailed explanation there.

Also done.

@kalenikaliaksandr kalenikaliaksandr merged commit 7f98976 into LadybirdBrowser:master Nov 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants