Settings: search does not gain focus with Ctrl+F (⌘+F for Mac) |
||||||
Issue descriptionNavigate 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.
,
Jul 11
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.
,
Jul 12
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
,
Jul 12
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.
,
Jul 12
Created the follow-up issue here https://bugs.chromium.org/p/chromium/issues/detail?id=862839
,
Jul 12
> 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).
,
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
,
Jul 12
,
Jul 19
Needed to revert. The fix in comment #7 caused a couple bad regressions ( https://crbug.com/863807 ).
,
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
,
Jul 19
Proposed CL to address this https://chromium-review.googlesource.com/c/chromium/src/+/1141157.
,
Jul 25
,
Jul 27
Issue 619829 has been merged into this issue.
,
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
,
Jul 30
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 |
||||||
Comment 1 by dpa...@chromium.org
, Jul 11