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

REGRESSION (iOS 16): sina.cn: Find on Page highlights obscure the text behind them #9618

Merged
merged 1 commit into from Feb 4, 2023

Conversation

pxlcoder
Copy link
Member

@pxlcoder pxlcoder commented Feb 3, 2023

ed56087

REGRESSION (iOS 16): sina.cn: Find on Page highlights obscure the text behind them
https://bugs.webkit.org/show_bug.cgi?id=251707
rdar://102302792

Reviewed by Wenson Hsieh.

sina.cn contains elements with "-webkit-user-select: none". Prior to iOS 16,
any text found within these elements would count towards the number of results,
but would not be highlighted.

Find-in-page logic was refactored in iOS 16 to support UIKit's new Find & Replace
API. As part of these changes, find-in-page began storing/restoring ranges, rather
than simply updating selection. Consequently, the `TextIndicator` used to draw
highlights is created from a `SimpleRange`, rather than the current selection.
The previously used constructor would early return if the selection was empty
(as is the case when attempting to select a "-webkit-user-select: none" element),
resulting the the pre-iOS 16 behavior described above.

`TextIndicator` does not know how to take snapshots of "-webkit-user-select: none"
content. Consequently, the highlight is painted without any text, leading to
found text getting obscured.

For now, restore the pre-iOS 16 behavior, and do not draw highlights if the
selection is empty. In the longer term, `TextIndicator` should be taught to
draw this content (tracked in webkit.org/b/251709). Note that following this
change, there is still a net progression over pre-iOS 16 behavior, where
WebKit will indicate found results with "-webkit-user-select: none" by drawing
"holes".

* Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:
(WebKit::WebFoundTextRangeController::drawRect):

Additionally, move the use of `GraphicsContext` closer to the painting logic to
avoid running unnecessary code.

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

99057c5

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

@pxlcoder pxlcoder requested a review from cdumez as a code owner February 3, 2023 20:04
@pxlcoder pxlcoder self-assigned this Feb 3, 2023
@pxlcoder pxlcoder added the New Bugs Unclassified bugs are placed in this component until the correct component can be determined. label Feb 3, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 3, 2023
@pxlcoder pxlcoder added unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing and removed merging-blocked Applied to prevent a change from being merged labels Feb 4, 2023
@pxlcoder
Copy link
Member Author

pxlcoder commented Feb 4, 2023

Thanks for the review!

…t behind them

https://bugs.webkit.org/show_bug.cgi?id=251707
rdar://102302792

Reviewed by Wenson Hsieh.

sina.cn contains elements with "-webkit-user-select: none". Prior to iOS 16,
any text found within these elements would count towards the number of results,
but would not be highlighted.

Find-in-page logic was refactored in iOS 16 to support UIKit's new Find & Replace
API. As part of these changes, find-in-page began storing/restoring ranges, rather
than simply updating selection. Consequently, the `TextIndicator` used to draw
highlights is created from a `SimpleRange`, rather than the current selection.
The previously used constructor would early return if the selection was empty
(as is the case when attempting to select a "-webkit-user-select: none" element),
resulting the the pre-iOS 16 behavior described above.

`TextIndicator` does not know how to take snapshots of "-webkit-user-select: none"
content. Consequently, the highlight is painted without any text, leading to
found text getting obscured.

For now, restore the pre-iOS 16 behavior, and do not draw highlights if the
selection is empty. In the longer term, `TextIndicator` should be taught to
draw this content (tracked in webkit.org/b/251709). Note that following this
change, there is still a net progression over pre-iOS 16 behavior, where
WebKit will indicate found results with "-webkit-user-select: none" by drawing
"holes".

* Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:
(WebKit::WebFoundTextRangeController::drawRect):

Additionally, move the use of `GraphicsContext` closer to the painting logic to
avoid running unnecessary code.

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

Committed 259857@main (ed56087): https://commits.webkit.org/259857@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit ed56087 into WebKit:main Feb 4, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Bugs Unclassified bugs are placed in this component until the correct component can be determined.
Projects
None yet
5 participants