Skip to content

Conversation

AlexsanderDamaceno
Copy link
Contributor

@AlexsanderDamaceno AlexsanderDamaceno commented Feb 19, 2025

1368da9

Fix Cursor is not updated when an element position changes
https://bugs.webkit.org/show_bug.cgi?id=53340

Reviewed by Alan Baradlay.

When an element is re-laid out, the cursor does not change its style until the mouse is moved because this triggers EventHandler::selectCursor.
This issue is resolved by calling a cursor update function to update the cursor type during document layout.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::updateLayout):
* LayoutTests/fast/events/mouse-cursor-change-02-expected.txt: Added.
* LayoutTests/fast/events/mouse-cursor-change-02.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:

Canonical link: https://commits.webkit.org/291948@main

a3172b0

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@AlexsanderDamaceno AlexsanderDamaceno added the CSS Cascading Style Sheets implementation label Feb 19, 2025
@AlexsanderDamaceno AlexsanderDamaceno requested review from fantasai and nt1m and removed request for cdumez February 19, 2025 20:43
@nt1m
Copy link
Member

nt1m commented Feb 19, 2025

This needs a test

@AlexsanderDamaceno
Copy link
Contributor Author

AlexsanderDamaceno commented Feb 19, 2025

This needs a test

@nt1m I did not added test case because, I did not found way of get current cursor displayed on the frame, using getComputedStyle() only returns the defined cursor type in CSS for the element, not the current cursor displayed.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 19, 2025
@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Feb 20, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 20, 2025
@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Feb 20, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

New tests should just use js-test.js instead of -pre.js and -post.js.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 21, 2025
@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Feb 21, 2025
@alanbaradlay
Copy link
Contributor

Do you know why RenderElement::styleDidChange does not cover this case?

I think styleDidChange is updating correctly the style information, but the function that select the type of cursor to display based on current style, only trigger when we move the mouse, If the element changes the position this do not trigger this function, even If element get the new style and is relayout

Ok, so it looks like styleDidChange only schedules cursor update when cursor style changes (which totally makes sense) and this PR is about when content changes under the cursor due to style mutation(s) unrelated to cursor.
While it is difficult to tell if layout has updated content under the cursor, this PR introduces an unconditional rendering schedule, which may very well be okay, but I'd like to hear @smfr's opinion on this.

@AlexsanderDamaceno
Copy link
Contributor Author

Do you know why RenderElement::styleDidChange does not cover this case?

I think styleDidChange is updating correctly the style information, but the function that select the type of cursor to display based on current style, only trigger when we move the mouse, If the element changes the position this do not trigger this function, even If element get the new style and is relayout

Ok, so it looks like styleDidChange only schedules cursor update when cursor style changes (which totally makes sense) and this PR is about when content changes under the cursor due to style mutation(s) unrelated to cursor. While it is difficult to tell if layout has updated content under the cursor, this PR introduces an unconditional rendering schedule, which may very well be okay, but I'd like to hear @smfr's opinion on this.

@smfr could give a review on the code in this pr to solve the cursor change problem ?

@smfr
Copy link
Contributor

smfr commented Feb 26, 2025

I think this is a reasonable approach, but we need to measure if this additional cursor update per rendering update (which requires a new hit-test) is a performance or power regression.

@AlexsanderDamaceno
Copy link
Contributor Author

I think this is a reasonable approach, but we need to measure if this additional cursor update per rendering update (which requires a new hit-test) is a performance or power regression.

Ok, could you schedule a perf test?

@AlexsanderDamaceno
Copy link
Contributor Author

@cdumez could set up a perf this for test this changes impact?

@cdumez
Copy link
Contributor

cdumez commented Feb 27, 2025

@cdumez could set up a perf this for test this changes impact?

Ok. I have started Speedometer A/B testing.

@AlexsanderDamaceno
Copy link
Contributor Author

@cdumez could set up a perf this for test this changes impact?

Ok. I have started Speedometer A/B testing.

Hey, any updates about the perf testing?

@cdumez
Copy link
Contributor

cdumez commented Mar 6, 2025

@cdumez could set up a perf this for test this changes impact?

Ok. I have started Speedometer A/B testing.

This is performance neutral on Speedometer 3.

@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Mar 6, 2025
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 6, 2025
@AlexsanderDamaceno
Copy link
Contributor Author

@cdumez could set up a perf this for test this changes impact?

Ok. I have started Speedometer A/B testing.

This is performance neutral on Speedometer 3.

@alanbaradlay @smfr no changes in perf, could be approved?

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of a 1s timer, I think we should just wait until the next rendering update. You can find many examples of how to do that in LayoutTests (UIHelper.renderingUpdate)

@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Mar 10, 2025
@AlexsanderDamaceno AlexsanderDamaceno added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Mar 11, 2025
@webkit-ews-buildbot webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Mar 11, 2025
@webkit-ews-buildbot
Copy link
Collaborator

Safe-Merge-Queue: Build #51157.

https://bugs.webkit.org/show_bug.cgi?id=53340

Reviewed by Alan Baradlay.

When an element is re-laid out, the cursor does not change its style until the mouse is moved because this triggers EventHandler::selectCursor.
This issue is resolved by calling a cursor update function to update the cursor type during document layout.

* Source/WebCore/dom/Document.cpp:
(WebCore::Document::updateLayout):
* LayoutTests/fast/events/mouse-cursor-change-02-expected.txt: Added.
* LayoutTests/fast/events/mouse-cursor-change-02.html: Added.
* LayoutTests/platform/ios/TestExpectations:
* LayoutTests/platform/win/TestExpectations:
* LayoutTests/platform/wpe/TestExpectations:

Canonical link: https://commits.webkit.org/291948@main
@webkit-commit-queue
Copy link
Collaborator

Committed 291948@main (1368da9): https://commits.webkit.org/291948@main

Reviewed commits have been landed. Closing PR #40887 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 1368da9 into WebKit:main Mar 11, 2025
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CSS Cascading Style Sheets implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants