Closed
Bug 1195038
Opened 9 years ago
Closed 9 years ago
In about:home, make accel+f focus the search box instead of invoking the find bar
Categories
(Firefox :: Keyboard Navigation, enhancement)
Firefox
Keyboard Navigation
Tracking
()
RESOLVED
FIXED
Firefox 43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: steffen.wilberg, Assigned: steffen.wilberg)
References
Details
Attachments
(1 file)
40 bytes,
text/x-review-board-request
|
Gijs
:
review+
phlsa
:
ui-review+
|
Details |
In about:home, accel+f should focus the search box instead of invoking the find bar.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak
Attachment #8648424 -
Flags: review?(mak77)
Assignee | ||
Comment 2•9 years ago
|
||
https://reviewboard.mozilla.org/r/16203/#review14475 ::: browser/base/content/abouthome/aboutHome.xhtml:46 (Diff revision 1) > + dir="auto" data-findkey="&find.commandkey;"/> I stored the localized string in a HTML data attribute, so that I can retrieve it from non-privileged javascript. Is there a better/recommended way to do this?
Comment 3•9 years ago
|
||
I'm currently on vacation, please ask for review to :adw or :gijs if you want a quicker reply.
Assignee | ||
Comment 4•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak
Attachment #8648424 -
Flags: review?(mak77) → review?(gijskruitbosch+bugs)
Comment 5•9 years ago
|
||
Can you elaborate on why we should do this? I mean, Accel+f is associated with "find in page" - why should we dilute that association by making it do something completely different (search the web) on about:home? Wouldn't Accel+k be a better choice for what you want to do? FWIW, Chrome has the same behavior for Accel+f that we do in its corresponding "new tab" page that has a Google search box. It invokes find-in-page so that you can search the titles in the thumbnails presented there. I find this to be a useful feature.
Assignee | ||
Comment 6•9 years ago
|
||
One argument is indeed consistency: We use accel+f to focus the search/filter box in preferences (cookie viewer; passwords manager; I just fixed Applications; bug 1192540 fixes the regression in about:config; bug 661831 introduces that to about:permissions) and the Add-ons manager (to be fixed by bug 1195060). You might argue that most of those operate on the same page though (with the exception being the Add-ons manager, which searches on addons.mozilla.org), so this argument is rather weak. A stronger argument is usefulness: On about:home, there is nothing to "find" on the page itself: You can't search for the buttons on the bottom and hit enter to activate them, even after closing the find bar. So the only useful action is to search on the web. Likewise, on about:newtab, after finding a tile you need to press Esc to close the findbar to be able to activate the tile by pressing enter. I'd consider this a rather rare usecase because of that. But I actually like the idea of stealing accel+k to focus the search box of about:home and about:newtab instead of focussing the search bar, because these 3 searches act and look exactly the same nowadays, with the benefit that the text is larger on about:home and about:newtab.
Comment 7•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. https://reviewboard.mozilla.org/r/16205/#review14493 ::: browser/base/content/abouthome/aboutHome.js:24 (Diff revision 1) > +var searchText, findKey; Nit: let ::: browser/base/content/abouthome/aboutHome.js:59 (Diff revision 1) > +window.addEventListener("keypress", (ev) => { Nit: you don't need braces around (ev) here. ::: browser/base/content/abouthome/aboutHome.js:61 (Diff revision 1) > + if (ev.getModifierState("Accel") && ev.key == findKey) { This doesn't check other modifier states, which means if other things take accel-shift-<same-shortcut-key> or accel-opt/alt-<same-shortcut-key>, this will steal those, too, which it shouldn't do.
Attachment #8648424 -
Flags: review?(gijskruitbosch+bugs)
Comment 8•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. I'd want UX to chime in here before taking this regardless. I am not entirely convinced it is a good idea.
Attachment #8648424 -
Flags: ui-review?(philipp)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Attachment #8648424 -
Attachment description: MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r?mak → MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Attachment #8648424 -
Flags: review?(gijskruitbosch+bugs)
Comment 10•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. https://reviewboard.mozilla.org/r/16205/#review15279 With the below addressed, r=me, but you still need a ui-review. ::: browser/base/content/abouthome/aboutHome.js:61 (Diff revision 2) > + if (ev.getModifierState("Accel") && !ev.shiftKey && !ev.altKey This isn't checking for ctrl on osx or meta on windows/linux.
Attachment #8648424 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 11•9 years ago
|
||
https://reviewboard.mozilla.org/r/16205/#review15279 > This isn't checking for ctrl on osx or meta on windows/linux. OK, I can fix it like this: let modifiers = ev.ctrlKey + ev.altKey + ev.shiftKey + ev.metaKey; if (ev.getModifierState("Accel") && modifiers == 1 && ev.key == findKey) {
Comment 12•9 years ago
|
||
(In reply to Steffen Wilberg from comment #11) > https://reviewboard.mozilla.org/r/16205/#review15279 > > > This isn't checking for ctrl on osx or meta on windows/linux. > > OK, I can fix it like this: > let modifiers = ev.ctrlKey + ev.altKey + ev.shiftKey + ev.metaKey; > if (ev.getModifierState("Accel") && modifiers == 1 && ev.key == findKey) { WFM!
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=927074e20f94
Comment 14•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. WFM too!
Attachment #8648424 -
Flags: ui-review?(philipp) → ui-review+
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar.
Assignee | ||
Comment 17•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. I added a test, please have a look.
Flags: needinfo?(steffen.wilberg)
Updated•9 years ago
|
Attachment #8648424 -
Flags: review+ → review?(gijskruitbosch+bugs)
Comment 18•9 years ago
|
||
Comment on attachment 8648424 [details] MozReview Request: Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. https://reviewboard.mozilla.org/r/16205/#review16537 LGTM but for the nit below. ::: browser/base/content/test/general/browser_aboutHome.js:436 (Diff revision 3) > + CustomizableUI.reset(); You don't need this line - we need it in the test you copy-pasted because setup() moves the searchbar - but you don't do that.
Attachment #8648424 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 19•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e1ff9d517b133e5f0022a2dee70e88c5c9573ad7 Bug 1195038 - In about:home, make accel+f focus the search box instead of invoking the find bar. r=gijs, ui-review=philipp
Comment 20•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e1ff9d517b13
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment 21•9 years ago
|
||
Well, it just doesn't work. Ctrl+F still triggers findbar on about:home. Should this be reopened?
> My info: Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode
> video: https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0
(In the end of video I just showed inconsist behavior - "Find" button isn't disabled on homepage)
Flags: needinfo?(steffen.wilberg)
Comment 22•9 years ago
|
||
(In reply to arni2033 from comment #21) > Well, it just doesn't work. Ctrl+F still triggers findbar on about:home. > Should this be reopened? No, see bug 1204836. > > My info: Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode > > video: https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0 > (In the end of video I just showed inconsist behavior - "Find" button isn't > disabled on homepage) FWIW, I can reproduce this if (and only if) focus is not inside the content page, which is expected given the patch here.
Comment 23•9 years ago
|
||
(In reply to arni2033 from comment #21) > Well, it just doesn't work. Ctrl+F still triggers findbar on about:home. > Should this be reopened? > > My info: Win7_64, Nightly 44, 32bit, ID 20150922030204, new profile, safe mode > > video: https://dl.dropboxusercontent.com/s/tgr3vsoup6nwc6a/video%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0 > (In the end of video I just showed inconsist behavior - "Find" button isn't > disabled on homepage) FWIW there's something fishy going on in there as well, Ctrl+F isn't supposed to close the Findbar (unless you have an add-on like FindBar Tweak to enable that behavior).
Comment 24•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #22) > FWIW, I can reproduce this if (and only if) focus is not inside the content page, which is expected given the patch here. On the following video it's clear that focus is inside the content. Also, I use Esc to close findbar > video 2: https://dl.dropboxusercontent.com/s/ezbm5buonvrnuh9/video%202%20-%20bug%201195038%20-%20just%20doesn%27t%20work.webm?dl=0 Actually, it's good that this patch will be canceled, because, well, it doesn't work.
Comment 25•9 years ago
|
||
Correction: I figured out that it works, but only on english keyboard layout. So this is basically one of the countless cases in firefox when shortcuts don't work in different locales. That's strange design I personally think that there's no point in enhancements for 1 keyboard layout.
Comment 26•9 years ago
|
||
(In reply to arni2033 from comment #24) > Also, I use Esc to close findbar Obviously! I'm sorry about that. (What's happening with me today? I'm completely off my game...) (In reply to arni2033 from comment #25) > Correction: I figured out that it works, but only on english keyboard > layout. So this is basically one of the countless cases in firefox when > shortcuts don't work in different locales. From your videos I assume you have a russian keyboard. I'm interested in this, do you know if there's some sort of tracking bug for inconsistencies in keyboard behavior like this? (Or maybe Gijs would know?) I wonder what the cause would be: find.commandkey entity localized differently; modifiers != 1; or event.key not actually being the exact same "f" on those layouts for whatever reason.
Comment 27•9 years ago
|
||
(In reply to Luís Miguel [:quicksaver] from comment #26) > From your videos I assume you have a russian keyboard. I'm interested in this, do you know if > there's some sort of tracking bug for inconsistencies in keyboard behavior like this? I only know bug 626594, bug 591935. Also, here's my list of such shortcuts (it may be incomplete): > Ctrl+Shift+Alt+I; In devtools: Ctrl+[, Ctrl+]; In debugger: Ctrl+Alt+V (Ctrl+Alt+F was fixed)
Comment 28•9 years ago
|
||
Sorry, Ctrl+Alt+F wasn't fixed - it still doesn't work if the filter field in debugger is focused
Assignee | ||
Comment 29•9 years ago
|
||
Looks like that is an issue with the localization then. Anyway, I'm changing this in bug 1204836.
Flags: needinfo?(steffen.wilberg)
You need to log in
before you can comment on or make changes to this bug.
Description
•