Skip to content

Auto-expand “details” element for find-in-page and scrolling to fragments #43558

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

Merged

Conversation

nt1m
Copy link
Member

@nt1m nt1m commented Apr 3, 2025

23cdbeb

Auto-expand “details” element for find-in-page and scrolling to fragments
https://bugs.webkit.org/show_bug.cgi?id=228843
rdar://81856620

Reviewed by Darin Adler.

This change causes any closed <details> element to be opened
automatically (auto-expanded) if either:

- the user is using find-in-page and a descendant node of the closed
  <details> element contains a match for the given search string
- the user is navigating to a fragment ID and the fragment ID is for a
  descendant element of the closed <details> element

…per the requirements in the HTML spec’s Find-in-page section at
https://html.spec.whatwg.org/#interaction-with-details-and-hidden=until-found

Otherwise, without this change, when a descendant node of a closed
<details> element contains a match for the given find-in-page search
string, or when navigating to a fragment ID for a descendant element of
a closed <details> element, the closed <details> element isn’t opened
automatically (not auto-expanded).

Changes to TextIterator ensure that content inside details elements are
not skipped over for the purposes of find-in-page.

This change also introduces the DetailsAutoExpandEnabled preference
(disabled by default), for controlling whether <details> elements get
auto-expanded under the conditions described above.

And this also adds a testRunner.indicateMatchIndex(index) function —
to emulate jumping through matches in the non-findString codepath.

Original PR by: Michael[tm] Smith <[email protected]>

* LayoutTests/TestExpectations:
* LayoutTests/editing/find/cocoa/find-and-replace-in-closed-details-expected.txt: Added.
* LayoutTests/editing/find/cocoa/find-and-replace-in-closed-details.html: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-closed-details-expected.txt: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-closed-details.html: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-summary-of-closed-details-expected.txt: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-summary-of-closed-details.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-details-element/auto-expand-details-element-fragment-expected.txt:
* LayoutTests/platform/mac-wk1/TestExpectations:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Headers.cmake:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/editing/TextIterator.cpp:
(WebCore::TextIterator::TextIterator):
(WebCore::isRendererAccessible):
(WebCore::isConsideredSkippedContent):
* Source/WebCore/editing/TextIteratorBehavior.h:
* Source/WebCore/html/HTMLDetailsElement.cpp:
(WebCore::revealClosedDetailsAncestors):
* Source/WebCore/html/HTMLDetailsElement.h:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::scrollToFragmentInternal):
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/RenderStyleInlines.h:
(WebCore::RenderStyle::autoRevealsWhenFound const):
* Source/WebCore/rendering/style/RenderStyleSetters.h:
(WebCore::RenderStyle::setAutoRevealsWhenFound):
* Source/WebCore/rendering/style/StyleRareInheritedData.cpp:
(WebCore::StyleRareInheritedData::StyleRareInheritedData):
(WebCore::StyleRareInheritedData::operator== const):
(WebCore::StyleRareInheritedData::dumpDifferences const):
* Source/WebCore/rendering/style/StyleRareInheritedData.h:
* Source/WebCore/style/StyleAdjuster.cpp:
(WebCore::Style::Adjuster::adjust const):
* Source/WebKit/UIProcess/API/C/WKPage.cpp:
(WKPageIndicateFindMatch):
* Source/WebKit/UIProcess/API/C/WKPage.h:
* Source/WebKit/WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::didFindString):
* Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:
(WebKit::WebFoundTextRangeController::decorateTextRangeWithStyle):
* Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm:
(WebKit::FindController::didFindString):
* Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
* Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
(WTR::postPageMessage):
* Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::indicateFindMatch):
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle):

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

79318c2

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
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 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 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch
✅ 🛠 watch-sim

@nt1m nt1m requested review from cdumez, rniwa and JonWBedard as code owners April 3, 2025 19:02
@nt1m nt1m self-assigned this Apr 3, 2025
@nt1m nt1m added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Apr 3, 2025
@nt1m nt1m marked this pull request as draft April 3, 2025 19:04
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 660e5ec to 47cffb9 Compare April 3, 2025 20:10
@nt1m nt1m marked this pull request as ready for review April 3, 2025 20:10
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 47cffb9 to b055db9 Compare April 3, 2025 20:12
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from b055db9 to 48b3402 Compare April 3, 2025 20:13
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 48b3402 to b1149db Compare April 3, 2025 20:28
@nt1m nt1m requested review from pxlcoder, darinadler and annevk and removed request for JonWBedard April 3, 2025 20:51
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Apr 3, 2025
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Apr 4, 2025
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from b1149db to d81e33c Compare April 4, 2025 02:50
Copy link
Member

@darinadler darinadler left a comment

Choose a reason for hiding this comment

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

Will this work for the find command that’s implemented in Safari?

@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Jun 25, 2025
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from d81e33c to 3e71e35 Compare June 25, 2025 17:08
@nt1m nt1m requested a review from darinadler June 26, 2025 20:35
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from e76bb53 to c2ba414 Compare June 26, 2025 20:35
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 26, 2025
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Jun 26, 2025
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from c2ba414 to c11fc64 Compare June 26, 2025 23:54
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from c11fc64 to 87f98d0 Compare June 27, 2025 00:02
@nt1m nt1m added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Jun 27, 2025
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 87f98d0 to 31d9861 Compare June 27, 2025 00:06
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 27, 2025
@nt1m nt1m removed the merging-blocked Applied to prevent a change from being merged label Jun 27, 2025
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 31d9861 to 8430e86 Compare June 27, 2025 01:41
@@ -218,4 +219,26 @@ void HTMLDetailsElement::toggleOpen()
setBooleanAttribute(HTMLNames::openAttr, !hasAttributeWithoutSynchronization(HTMLNames::openAttr));
}

