New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 862701 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Settings: search does not gain focus with Ctrl+F (⌘+F for Mac)

Project Member Reported by aee@chromium.org, Jul 11

Issue description

Navigate to chrome://settings.
Click on background to take away focus from the search box.
Press Ctrl+F.

Expected: the search box gains focus.
Observed: the search box does not gain focus.

Note that when a control has focus for instance when tab is pressed to focus on a control, Ctrl+F works as instended.

 
Cc: dbeam@chromium.org
Downloads and History have their own set up that works with the use of the command tag. Extensions and Bookmarks do not have the Ctrl+F focuses search bar implemented.

We should probably replace the use of the command tag implementation with a behavior like FindShortcutBehavior that includes the logic that keeps tracks of open dialogs. And also we should add the same functionality in Extensions and Bookmarks.
For the points raised at #2, I think we should continue this duscussion at  issue 819770  and rename the title to be more generic title, since it already discusses the differences across pages, see [1].

Regarding <command> tags, I recall that dbeam's original implementation was using them in Settings, but then it was changed later in the CL, see [2].

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=819770#c5
[2] https://chromium-review.googlesource.com/c/chromium/src/+/956901/2/chrome/browser/resources/settings/settings.html#26
I don't follow the reasoning to change the title. This issue is specific to Ctrl+F not working on the settings page. The 819770 issue pertains to search on the chrome://settings/password page (a page with two search boxes, three if the page search box is shown).

Under 819770, FindShortcutBehavior was added. I'll need to investigate what is remaining for 819770 to be fixed. Some of the comments refer to the ability to focus on a contextual search box in a dialog. This does not seem to directly pertain to the passwords subpage. Though the existence of a subpage search could be focused in the same way.

I'll create a new issue to capture #2.

Created the follow-up issue here https://bugs.chromium.org/p/chromium/issues/detail?id=862839
>  issue 819770  and rename the title to be more generic title, since it already discusses the differences across pages, see [1].

Sorry for the confusion. I was suggesting renaming the title of  issue 819770  to be more generic, and leave this issue (862701) as-is.

The contextual search mechanism was designed such that it can be used in different contexts
1) top-level, like settings-ui.
2) Within a dialog that has its own search box, like the add language dialog (already used there).
3) A subpage that has a dedicated searchbox, like search engines, or passwords (not used currently AfAIK).
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/123398402602d1d9259cc9e3b71f50eae20c2415

commit 123398402602d1d9259cc9e3b71f50eae20c2415
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Thu Jul 12 02:14:38 2018

Settings: fix find in page shortcut handling

Bug:  862701 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I10c6f5888d53e0faf055adf9a45216bd83a71767
Reviewed-on: https://chromium-review.googlesource.com/1133969
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574464}
[modify] https://crrev.com/123398402602d1d9259cc9e3b71f50eae20c2415/chrome/browser/resources/settings/settings.html
[modify] https://crrev.com/123398402602d1d9259cc9e3b71f50eae20c2415/chrome/browser/resources/settings/settings_ui/settings_ui.html

Status: Fixed (was: Started)
Status: Assigned (was: Fixed)
Needed to revert. The fix in comment #7 caused a couple bad regressions ( https://crbug.com/863807 ).


Project Member

Comment 10 by bugdroid1@chromium.org, Jul 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4cb0e4c6372f37e2c9302ac3302a0078d623af4

commit b4cb0e4c6372f37e2c9302ac3302a0078d623af4
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Thu Jul 19 20:22:34 2018

Revert "Settings: fix find in page shortcut handling"

This reverts commit 123398402602d1d9259cc9e3b71f50eae20c2415.

Reason for revert:  https://crbug.com/863807 

Original change's description:
> Settings: fix find in page shortcut handling
> 
> Bug:  862701 
> Cq-Include-Trybots: luci.chromium.try:closure_compilation
> Change-Id: I10c6f5888d53e0faf055adf9a45216bd83a71767
> Reviewed-on: https://chromium-review.googlesource.com/1133969
> Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574464}

TBR=dpapad@chromium.org,aee@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  862701 
Change-Id: If46c83aa4d65a927709b3c89470b61a6378ffade
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Reviewed-on: https://chromium-review.googlesource.com/1144000
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576605}
[modify] https://crrev.com/b4cb0e4c6372f37e2c9302ac3302a0078d623af4/chrome/browser/resources/settings/settings.html
[modify] https://crrev.com/b4cb0e4c6372f37e2c9302ac3302a0078d623af4/chrome/browser/resources/settings/settings_ui/settings_ui.html

Status: Started (was: Assigned)
Proposed CL to address this https://chromium-review.googlesource.com/c/chromium/src/+/1141157.
Cc: vamshi.k...@techmahindra.com
 Issue 784161  has been merged into this issue.
Cc: tsergeant@chromium.org est...@chromium.org tbuck...@chromium.org
 Issue 619829  has been merged into this issue.
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8707c01bdbaef12bb57972a40bffef7eed04187b

commit 8707c01bdbaef12bb57972a40bffef7eed04187b
Author: Esmael El-Moslimany <aee@chromium.org>
Date: Mon Jul 30 23:16:55 2018

Settings: register find key event listeners on the window

Add find shortcut handling to sub-pages with search.

Bug:  862701 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I0db88c2712cac918f1be18068fe02df5d942e53d
Reviewed-on: https://chromium-review.googlesource.com/1141157
Commit-Queue: Esmael El-Moslimany <aee@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579225}
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/BUILD.gn
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/find_shortcut_behavior.html
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/find_shortcut_behavior.js
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/languages_page/add_languages_dialog.js
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/settings_page/BUILD.gn
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/settings_page/settings_subpage.html
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/settings_page/settings_subpage.js
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/browser/resources/settings/settings_ui/settings_ui.js
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/test/data/webui/settings/cr_settings_browsertest.js
[add] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/test/data/webui/settings/find_shortcut_behavior_test.js
[modify] https://crrev.com/8707c01bdbaef12bb57972a40bffef7eed04187b/chrome/test/data/webui/settings/settings_ui_browsertest.js

Status: Fixed (was: Started)
The current implementation only considers the last listener added as the active listener. If that active listener does not handle the find shortcut (by returning false in the handler method), the other listeners are not tried.

If the search box of the active listener already has focus, it might be useful to indicate that the find shortcut was not handled and let the focus move up to the next listener in the stack. If we wanted to support only allowing the focus to only proceed in one direction (from last added to first added listener), we would possibly need another method on the FindShortcutBehavior to indicate if the search box for that listener already has focus.

Also it might be useful to consider situations where listeners are removed out of order. In the current usage, there is an established nesting of listeners (settings-ui>settings-subpage>dialog or settings-ui>dialog).

Sign in to add a comment