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

Incorrect behavior of "cursor: auto" over links #4161

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Sep 9, 2022

7aae85f

Incorrect behavior of "cursor: auto" over links
https://bugs.webkit.org/show_bug.cgi?id=173909
rdar://99495893

Reviewed by NOBODY (OOPS!).

The initial attempt to fix this bug and comply to spec was made
at https://commits.webkit.org/253685@main

After that, a:any-link no longer depends on 'cursor: auto'
for achieving "Hand" (css: pointer) cursor. This is because, according to spec,
'cursor: auto' can only result in default or text cursor. Therefore, 253685@main
sets 'cursor: pointer' in the UA stylesheet (html.css).

Because of that, a test case for cursor types for recognized text (live-text)
was broken. There, we were testing a <img> nested in a <a href="">. Since,
this <a> is now selected to have 'cursor: pointer', its nested <img>
has no longer 'cursor: auto', which is required for resulting in IBeam (text)
cursor for the overlay installed in the img (see EventHandler::selectCursor).

Therefore, we are updating the test case here. A <img> inside a <a href="...">
is by default styled with cursor: pointer, and therefore hovering over
its overlay doesn't show IBeam (text) cursor, unless user styles its cursor to auto.

* LayoutTests/fast/images/text-recognition/mac/cursor-types-for-recognized-text-expected.txt:
* LayoutTests/fast/images/text-recognition/mac/cursor-types-for-recognized-text.html:
Updating tests. Note that one of the test cases is commented out. As stated in the comment,
this would fail due to https://bugs.webkit.org/show_bug.cgi?id=244944 (radar: 99495849).
In short, EventHandler::selectCursor will select an IBeam cursor although it shouldn't.
This is because Node::canStartSelection() is not false for user-select: none.
* LayoutTests/platform/mac-wk2/TestExpectations:
Reseting TextExpectations for this test, before set as failure.
* Source/WebCore/html/shadow/imageOverlay.css:
(div#image-overlay):
We should not style cursor for the live-text overlay that is installed
on top of images with text. Doing that would disable user to control which
cursor is displayed for the text portion of the image, since the overlay
is in the shadow tree, hosted by the image element. I.e.: user's cursor style
for the image would not be propagated to the overlay.

7aae85f

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-debug βœ… πŸ›  gtk βœ… πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios ❌ πŸ§ͺ api-mac βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk1
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch-sim βœ… πŸ§ͺ mac-wk2-stress

@vitorroriz vitorroriz self-assigned this Sep 9, 2022
@vitorroriz vitorroriz added CSS Cascading Style Sheets implementation WebKit Nightly Build labels Sep 9, 2022
@vitorroriz vitorroriz force-pushed the eng/Incorrect-behavior-of-cursor-auto-over-links branch from b5d767e to 91c2ae1 Compare September 9, 2022 13:21
@vitorroriz vitorroriz force-pushed the eng/Incorrect-behavior-of-cursor-auto-over-links branch 2 times, most recently from f42598c to ef926ac Compare September 9, 2022 15:27
https://bugs.webkit.org/show_bug.cgi?id=173909
rdar://99495893

Reviewed by NOBODY (OOPS!).

The initial attempt to fix this bug and comply to spec was made
at https://commits.webkit.org/253685@main

After that, a:any-link no longer depends on 'cursor: auto'
for achieving "Hand" (css: pointer) cursor. This is because, according to spec,
'cursor: auto' can only result in default or text cursor. Therefore, 253685@main
sets 'cursor: pointer' in the UA stylesheet (html.css).

Because of that, a test case for cursor types for recognized text (live-text)
was broken. There, we were testing a <img> nested in a <a href="">. Since,
this <a> is now selected to have 'cursor: pointer', its nested <img>
has no longer 'cursor: auto', which is required for resulting in IBeam (text)
cursor for the overlay installed in the img (see EventHandler::selectCursor).

Therefore, we are updating the test case here. A <img> inside a <a href="...">
is by default styled with cursor: pointer, and therefore hovering over
its overlay doesn't show IBeam (text) cursor, unless user styles its cursor to auto.

* LayoutTests/fast/images/text-recognition/mac/cursor-types-for-recognized-text-expected.txt:
* LayoutTests/fast/images/text-recognition/mac/cursor-types-for-recognized-text.html:
Updating tests. Note that one of the test cases is commented out. As stated in the comment,
this would fail due to https://bugs.webkit.org/show_bug.cgi?id=244944 (radar: 99495849).
In short, EventHandler::selectCursor will select an IBeam cursor although it shouldn't.
This is because Node::canStartSelection() is not false for user-select: none.
* LayoutTests/platform/mac-wk2/TestExpectations:
Reseting TextExpectations for this test, before set as failure.
* Source/WebCore/html/shadow/imageOverlay.css:
(div#image-overlay):
We should not style cursor for the live-text overlay that is installed
on top of images with text. Doing that would disable user to control which
cursor is displayed for the text portion of the image, since the overlay
is in the shadow tree, hosted by the image element. I.e.: user's cursor style
for the image would not be propagated to the overlay.
@vitorroriz vitorroriz force-pushed the eng/Incorrect-behavior-of-cursor-auto-over-links branch from ef926ac to 7aae85f Compare September 9, 2022 19:04
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for b5d767e. Live statuses available at the PR page, #4161

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Sep 14, 2022
@Ahmad-S792
Copy link
Contributor

Is this PR reverted the patch? Link since the bug seems to be closed, do we need to reopen bug to land this?

@vitorroriz
Copy link
Contributor Author

Is this PR reverted the patch? Link since the bug seems to be closed, do we need to reopen bug to land this?
@Ahmad-S792 yes, this was reverted and we need to reopen it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation merging-blocked Applied to prevent a change from being merged
Projects
None yet
5 participants