Skip to content

Refactor common FocusController code path to be site-isolation aware #47162

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 25, 2025
Merged

Refactor common FocusController code path to be site-isolation aware #47162

merged 1 commit into from
Jun 25, 2025
+89 −53

Conversation

beidson
Copy link
Contributor

@beidson beidson commented Jun 25, 2025 •
edited by webkit-early-warning-system
Loading

5ae55be

Refactor common FocusController code path to be site-isolation aware
rdar://154258045
https://bugs.webkit.org/show_bug.cgi?id=294951

Reviewed by Ryosuke Niwa.

To prepare for making "advancing focus" to be able to cross frame boundaries with site isolation enabled,
this patch refactors the relevant code path of FocusController to include more context about an attempted
focus operation.

With site isolation disabled, this doesn't change behavior.

With site isolation enabled, you'll see the added log statements fire when you try certain operations into
an isolated iframe.

* Source/WebCore/editing/cocoa/AutofillElements.cpp:
(WebCore::nextAutofillableElement):
(WebCore::previousAutofillableElement):
* Source/WebCore/page/FocusController.cpp:
(WebCore::FocusController::findFocusableElementDescendingIntoSubframes):
(WebCore::FocusController::advanceFocusInDocumentOrder):
(WebCore::FocusController::findFocusableElementAcrossFocusScope):
(WebCore::FocusController::findFocusableElementWithinScope):
(WebCore::FocusController::nextFocusableElementWithinScope):
(WebCore::FocusController::previousFocusableElementWithinScope):
(WebCore::FocusController::nextFocusableElement):
(WebCore::FocusController::previousFocusableElement):
(WebCore::shouldClearSelectionWhenChangingFocusedElement):
(WebCore::updateFocusCandidateIfNeeded):
* Source/WebCore/page/FocusController.h:
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::nextAssistableElement):
* Source/WebKitLegacy/mac/DOM/DOM.mm:
(-[DOMNode nextFocusNode]):
(-[DOMNode previousFocusNode]):

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

36029b0

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

@beidson beidson requested review from cdumez and rniwa as code owners June 25, 2025 04:24
@beidson beidson self-assigned this Jun 25, 2025
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash fd280e4)

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
⏳ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
❌ 🛠 tv   🛠 mac-safer-cpp
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

rniwa
rniwa reviewed Jun 25, 2025
View reviewed changes
@jelee53 jelee53 self-requested a review June 25, 2025 04:51
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@rniwa rniwa requested a review from jelee53 June 25, 2025 05:09
@beidson beidson removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@beidson beidson force-pushed the eng/Refactor-common-FocusController-code-path-to-be-site-isolation-aware branch from fd280e4 to 6d1b022 Compare June 25, 2025 05:27
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 6d1b022)

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
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv ⏳ 🛠 mac-safer-cpp
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@jelee53
Copy link
Contributor

jelee53 commented Jun 25, 2025

I took a look at this and it LGTM! Thanks for inviting me to informally review Brady!

@beidson beidson force-pushed the eng/Refactor-common-FocusController-code-path-to-be-site-isolation-aware branch from 6d1b022 to 2988697 Compare June 25, 2025 05:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 2988697)

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
⏳ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ⏳ 🛠 playstation
❌ 🛠 tv   🛠 mac-safer-cpp
❌ 🛠 tv-sim
  🛠 watch
❌ 🛠 watch-sim

@beidson beidson force-pushed the eng/Refactor-common-FocusController-code-path-to-be-site-isolation-aware branch from 2988697 to 2415e06 Compare June 25, 2025 05:57
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on previous version of this PR (hash 2415e06)

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
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 playstation
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

lilyspiniolas
lilyspiniolas reviewed Jun 25, 2025
View reviewed changes
lilyspiniolas
lilyspiniolas reviewed Jun 25, 2025
View reviewed changes
@beidson beidson force-pushed the eng/Refactor-common-FocusController-code-path-to-be-site-isolation-aware branch from 2415e06 to 36029b0 Compare June 25, 2025 15:22
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 25, 2025 •
edited
Loading

EWS run on current version of this PR (hash 36029b0)

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

rniwa
rniwa reviewed Jun 25, 2025
View reviewed changes
Source/WebCore/page/FocusController.h
@@ -50,6 +50,12 @@ class TreeScope;

struct FocusCandidate;

enum class ContinuedSearchInRemoteFrame : bool { No, Yes };
Copy link
Member

Choose a reason for hiding this comment

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

I still think this should be ContinueSearchInRemoteFrame because a function which returns this value didn't continue the search in a remote frame. It's requesting that the caller continue its search in a remote frame as I understand it.

Copy link
Contributor Author

@beidson beidson Jun 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know it looks like that with just this patch, but this is just the first patch in a series - In the larger set of completed work, the continued search has already begun.

@beidson beidson added the merge-queue Applied to send a pull request to merge-queue label Jun 25, 2025
@beidson
Refactor common FocusController code path to be site-isolation aware
5ae55be 
rdar://154258045
https://bugs.webkit.org/show_bug.cgi?id=294951

Reviewed by Ryosuke Niwa.

To prepare for making "advancing focus" to be able to cross frame boundaries with site isolation enabled,
this patch refactors the relevant code path of FocusController to include more context about an attempted
focus operation.

With site isolation disabled, this doesn't change behavior.

With site isolation enabled, you'll see the added log statements fire when you try certain operations into
an isolated iframe.

* Source/WebCore/editing/cocoa/AutofillElements.cpp:
(WebCore::nextAutofillableElement):
(WebCore::previousAutofillableElement):
* Source/WebCore/page/FocusController.cpp:
(WebCore::FocusController::findFocusableElementDescendingIntoSubframes):
(WebCore::FocusController::advanceFocusInDocumentOrder):
(WebCore::FocusController::findFocusableElementAcrossFocusScope):
(WebCore::FocusController::findFocusableElementWithinScope):
(WebCore::FocusController::nextFocusableElementWithinScope):
(WebCore::FocusController::previousFocusableElementWithinScope):
(WebCore::FocusController::nextFocusableElement):
(WebCore::FocusController::previousFocusableElement):
(WebCore::shouldClearSelectionWhenChangingFocusedElement):
(WebCore::updateFocusCandidateIfNeeded):
* Source/WebCore/page/FocusController.h:
* Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:
(WebKit::nextAssistableElement):
* Source/WebKitLegacy/mac/DOM/DOM.mm:
(-[DOMNode nextFocusNode]):
(-[DOMNode previousFocusNode]):

Canonical link: https://commits.webkit.org/296625@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Refactor-common-FocusController-code-path-to-be-site-isolation-aware branch from 36029b0 to 5ae55be Compare June 25, 2025 17:36
@webkit-commit-queue
Copy link
Collaborator

Committed 296625@main (5ae55be): https://commits.webkit.org/296625@main

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

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

@lilyspiniolas lilyspiniolas lilyspiniolas left review comments

@rniwa rniwa rniwa approved these changes

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

@jelee53 jelee53 Awaiting requested review from jelee53

Assignees

@beidson beidson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@beidson @webkit-early-warning-system @jelee53 @webkit-commit-queue @rniwa @lilyspiniolas @webkit-ews-buildbot