// https://html.spec.whatwg.org/#ancestor-details-revealing-algorithm
Copy link
Member

Choose a reason for hiding this comment

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

The algorithm in the spec does the toggling during the iteration, not separately after building a list of details elements. Is that observably different in any unusual edge cases?

Copy link
Member Author

@nt1m nt1m Jun 27, 2025

Choose a reason for hiding this comment

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

It doesn't make a difference. I was just being cautious since we usually don't mutate during iteration, but it's probably more relevant when you use the special iterators, which is not the case now that I switched to a while loop

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to match the spec

@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from 8430e86 to aa8c8c0 Compare June 27, 2025 05:13
@nt1m nt1m force-pushed the eng/Implement-auto-expanding-details branch from aa8c8c0 to 79318c2 Compare June 27, 2025 05:24
@nt1m nt1m added the merge-queue Applied to send a pull request to merge-queue label Jun 27, 2025
…ents

https://bugs.webkit.org/show_bug.cgi?id=228843
rdar://81856620

Reviewed by Darin Adler.

This change causes any closed <details> element to be opened
automatically (auto-expanded) if either:

- the user is using find-in-page and a descendant node of the closed
  <details> element contains a match for the given search string
- the user is navigating to a fragment ID and the fragment ID is for a
  descendant element of the closed <details> element

…per the requirements in the HTML spec’s Find-in-page section at
https://html.spec.whatwg.org/#interaction-with-details-and-hidden=until-found

Otherwise, without this change, when a descendant node of a closed
<details> element contains a match for the given find-in-page search
string, or when navigating to a fragment ID for a descendant element of
a closed <details> element, the closed <details> element isn’t opened
automatically (not auto-expanded).

Changes to TextIterator ensure that content inside details elements are
not skipped over for the purposes of find-in-page.

This change also introduces the DetailsAutoExpandEnabled preference
(disabled by default), for controlling whether <details> elements get
auto-expanded under the conditions described above.

And this also adds a testRunner.indicateMatchIndex(index) function —
to emulate jumping through matches in the non-findString codepath.

Original PR by: Michael[tm] Smith <[email protected]>

* LayoutTests/TestExpectations:
* LayoutTests/editing/find/cocoa/find-and-replace-in-closed-details-expected.txt: Added.
* LayoutTests/editing/find/cocoa/find-and-replace-in-closed-details.html: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-closed-details-expected.txt: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-closed-details.html: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-summary-of-closed-details-expected.txt: Added.
* LayoutTests/editing/text-iterator/find-in-page-in-summary-of-closed-details.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-details-element/auto-expand-details-element-fragment-expected.txt:
* LayoutTests/platform/mac-wk1/TestExpectations:
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/Headers.cmake:
* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/editing/TextIterator.cpp:
(WebCore::TextIterator::TextIterator):
(WebCore::isRendererAccessible):
(WebCore::isConsideredSkippedContent):
* Source/WebCore/editing/TextIteratorBehavior.h:
* Source/WebCore/html/HTMLDetailsElement.cpp:
(WebCore::revealClosedDetailsAncestors):
* Source/WebCore/html/HTMLDetailsElement.h:
* Source/WebCore/page/LocalFrameView.cpp:
(WebCore::LocalFrameView::scrollToFragmentInternal):
* Source/WebCore/rendering/style/RenderStyle.h:
* Source/WebCore/rendering/style/RenderStyleInlines.h:
(WebCore::RenderStyle::autoRevealsWhenFound const):
* Source/WebCore/rendering/style/RenderStyleSetters.h:
(WebCore::RenderStyle::setAutoRevealsWhenFound):
* Source/WebCore/rendering/style/StyleRareInheritedData.cpp:
(WebCore::StyleRareInheritedData::StyleRareInheritedData):
(WebCore::StyleRareInheritedData::operator== const):
(WebCore::StyleRareInheritedData::dumpDifferences const):
* Source/WebCore/rendering/style/StyleRareInheritedData.h:
* Source/WebCore/style/StyleAdjuster.cpp:
(WebCore::Style::Adjuster::adjust const):
* Source/WebKit/UIProcess/API/C/WKPage.cpp:
(WKPageIndicateFindMatch):
* Source/WebKit/UIProcess/API/C/WKPage.h:
* Source/WebKit/WebProcess/WebPage/FindController.cpp:
(WebKit::FindController::didFindString):
* Source/WebKit/WebProcess/WebPage/WebFoundTextRangeController.cpp:
(WebKit::WebFoundTextRangeController::decorateTextRangeWithStyle):
* Source/WebKit/WebProcess/WebPage/ios/FindControllerIOS.mm:
(WebKit::FindController::didFindString):
* Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:
* Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
(WTR::postPageMessage):
* Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h:
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::indicateFindMatch):
* Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:
* Tools/WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle):

Canonical link: https://commits.webkit.org/296708@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Implement-auto-expanding-details branch from 79318c2 to 23cdbeb Compare June 27, 2025 06:15
@webkit-commit-queue
Copy link
Collaborator

Committed 296708@main (23cdbeb): https://commits.webkit.org/296708@main

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

@webkit-commit-queue webkit-commit-queue merged commit 23cdbeb into WebKit:main Jun 27, 2025
@webkit-commit-queue webkit-commit-queue removed merge-queue Applied to send a pull request to merge-queue safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jun 27, 2025
@nt1m nt1m deleted the eng/Implement-auto-expanding-details branch June 27, 2025 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants